lkml.org 
[lkml]   [2008]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3][BUGFIX] configfs: Introduce configfs_dirent_lock
On Fri, Jun 13, 2008 at 01:17:46PM -0700, Joel Becker wrote:
> Louis,
> Can I just say, you're the first person to do serious review
> other than myself, and I really appreciate it :-)

It's just that I use configfs in my own work, and I'm playing hard with
it, especially with modules. So I need to understand exactly what it
does, and what is possible with it.

>
> On Fri, Jun 13, 2008 at 12:45:13PM +0200, Louis Rilling wrote:
> > On Thu, Jun 12, 2008 at 07:41:31PM -0700, Joel Becker wrote:
> > Unfortunately, thinking a bit more about it I found some issues with
> > i_mutex lock free detach_prep(), but nothing that can't be fixed ;)
> > Between detach_prep() in A and mkdir() in a default group A/B:
> > detach_prep() can be called in the middle of attach_group(), for instance after
> > having attached A/B/C, but attach_group() may then fail (because of memory
> > pressure for instance) while attaching C's default group A/B/C/D. This would
> > lead to both mkdir(A/B/C) and rmdir(A) failing, the reason for rmdir failure
> > being at best obscure: the user would have expected to either see mkdir succeed
> > and rmdir fail because of the new A/B/C group, or see mkdir fail and rmdir
> > succeed because no user-created group lived under A. Solution: tag A/B with
> > USET_IN_MKDIR on mkdir entrance, remove that tag on mkdir exit, and retry
> > detach_prep() as long as USET_IN_MKDIR is found under A/*.
>
> I see what you are saying here. I'm not sure if that is worth
> the complexity - we can say "it was kind of there". No one will ever
> hit it :-) But let me think about it more.

To me it's an issue only if we want to provide some atomic view to
userspace: either userspace sees a group with all of its default groups,
or it sees none. So the question is: does userspace need such atomicity?
Currently configfs provides it, so this would be a userspace visible
change if we break it.

>
> > Between rmdir() and readdir(): dir_open() might add a configfs_dirent
> > to a default group A/B that detach_prep() already marked with USET_DROPPING.
> > This could result in detach_groups() dropping the dirent and make readdir() in
> > A/B crash afterwards. Solution: check USET_DROPPING in dir_open() and fail if
> > it is set.
>
> I was trying to see why this could happen, given that we can
> come to this from other places - the dir could have been open before we
> set USET_DROPPING. Oh! We actually fail rmdir with ENOTEMPTY when the
> dir is open? That's wrong. Ignore it though - we'll fix it later.
> But back to your concern. configfs_readdir() can't crash for
> two reasons. First, detach_groups() won't remove this dirent. A
> readdir placeholder has s_element==NULL. Note the check in
> detach_groups():
>
> if (!sd->s_element ||
> !(sd->s_type & CONFIGFS_USET_DEFAULT))
> continue;
>
> It skips our readdir placeholder, allowing us to free it in dir_close().

I had not noticed this. Thanks for pointed it out.

> There's another reason this can't be a problem. If we get into
> detach_groups(), we take i_mutex, locking out readdir(). Then we delete
> the directory, setting S_DEAD. In vfs_readdir(), they check
> IS_DEADDIR() after getting i_mutex. So they will see S_DEAD and not
> call our ->readdir(). S_DEAD is important. Someone could actually have
> our default_group as their cwd. S_DEAD prevents them from doing
> anything :-)

As I told you in a previous email, I'm missing some VFS skills, so
thanks again for the explanation.

>
> > Between rmdir() and lookup(): several lookup() called under A/* while
> > rmdir(A) in the middle of detach_groups() could return inconsistent results (for
> > instance some default groups being there and some other ones not). Solution:
> > lock dirent_lock for the whole lookup() duration, check USET_DROPPING of current
> > dir, and fail with ENOENT if it is set.
>
> Nah, we don't care about the spurious lookups. This is a normal
> race of i_mutex. USET_DROPPING is not a way to prevent VFS views from
> changing - it's only a way to prevent new children.
> Remember, ->lookup() comes with i_mutex locking. We hold
> i_mutex during the entire delete, so they can't call ->lookup() until
> we're done with a directory. Conversely, if they win i_mutex and ->lookup()
> a default group, then try to use it after we've removed it, they'll just
> ENOENT. This is evident back in do_rename(). They call lookup, which
> takes and drops locks, then call lock_rename() to get the locks back.
> And they can handle ENOENT at that point.

Sure, my only concern is the atomic view of userspace: can userspace
tolerate that (pwd=A/B, with B a default group of A, B having default groups C
and D, and A being removed) 'ls C' returns error because default group C is
already removed and 'ls D' is ok because default group D is not removed yet?

>
> > I was speaking as if we replaced i_mutex protection with dirent_lock
> > protection for a whole mkdir(), that is taking the lock before attach_* and
> > releasing it after.
>
> Ok. I think that's not the way to go, what you currently have
> is better.

Agreed.

>
> > The intermediate conditions that really matter are:
> > 1/ the existence of partial default groups trees (I mean configfs_dirent trees)
> > in the middle of attach_group() and detach_group(),
>
> This is your first case, the mkdir ENOMEM vs rmdir ENOTEMPTY.

Exactly.

>
> > 2/ the existence of default group trees that are tagged as USET_DROPPING and
> > should be treated as not existing anymore.
>
> This is not an issue. USET_DROPPING does *not* mean it went
> away. It means we're safe to make it go away. We protect the actual
> going-away with i_mutex. And that's normal VFS behavior.

Again this is the concern of atomicity from userspace point of view: to
provide such atomic view, mkdir(), lookup(), readdir(), and probably
attributes open() should just fail when done in a default group flagged with
USET_DROPPING.

Anyway, if atomicity from userspace point of view is not a concern, this
just makes things simpler, and I'm ok with it.

Louis

--
Dr Louis Rilling Kerlabs - IRISA
Skype: louis.rilling Campus Universitaire de Beaulieu
Phone: (+33|0) 2 99 84 71 52 Avenue du General Leclerc
Fax: (+33|0) 2 99 84 71 71 35042 Rennes CEDEX - France
http://www.kerlabs.com/


\
 
 \ /
  Last update: 2008-06-13 23:57    [W:0.074 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site