Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Louis Rilling <Louis.Rilling@kerlabs.com>]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

On Wed, Jun 18, 2008 at 01:11:07PM -0700, Joel Becker wrote:
> On Wed, Jun 18, 2008 at 01:40:43PM +0200, Louis Rilling wrote:
> > The problem is rmdir() of the target item (see below). ATTACHING only protects
> > us from rmdir() of the parent. This is the exact reason why I attach the link to
> > the target in last place, where we know that we won't have to rollback.
> 
> 	Why wouldn't it protect the target, given that detach_prep()
> will be called against the target if it's being rmdir'd?

Because
1/ setting and clearing ATTACHING could badly interact with mkdir()/symlink()
inside the target item (for instance clear the flag before mkdir() has finished
attaching a new item); to avoid this we could use a different flag, but
2/ rmdir() of the target cannot lock the inode of the new symlink's parent like
it does for mkdir(), otherwise we would risk a deadlock with other symlink() and
sys_rename(). This means that rmdir() should retry aggressively, in a busy
waiting loop, or replacing mutex_lock()/mutex_unlock() with yield().

> 
> > 	And AFAICS, creating a VFS object can not hurt as long as we hold the
> > parent i_mutex, right? Otherwise there already is a problem in
> > configfs_attach_item() where a failure in populate_attrs() leads to rollback the
> > creation of the VFS object already created for the item.
> 
> 	We *can* do that, but we try to isolate it - hand-building VFS
> objects is complex and error prone, and I try to isolate that to
> specific cases.  I'd rather avoid it when not necessary.

In the case of symlink(), building a new inode is what all filesystems must do.
The only "bad" side-effect I can figure out of having to rollback is that the
new entry will be visible for a short time until it is removed.

Anyway, do you think that the "solutions" above are more acceptable?

> 
> > > 		spin_lock(&configfs_dirent_lock);
> > > 		parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
> > > 		if (ret) {
> > 
> > Here, if detach_prep() of the target failed because of the link attached above,
> > it had no means to retry. rmdir() of the target fails because of this
> > temporary link, which results in a failing symlink() making rmdir() of the
> > target fail.
> 
> 	How so?  It sees ATTACHING, it gets -EAGAIN, it tries again,
> just like before.  What's different?

See above the reasons for not using ATTACHING on the target.

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]