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

[Louis Rilling]

On Fri, Jun 13, 2008 at 03:34:41PM -0700, Joel Becker wrote:
> > 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.
> 
> 	People *won't* see that.  default groups are populated and
> cleaned under i_mutex.  The race of mkdir vs rmdir isn't about seeing
> partial default groups, it's about the ENOMEM racing the ENOTEMPTY.  It
> doesn't impact lookup or other operations.  We can fix it.  I'm just not
> sure it's worth the complexity (and this is an open question).

It's not that difficult to implement, you may just find it a bit ugly... I hope
to send you a corrected "rename fix" today.

> 
> > 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?
> 
> 	They can't see that.  We take i_mutex in detach_group.  This
> locks out lookup and readdir.  When we're done with detach_group, all
> default groups are gone.

If I understand correctly, lookup() is not called each time userspace does ls,
and in configfs case, it is never called for existing items since the d_cache is
populated as soon as the user creates items. So lookup() does not block 'ls'
during rmdir() (unless it is a lookup for a never accessed attribute). I think
that this is the point that invalidates all my theory about atomicity :)

> 
> > > > 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.
> 
> 	It's not atomic, though, and never has been.  I'm not quite sure
> what you are unsure of here.  Let me try to clarify a little.
> 	Are you worried about two separate runs of the ls(1) command?
> 
>   # ls A/B/C
>   # ls A/B/D
> 
> These can't be atomic, because someone else could rmdir(1) in the
> middle:
> 
>   # ls A/B/C
>   # rmdir A/B
>   # ls A/B/D
>   ls: No such file or directory
> 
> This is perfectly normal, and there is no way to prevent it - it is
> separate entrances to the system call.
> 	Do you mean inside one call?  That is "ls A/B" would print "C"
> but not "D"?  That cannot happen, because we hold B's i_mutex during
> detach_group.  So, if readdir beat us to i_mutex, it lists "C D".  If we
> win, we remove both before releasing B's i_Mutex, and readdir errors
> with ENOENT - we removed B.
> 	I'm not quite sure what inconsistency you are asking about here.

The scenario that made me worry was more:
process 1:
/* PWD=A/B */
# ls C
ls: No such file or directory
/* some sync between process 1 and 2 */
process 2:
/* PWD=A/D */
# ls E
/* ok */
# ls E
ls: No such file or directory

From a user's point of view, this looks as if somebody did 'rmdir A; mkdir A;
rmdir A', while there actually were only 'rmdir A'.

If there were no d_cache, this would be impossible with the current
implementation of detach_prep() locking all default groups. But with the d_cache
this has always been possible.

Anyway, I give up with this (wrong) atomicity concern.

Louis

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