[Cluster-devel] Re: Make lockdep happy with configfs

[Cluster-devel] Re: Make lockdep happy with configfs

[Joel Becker]

On Mon, Jan 26, 2009 at 12:51:52PM +0100, Louis Rilling wrote:
> Are you going to take one of these approaches to make lockdep and configfs
> cohabit?

	I'm going to look at your patches now and do some review - sorry
it took me so long.
	I do think our discussion with Peter elsewhere may have even
better fruit, so I'm going to hold of on including these just yet.

Joel


-- 

Life's Little Instruction Book #510

	"Count your blessings."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Cluster-devel] Re: [PATCH 1/2] configfs: Silence lockdep on mkdir() and rmdir()

[Joel Becker]

On Thu, Dec 18, 2008 at 07:00:17PM +0100, Louis Rilling wrote:
> >From inside configfs it is not possible to serialize those recursive
> locking with a top-level one, because mkdir() and rmdir() are already
> called with inodes locked by the VFS. So using some
> mutex_lock_nest_lock() is not an option.
> 
> I am proposing two solutions:
> 1) one that wraps recursive mutex_lock()s with
>    lockdep_off()/lockdep_on().
> 2) (as suggested earlier by Peter Zijlstra) one that puts the
>    i_mutexes recursively locked in different classes based on their
>    depth from the top-level config_group created. This
>    induces an arbitrary limit (MAX_LOCK_DEPTH - 2 == 46) on the
>    nesting of configfs default groups whenever lockdep is activated
>    but this limit looks reasonably high. Unfortunately, this also
>    isolates VFS operations on configfs default groups from the others
>    and thus lowers the chances to detect locking issues.
> 
> Nobody likes solution 1), what I can understand.

	Me too :-P

> This patch implements solution 2). However lockdep is still not happy with
> configfs_depend_item(). Next patch reworks the locking of
> configfs_depend_item() and finally makes lockdep happy.

<snip>

>  #define CONFIGFS_ROOT		0x0001
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 8e93341..f21be74 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -94,6 +94,9 @@ 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;
> +#ifdef CONFIG_LOCKDEP
> +	sd->s_depth = -1;
> +#endif
>  	spin_lock(&configfs_dirent_lock);
>  	if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
>  		spin_unlock(&configfs_dirent_lock);
> @@ -176,6 +179,17 @@ static int init_symlink(struct inode * inode)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LOCKDEP
> +static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
> +					  struct configfs_dirent *sd)
> +{
> +	int parent_depth = parent_sd->s_depth;
> +
> +	if (parent_depth >= 0)
> +		sd->s_depth = parent_depth + 1;
> +}
> +#endif
> +
>  static int create_dir(struct config_item * k, struct dentry * p,
>  		      struct dentry * d)
>  {
> @@ -187,6 +201,9 @@ static int create_dir(struct config_item * k, struct dentry * p,
>  		error = configfs_make_dirent(p->d_fsdata, d, k, mode,
>  					     CONFIGFS_DIR | CONFIGFS_USET_CREATING);
>  	if (!error) {
> +#ifdef CONFIG_LOCKDEP
> +		configfs_set_dir_dirent_depth(p->d_fsdata, d->d_fsdata);
> +#endif
>  		error = configfs_create(d, mode, init_dir);
>  		if (!error) {
>  			inc_nlink(p->d_inode);

	Can you change this to provide non-lockdep versions of
functions?  We don't want "#ifdef CONFIG_LOCKDEP" everywhere.  What we
want is the code to call functions unconditionally, and the functions to
do nothing if lockdep is not enabled.  Like:

#ifdef CONFIG_LOCKDEP
static inline void configfs_init_dir_dirent_depth(dirent)
{
    dirent->s_depth = -1;
}

static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
					  struct configfs_dirent *sd)
{
	int parent_depth = parent_sd->s_depth;

	if (parent_depth >= 0)
		sd->s_depth = parent_depth + 1;
}
#else
static inline void configfs_init_dir_dirent_depth(dirent)
{
}

static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
					  struct configfs_dirent *sd)
{
}
#endif

	This makes the callsites much nicer to read:

@@ -94,6 +94,9 @@ 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;
+	configfs_init_dir_dirent_depth(sd);
 	spin_lock(&configfs_dirent_lock);
 	if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
 		spin_unlock(&configfs_dirent_lock);
@@ -187,6 +201,7 @@ static int create_dir(struct config_item * k, struct dentry * p,
 		error = configfs_make_dirent(p->d_fsdata, d, k, mode,
 					     CONFIGFS_DIR | CONFIGFS_USET_CREATING);
 	if (!error) {
+		configfs_set_dir_dirent_depth(p->d_fsdata, d->d_fsdata);
 		error = configfs_create(d, mode, init_dir);
 		if (!error) {
 			inc_nlink(p->d_inode);

	Otherwise, this patch seems pretty straightforward.

Joel

-- 

Life's Little Instruction Book #30

	"Never buy a house without a fireplace."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Cluster-devel] Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy

[Joel Becker]

On Thu, Dec 18, 2008 at 07:00:18PM +0100, Louis Rilling wrote:
> configfs_depend_item() recursively locks all inodes mutex from configfs root to
> the target item, which makes lockdep unhappy. The purpose of this recursive
> locking is to ensure that the item tree can be safely parsed and that the target
> item, if found, is not about to leave.
> 
> This patch reworks configfs_depend_item() locking using configfs_dirent_lock.
> Since configfs_dirent_lock protects all changes to the configfs_dirent tree, and
> protects tagging of items to be removed, this lock can be used instead of the
> inodes mutex lock chain.
> This needs that the check for dependents be done atomically with
> CONFIGFS_USET_DROPPING tagging.
> 
> Now lockdep looks happy with configfs.

	This looks almost, but not quite right.
	In the create path, we do configfs_new_dirent() before we set
sd->s_type.  But configfs_new_dirent() attaches sd->s_sibling.  So, in
aonther thread, configfs_depend_prep() can traverse this s_sibling
without CONFIGFS_USET_CREATING being set.  This turns out to be safe
because CONFIGFS_DIR is also not set - but boy I'd like a comment about
that.
	What if we're in mkdir(2) in one thread and another thread is  
trying to pin the parent directory?  That is, we are inside
configfs_mkdir(parent, new_dentry, mode).  The other thread is doing
configfs_depend_item(subsys, parent).  With this patch, the other thread
will not take parent->i_mutex.  It will happily determine that
parent is part of the tree and bump its s_dependent with no locking.  Is
this OK?
	If it is - isn't this patch good without any other reason?  That
is, aside from the issues of lockdep, isn't it better for
configfs_depend_item() to never have to worry about the VFS locks other
than the configfs root?

Joel


-- 

 The zen have a saying:
 "When you learn how to listen, ANYONE can be your teacher."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127