All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joel Becker <Joel.Becker@oracle.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy
Date: Tue, 27 Jan 2009 20:13:53 -0800	[thread overview]
Message-ID: <20090128041353.GF7244@mail.oracle.com> (raw)
In-Reply-To: <1229623218-8056-3-git-send-email-louis.rilling@kerlabs.com>

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



WARNING: multiple messages have this Message-ID (diff)
From: Joel Becker <Joel.Becker@oracle.com>
To: Louis Rilling <louis.rilling@kerlabs.com>
Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
	cluster-devel@redhat.com, swhiteho@redhat.com,
	peterz@infradead.org
Subject: Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy
Date: Tue, 27 Jan 2009 20:13:53 -0800	[thread overview]
Message-ID: <20090128041353.GF7244@mail.oracle.com> (raw)
In-Reply-To: <1229623218-8056-3-git-send-email-louis.rilling@kerlabs.com>

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@oracle.com
Phone: (650) 506-8127

  reply	other threads:[~2009-01-28  4:13 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-12-11 14:20 [Cluster-devel] configfs, dlm_controld & lockdep Steven Whitehouse
2008-12-11 14:20 ` Steven Whitehouse
2008-12-11 14:44 ` Louis Rilling
2008-12-11 17:34   ` [Cluster-devel] " Joel Becker
2008-12-11 17:34     ` Joel Becker
2008-12-12 10:06     ` Louis Rilling
2008-12-12 15:29       ` [PATCH] configfs: Silence lockdep on mkdir(), rmdir() and configfs_depend_item() Louis Rilling
2008-12-17 21:40         ` [Cluster-devel] " Andrew Morton
2008-12-17 21:40           ` Andrew Morton
2008-12-17 22:03           ` [Cluster-devel] " Joel Becker
2008-12-17 22:03             ` Joel Becker
2008-12-17 22:09             ` [Cluster-devel] " Andrew Morton
2008-12-17 22:09               ` Andrew Morton
2008-12-18  7:26           ` Peter Zijlstra
2008-12-18  9:27             ` [Cluster-devel] " Joel Becker
2008-12-18  9:27               ` Joel Becker
2008-12-18 11:15               ` Louis Rilling
2008-12-18 18:00                 ` Make lockdep happy with configfs Louis Rilling
2009-01-26 11:51                   ` Louis Rilling
2009-01-28  3:44                     ` [Cluster-devel] " Joel Becker
2009-01-28  3:44                       ` Joel Becker
2008-12-18 18:00                 ` [PATCH 1/2] configfs: Silence lockdep on mkdir() and rmdir() Louis Rilling
2009-01-28  3:55                   ` [Cluster-devel] " Joel Becker
2009-01-28  3:55                     ` Joel Becker
2009-01-28 10:38                     ` Louis Rilling
2008-12-18 18:00                 ` [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy Louis Rilling
2009-01-28  4:13                   ` Joel Becker [this message]
2009-01-28  4:13                     ` Joel Becker
2009-01-28 10:32                     ` Louis Rilling
2008-12-18 11:26               ` [Cluster-devel] Re: [PATCH] configfs: Silence lockdep on mkdir(), rmdir() and configfs_depend_item() Steven Whitehouse
2008-12-18 11:26                 ` Steven Whitehouse
2008-12-18 11:48                 ` Louis Rilling
2008-12-18 11:56               ` Peter Zijlstra
2008-12-18 12:28                 ` Peter Zijlstra
2008-12-18 22:58                   ` [Cluster-devel] " Joel Becker
2008-12-18 22:58                     ` Joel Becker
2008-12-19 10:29                     ` Louis Rilling
2009-01-26 12:30                     ` Peter Zijlstra
2009-01-26 13:24                       ` Louis Rilling
2009-01-26 13:41                         ` Peter Zijlstra
2009-01-26 14:00                           ` Louis Rilling
2009-01-26 14:19                             ` Peter Zijlstra
2009-01-26 14:55                               ` Louis Rilling
2009-01-28  3:05                                 ` [Cluster-devel] " Joel Becker
2009-01-28  3:05                                   ` Joel Becker
2009-01-28  3:41                       ` [Cluster-devel] " Joel Becker
2009-01-28  3:41                         ` Joel Becker
  -- strict thread matches above, loose matches on Subject: below --
2009-01-28 18:18 [PATCH v2] Make lockdep happy with configfs Louis Rilling
2009-01-28 18:18 ` [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy Louis Rilling
2009-04-29 18:52   ` [Cluster-devel] " Joel Becker
2009-04-30  9:18     ` Louis Rilling
2009-04-30 17:20       ` [Cluster-devel] " Joel Becker
2009-04-30 17:30         ` Joel Becker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090128041353.GF7244@mail.oracle.com \
    --to=joel.becker@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.