Re: [PATCH 1/3][BUGFIX] configfs: Introduce configfs_dirent_lock

[Joel Becker <Joel.Becker@oracle.com>]

Louis,
	Can I just say, you're the first person to do serious review
other than myself, and I really appreciate 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.

> 	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().
	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 :-)

> 	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.

> 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.

> > 	I'm not even sure what you said here :-)
> 
> I was just saying that with i_mutex lock free detach_prep(), we have kind of
> optimistic mkdir(), with conflicts resolved as error cases of attach_*.

	Basically, the concerns you had above.

> 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.

> 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.

Joel

-- 

"I don't know anything about music. In my line you don't have
 to."
        - Elvis Presley

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