Re: [PATCH 2/2] configfs: Rework configfs_depend_item() lockingand make lockdep happy

[Louis Rilling]

On 30/04/09 10:30 -0700, Joel Becker wrote:
> On Thu, Apr 30, 2009 at 10:20:14AM -0700, Joel Becker wrote:
> > On Thu, Apr 30, 2009 at 11:18:28AM +0200, Louis Rilling wrote:
> > > Agreed, I was going to suggest something like this. Actually I'd push the
> > > initialization of s_type down to configfs_new_dirent(), so that s_type either
> > > is always NULL, or always shows the correct type of object.
> > 
> > 	0, not "NULL", but yeah I think that's a good plan.
> 
> 	Like this.  Please review the comment change mostly.

Indeed, the code is exactly what I had in mind. Small comments about the comment
change below.

Thanks!

Acked-by: Louis Rilling <louis.rilling@xxxxxxxxxxx>

> 
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 63d8815..8e48b52 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -167,8 +167,8 @@ configfs_adjust_dir_dirent_depth_after_populate(struct configfs_dirent *sd)
>  /*
>   * Allocates a new configfs_dirent and links it to the parent configfs_dirent
>   */
> -static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * parent_sd,
> -						void * element)
> +static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent *parent_sd,
> +						   void *element, int type)
>  {
>  	struct configfs_dirent * sd;
>  
> @@ -180,6 +180,7 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * pare
>  	INIT_LIST_HEAD(&sd->s_links);
>  	INIT_LIST_HEAD(&sd->s_children);
>  	sd->s_element = element;
> +	sd->s_type = type;
>  	configfs_init_dirent_depth(sd);
>  	spin_lock(&configfs_dirent_lock);
>  	if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
> @@ -225,19 +226,12 @@ int configfs_make_dirent(struct configfs_dirent * parent_sd,
>  {
>  	struct configfs_dirent * sd;
>  
> -	sd = configfs_new_dirent(parent_sd, element);
> +	sd = configfs_new_dirent(parent_sd, element, type);
>  	if (IS_ERR(sd))
>  		return PTR_ERR(sd);
>  
> -	/*
> -	 * We need configfs_dirent_lock so that configfs_depend_prep()
> -	 * can see s_type accurately on other CPUs.
> -	 */
> -	spin_lock(&configfs_dirent_lock);
>  	sd->s_mode = mode;
> -	sd->s_type = type;
>  	sd->s_dentry = dentry;
> -	spin_unlock(&configfs_dirent_lock);
>  	if (dentry) {
>  		dentry->d_fsdata = configfs_get(sd);
>  		dentry->d_op = &configfs_dentry_ops;
> @@ -1034,11 +1028,10 @@ static int configfs_dump(struct configfs_dirent *sd, int level)
>   * dead, as well as items in the middle of attachment since they virtually
>   * do not exist yet. This completes the locking out of racing mkdir() and
>   * rmdir().
> - * Note: items in the middle of attachment start with s_type = 0
> - * (configfs_new_dirent()), and configfs_make_dirent() (called from
> - * create_dir()) sets s_type = CONFIGFS_DIR|CONFIGFS_USET_CREATING. In both
> - * cases the item is ignored.  configfs_make_dirent() is locked out from
> - * updating s_type by configfs_dirent_lock.
> + * Note: subdirectories in the middle of attachment start with s_type =
> + * CONFIGFS_DIR|CONFIGFS_USET_CREATING set by create_dir().  When

I'd say "As long as CONFIGFS_USET_CREATING is set", since, by design, once cleared,
CONFIGFS_USET_CREATING never comes back.

Louis

> + * CONFIGFS_USET_CREATING is set, we ignore the item.  The actual set of
> + * s_type is in configfs_new_dirent(), which has configfs_dirent_lock.
>   *
>   * If the target is not found, -ENOENT is bubbled up.
>   *
> @@ -1514,7 +1507,7 @@ static int configfs_dir_open(struct inode *inode, struct file *file)
>  	 */
>  	err = -ENOENT;
>  	if (configfs_dirent_is_ready(parent_sd)) {
> -		file->private_data = configfs_new_dirent(parent_sd, NULL);
> +		file->private_data = configfs_new_dirent(parent_sd, NULL, 0);
>  		if (IS_ERR(file->private_data))
>  			err = PTR_ERR(file->private_data);
>  		else
> 
> -- 
> 
> "There is no sincerer love than the love of food."  
>          - George Bernard Shaw 
> 
> Joel Becker
> Principal Software Developer
> Oracle
> E-mail: joel.becker@xxxxxxxxxx
> Phone: (650) 506-8127
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Attachment: signature.asc
Description: Digital signature