[ applies on top of the previously submitted rename() vs rmdir() deadlock fix ]
Hi,
The following patchset fixes incorrect symlinks to dead items in configfs, which
are forbidden by specification.
The first patch actually prevents such dangling symlinks from being created, but
introduces a weird(?) behavior where a failing symlink() can make a racing
rmdir() fail in the symlink's parent and in the symlink's target as well. The
next patches prevent this behavior using a similar idea as for the mkdir() vs
rmdir() case previously submitted.
Summary:
configfs: Fix symlink() to a removing item
configfs: Rename CONFIGFS_USET_IN_MKDIR to CONFIGFS_USET_ATTACHING
configfs: Fix failing symlink() making rmdir() fail
fs/configfs/configfs_internal.h | 2 +-
fs/configfs/dir.c | 20 ++++++++++----------
fs/configfs/symlink.c | 33 +++++++++++++++++++++++++++++----
3 files changed, 40 insertions(+), 15 deletions(-)
--
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
The rule for configfs symlinks is that symlinks always point to valid
config_items, and prevent the target from being removed. However,
configfs_symlink() only checks that it can grab a reference on the target item,
without ensuring that it remains alive until the symlink is correctly attached.
This patch makes configfs_symlink() fail whenever the target is being removed,
using the CONFIGFS_USET_DROPPING flag set by configfs_detach_prep() and
protected by configfs_dirent_lock.
This patch introduces a similar (weird?) behavior as with mkdir failures making
rmdir fail: if symlink() races with rmdir() of the parent directory (or its
youngest user-created ancestor if parent is a default group) or rmdir() of the
target directory, and then fails in configfs_create(), this can make the racing
rmdir() fail despite the concerned directory having no user-created entry (resp.
no symlink pointing to it or one of its default groups) in the end.
This behavior is fixed in later patches.
Signed-off-by: Louis Rilling <[email protected]>
---
fs/configfs/dir.c | 14 +++++++-------
fs/configfs/symlink.c | 6 ++++++
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 614e382..f2a12d0 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -370,6 +370,9 @@ static int configfs_detach_prep(struct dentry *dentry, struct mutex **wait_mutex
struct configfs_dirent *sd;
int ret;
+ /* Mark that we're trying to drop the group */
+ parent_sd->s_type |= CONFIGFS_USET_DROPPING;
+
ret = -EBUSY;
if (!list_empty(&parent_sd->s_links))
goto out;
@@ -385,8 +388,6 @@ static int configfs_detach_prep(struct dentry *dentry, struct mutex **wait_mutex
*wait_mutex = &sd->s_dentry->d_inode->i_mutex;
return -EAGAIN;
}
- /* Mark that we're trying to drop the group */
- sd->s_type |= CONFIGFS_USET_DROPPING;
/*
* Yup, recursive. If there's a problem, blame
@@ -414,12 +415,11 @@ static void configfs_detach_rollback(struct dentry *dentry)
struct configfs_dirent *parent_sd = dentry->d_fsdata;
struct configfs_dirent *sd;
- list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
- if (sd->s_type & CONFIGFS_USET_DEFAULT) {
+ parent_sd->s_type &= ~CONFIGFS_USET_DROPPING;
+
+ list_for_each_entry(sd, &parent_sd->s_children, s_sibling)
+ if (sd->s_type & CONFIGFS_USET_DEFAULT)
configfs_detach_rollback(sd->s_dentry);
- sd->s_type &= ~CONFIGFS_USET_DROPPING;
- }
- }
}
static void detach_attrs(struct config_item * item)
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index faeb441..58722a9 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -78,6 +78,12 @@ static int create_link(struct config_item *parent_item,
if (sl) {
sl->sl_target = config_item_get(item);
spin_lock(&configfs_dirent_lock);
+ if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
+ spin_unlock(&configfs_dirent_lock);
+ config_item_put(item);
+ kfree(sl);
+ return -EPERM;
+ }
list_add(&sl->sl_list, &target_sd->s_links);
spin_unlock(&configfs_dirent_lock);
ret = configfs_create_link(sl, parent_item->ci_dentry,
--
1.5.5.3
On a similar pattern as mkdir() vs rmdir(), a failing symlink() may make rmdir()
fail for the symlink's parent and the symlink's target as well.
failing symlink() making target's rmdir() fail:
process 1: process 2:
symlink("A/S" -> "B")
allow_link()
create_link()
attach to "B" links list
rmdir("B")
detach_prep("B")
error because of new link
configfs_create_link("A", "S")
error (eg -ENOMEM)
failing symlink() making parent's rmdir() fail:
process 1: process 2:
symlink("A/D/S" -> "B")
allow_link()
create_link()
attach to "B" links list
configfs_create_link("A/D", "S")
make_dirent("A/D", "S")
rmdir("A")
detach_prep("A")
detach_prep("A/D")
error because of "S"
create("S")
error (eg -ENOMEM)
For the parent's rmdir() case, we can use the same solution as with mkdir() vs
rmdir(). For the target's rmdir() case, we cannot, since we do not and cannot
lock the target's inode while in symlink(). Fortunately, once create_link()
terminates, no further operation can fail in symlink(). So we first reorder the
operations in create_link() to attach the new symlink to its target in last
place, and second handle symlink creation failure the same way as a new item
creation failure.
Signed-off-by: Louis Rilling <[email protected]>
---
fs/configfs/symlink.c | 39 +++++++++++++++++++++++++++++----------
1 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 58722a9..0c5e650 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -69,6 +69,7 @@ static int create_link(struct config_item *parent_item,
struct config_item *item,
struct dentry *dentry)
{
+ struct configfs_dirent *parent_sd = parent_item->ci_dentry->d_fsdata;
struct configfs_dirent *target_sd = item->ci_dentry->d_fsdata;
struct configfs_symlink *sl;
int ret;
@@ -77,24 +78,42 @@ static int create_link(struct config_item *parent_item,
sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
if (sl) {
sl->sl_target = config_item_get(item);
+
spin_lock(&configfs_dirent_lock);
- if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
- spin_unlock(&configfs_dirent_lock);
- config_item_put(item);
- kfree(sl);
- return -EPERM;
- }
- list_add(&sl->sl_list, &target_sd->s_links);
+ /*
+ * Force rmdir() of parent_item to wait until we know if we
+ * succeed.
+ */
+ parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
spin_unlock(&configfs_dirent_lock);
+
ret = configfs_create_link(sl, parent_item->ci_dentry,
dentry);
- if (ret) {
- spin_lock(&configfs_dirent_lock);
- list_del_init(&sl->sl_list);
+
+ spin_lock(&configfs_dirent_lock);
+ parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
+
+ if (ret || target_sd->s_type & CONFIGFS_USET_DROPPING) {
+ struct configfs_dirent *sd = NULL;
+
+ if (!ret) {
+ sd = dentry->d_fsdata;
+ list_del_init(&sd->s_sibling);
+ }
spin_unlock(&configfs_dirent_lock);
+
+ if (!ret) {
+ configfs_drop_dentry(sd, dentry->d_parent);
+ d_delete(dentry);
+ configfs_put(sd);
+ }
config_item_put(item);
kfree(sl);
+ return ret ? ret : -EPERM;
}
+
+ list_add(&sl->sl_list, &target_sd->s_links);
+ spin_unlock(&configfs_dirent_lock);
}
return ret;
--
1.5.5.3
The CONFIGFS_USET_IN_MKDIR flag can be reused with symlink() to solve a similar
issue as mkdir() vs rmdir(). This patch renames the flag to make it more
meaningful for this purpose.
Signed-off-by: Louis Rilling <[email protected]>
---
fs/configfs/configfs_internal.h | 2 +-
fs/configfs/dir.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index da015c1..a28f37d 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -48,7 +48,7 @@ struct configfs_dirent {
#define CONFIGFS_USET_DIR 0x0040
#define CONFIGFS_USET_DEFAULT 0x0080
#define CONFIGFS_USET_DROPPING 0x0100
-#define CONFIGFS_USET_IN_MKDIR 0x0200
+#define CONFIGFS_USET_ATTACHING 0x0200
#define CONFIGFS_NOT_PINNED (CONFIGFS_ITEM_ATTR)
extern spinlock_t configfs_dirent_lock;
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index f2a12d0..629c938 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -383,7 +383,7 @@ static int configfs_detach_prep(struct dentry *dentry, struct mutex **wait_mutex
continue;
if (sd->s_type & CONFIGFS_USET_DEFAULT) {
/* Abort if racing with mkdir() */
- if (sd->s_type & CONFIGFS_USET_IN_MKDIR) {
+ if (sd->s_type & CONFIGFS_USET_ATTACHING) {
if (wait_mutex)
*wait_mutex = &sd->s_dentry->d_inode->i_mutex;
return -EAGAIN;
@@ -1127,7 +1127,7 @@ static int configfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
*/
spin_lock(&configfs_dirent_lock);
/* This will make configfs_detach_prep() fail */
- sd->s_type |= CONFIGFS_USET_IN_MKDIR;
+ sd->s_type |= CONFIGFS_USET_ATTACHING;
spin_unlock(&configfs_dirent_lock);
if (group)
@@ -1136,7 +1136,7 @@ static int configfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
ret = configfs_attach_item(parent_item, item, dentry);
spin_lock(&configfs_dirent_lock);
- sd->s_type &= ~CONFIGFS_USET_IN_MKDIR;
+ sd->s_type &= ~CONFIGFS_USET_ATTACHING;
spin_unlock(&configfs_dirent_lock);
out_unlink:
--
1.5.5.3
On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:
> For the parent's rmdir() case, we can use the same solution as with mkdir() vs
> rmdir(). For the target's rmdir() case, we cannot, since we do not and cannot
> lock the target's inode while in symlink(). Fortunately, once create_link()
> terminates, no further operation can fail in symlink(). So we first reorder the
> operations in create_link() to attach the new symlink to its target in last
> place, and second handle symlink creation failure the same way as a new item
> creation failure.
Oh, no, ugh. We don't want to create vfs objects first and ask
questions later. Otherwise we wouldn't need ATTACHING - we'd just
create the symlink, then check dropping.
If you have ATTACHING set, the rmdir cannot continue - you can
check dropping at that time. That is, you keep the DROPPING check where
it is - if it is already set, you know that rmdir() is going to complete
successfully. You can bail before even calling configfs_create_link().
If, however, it isn't set, your ATTACHING protects you from rmdir
throughout.
sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
if (sl) {
sl->sl_target = config_item_get(item);
spin_lock(&configfs_dirent_lock);
if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
spin_unlock(&configfs_dirent_lock);
config_item_put(item);
kfree(sl);
return -ENOENT;
/*
* Force rmdir() of parent_item to wait until we know
* if we succeed.
*/
parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
}
list_add(&sl->sl_list, &target_sd->s_links);
spin_unlock(&configfs_dirent_lock);
ret = configfs_create_link(sl, parent_item->ci_dentry,
dentry);
spin_lock(&configfs_dirent_lock);
parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
if (ret) {
list_del_init(&sl->sl_list);
spin_unlock(&configfs_dirent_lock);
config_item_put(item);
kfree(sl);
} else
spin_unlock(&configfs_dirent_lock);
}
return ret;
--
"When arrows don't penetrate, see...
Cupid grabs the pistol."
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Tue, Jun 17, 2008 at 07:37:21PM +0200, Louis Rilling wrote:
> This patch makes configfs_symlink() fail whenever the target is being removed,
> using the CONFIGFS_USET_DROPPING flag set by configfs_detach_prep() and
> protected by configfs_dirent_lock.
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index faeb441..58722a9 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -78,6 +78,12 @@ static int create_link(struct config_item *parent_item,
> if (sl) {
> sl->sl_target = config_item_get(item);
> spin_lock(&configfs_dirent_lock);
> + if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
> + spin_unlock(&configfs_dirent_lock);
> + config_item_put(item);
> + kfree(sl);
> + return -EPERM;
This needs to be -ENOENT. If rmdir set DROPPING and then
released dirent_lock, it's going to remove the target. That's ENOENT.
Joel
--
"Sometimes when reading Goethe I have the paralyzing suspicion
that he is trying to be funny."
- Guy Davenport
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Tue, Jun 17, 2008 at 03:15:28PM -0700, Joel Becker wrote:
> On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:
> > For the parent's rmdir() case, we can use the same solution as with mkdir() vs
> > rmdir(). For the target's rmdir() case, we cannot, since we do not and cannot
> > lock the target's inode while in symlink(). Fortunately, once create_link()
> > terminates, no further operation can fail in symlink(). So we first reorder the
> > operations in create_link() to attach the new symlink to its target in last
> > place, and second handle symlink creation failure the same way as a new item
> > creation failure.
>
> Oh, no, ugh. We don't want to create vfs objects first and ask
> questions later. Otherwise we wouldn't need ATTACHING - we'd just
> create the symlink, then check dropping.
> If you have ATTACHING set, the rmdir cannot continue - you can
> check dropping at that time. That is, you keep the DROPPING check where
> it is - if it is already set, you know that rmdir() is going to complete
> successfully. You can bail before even calling configfs_create_link().
> If, however, it isn't set, your ATTACHING protects you from rmdir
> throughout.
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.
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.
>
> sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
> if (sl) {
> sl->sl_target = config_item_get(item);
> spin_lock(&configfs_dirent_lock);
> if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
> spin_unlock(&configfs_dirent_lock);
> config_item_put(item);
> kfree(sl);
> return -ENOENT;
> /*
> * Force rmdir() of parent_item to wait until we know
> * if we succeed.
> */
> parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
> }
> list_add(&sl->sl_list, &target_sd->s_links);
> spin_unlock(&configfs_dirent_lock);
> ret = configfs_create_link(sl, parent_item->ci_dentry,
> dentry);
> 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.
> list_del_init(&sl->sl_list);
> spin_unlock(&configfs_dirent_lock);
> config_item_put(item);
> kfree(sl);
> } else
> spin_unlock(&configfs_dirent_lock);
> }
>
> return ret;
>
--
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
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?
> 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.
> > 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?
Joel
--
"Anything that is too stupid to be spoken is sung."
- Voltaire
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
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
On Thu, Jun 19, 2008 at 11:28:42AM +0200, Louis Rilling wrote:
> 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
Ok, true, we don't have a lock to protect mkdir.
> 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().
Yup, we'd have to have some other form of retry - note that this
is all spinlock territory. Thus, it should be fast. By the time
rmdir() gets back out to the toplevel, symlink/mkdir should be done
creating whatever they needed and waiting on the dirnet_lock. Then
rmdir waits again on the lock. It "should" be bang-bang.
Yes, I know, assumptions all around.
> > 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.
It won't be visible, because we hold i_mutex until we're done.
> Anyway, do you think that the "solutions" above are more acceptable?
The code for create then destroy was quite ugly. Maybe it
struck me because of that.
Joel
--
print STDOUT q
Just another Perl hacker,
unless $spring
- Larry Wall
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:
Ok, I can see some of why I hated this.
> +
> spin_lock(&configfs_dirent_lock);
> - if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
> - spin_unlock(&configfs_dirent_lock);
> - config_item_put(item);
> - kfree(sl);
> - return -EPERM;
> - }
> - list_add(&sl->sl_list, &target_sd->s_links);
> + /*
> + * Force rmdir() of parent_item to wait until we know if we
> + * succeed.
> + */
> + parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
> spin_unlock(&configfs_dirent_lock);
> +
> ret = configfs_create_link(sl, parent_item->ci_dentry,
> dentry);
> - if (ret) {
> - spin_lock(&configfs_dirent_lock);
> - list_del_init(&sl->sl_list);
> +
> + spin_lock(&configfs_dirent_lock);
> + parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
> +
> + if (ret || target_sd->s_type & CONFIGFS_USET_DROPPING) {
Use parenthesis. Actually, separate out the error cases.
> + struct configfs_dirent *sd = NULL;
> +
> + if (!ret) {
> + sd = dentry->d_fsdata;
> + list_del_init(&sd->s_sibling);
> + }
> spin_unlock(&configfs_dirent_lock);
> +
> + if (!ret) {
> + configfs_drop_dentry(sd, dentry->d_parent);
> + d_delete(dentry);
> + configfs_put(sd);
> + }
This open-code of the VFS munging is ripe to break when the VFS
changes or other configfs changes happen. The real issue is that you
are reimplementing the core of configfs_unlink(). Note how the core VFS
munging of configfs_rmdir() is separated out so that configfs_mkdir()
can also use it in the failure case? Do the same with unlink() and it
will read much better ("if (DROPPING) configfs_delete_link()"). Call it
configfs_remove_link() or configfs_delete_link().
Joel
--
Life's Little Instruction Book #337
"Reread your favorite book."
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
Joel,
What about this other solution? By the way, it does not even need to
rename CONFIGFS_USET_IN_MKDIR.
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
On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
> What about this other solution? By the way, it does not even need to
> rename CONFIGFS_USET_IN_MKDIR.
We don't need the check for USET_DROPPING either. I like a lot!
Joel
--
"I don't want to achieve immortality through my work; I want to
achieve immortality through not dying."
- Woody Allen
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
> What about this other solution? By the way, it does not even need to
> rename CONFIGFS_USET_IN_MKDIR.
I like it. My first thought was "why doesn't he take the mutex
during configfs_unlink()?", but the unlink is protected enough by
dirent_lock.
Joel
--
"Reality is merely an illusion, albeit a very persistent one."
- Albert Einstien
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Fri, Jun 20, 2008 at 03:44:20PM -0700, Joel Becker wrote:
> On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
> > What about this other solution? By the way, it does not even need to
> > rename CONFIGFS_USET_IN_MKDIR.
>
> We don't need the check for USET_DROPPING either. I like a lot!
I hope that you wasn't speaking about checking USET_DROPPING on the target. We
still need to check it, of course. I'm resending the updated, hopefully final,
patchset.
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