| Submitter | Eric W. Biederman |
|---|---|
| Date | 2009-11-03 11:57:08 |
| Message ID | <1257249429-12384-12-git-send-email-ebiederm@xmission.com> |
| Download | mbox | patch |
| Permalink | /patch/57287/ |
| State | New |
| Headers | show |
Comments
Quoting Eric W. Biederman (ebiederm@xmission.com): > From: Eric W. Biederman <ebiederm@xmission.com> > > By teaching sysfs_revalidate to hide a dentry for > a sysfs_dirent if the sysfs_dirent has been renamed, > and by teaching sysfs_lookup to return the original > dentry if the sysfs dirent has been renamed. I can > show the results of renames correctly without having to > update the dcache during the directory rename. > > This massively simplifies the rename logic allowing a lot > of weird sysfs special cases to be removed along with > a lot of now unnecesary helper code. > > Acked-by: Tejun Heo <tj@kernel.org> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com> Patch looks *great*, except: ... > @@ -315,6 +274,14 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd) > if (sd->s_flags & SYSFS_FLAG_REMOVED) > goto out_bad; > > + /* The sysfs dirent has been moved? */ > + if (dentry->d_parent->d_fsdata != sd->s_parent) > + goto out_bad; > + > + /* The sysfs dirent has been renamed */ > + if (strcmp(dentry->d_name.name, sd->s_name) != 0) > + goto out_bad; > + > mutex_unlock(&sysfs_mutex); > out_valid: > return 1; > @@ -322,6 +289,12 @@ out_bad: > /* Remove the dentry from the dcache hashes. > * If this is a deleted dentry we use d_drop instead of d_delete > * so sysfs doesn't need to cope with negative dentries. > + * > + * If this is a dentry that has simply been renamed we > + * use d_drop to remove it from the dcache lookup on its > + * old parent. If this dentry persists later when a lookup > + * is performed at its new name the dentry will be readded > + * to the dcache hashes. > */ > is_dir = (sysfs_type(sd) == SYSFS_DIR); > mutex_unlock(&sysfs_mutex); After this, if (is_dir) and (have_submounts(dentry)) then you'll still goto out_valid and return 1. Is that what you want? -serge -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
"Serge E. Hallyn" <serue@us.ibm.com> writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> From: Eric W. Biederman <ebiederm@xmission.com> >> >> By teaching sysfs_revalidate to hide a dentry for >> a sysfs_dirent if the sysfs_dirent has been renamed, >> and by teaching sysfs_lookup to return the original >> dentry if the sysfs dirent has been renamed. I can >> show the results of renames correctly without having to >> update the dcache during the directory rename. >> >> This massively simplifies the rename logic allowing a lot >> of weird sysfs special cases to be removed along with >> a lot of now unnecesary helper code. >> >> Acked-by: Tejun Heo <tj@kernel.org> >> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com> > > Patch looks *great*, except: > > ... > >> @@ -315,6 +274,14 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd) >> if (sd->s_flags & SYSFS_FLAG_REMOVED) >> goto out_bad; >> >> + /* The sysfs dirent has been moved? */ >> + if (dentry->d_parent->d_fsdata != sd->s_parent) >> + goto out_bad; >> + >> + /* The sysfs dirent has been renamed */ >> + if (strcmp(dentry->d_name.name, sd->s_name) != 0) >> + goto out_bad; >> + >> mutex_unlock(&sysfs_mutex); >> out_valid: >> return 1; >> @@ -322,6 +289,12 @@ out_bad: >> /* Remove the dentry from the dcache hashes. >> * If this is a deleted dentry we use d_drop instead of d_delete >> * so sysfs doesn't need to cope with negative dentries. >> + * >> + * If this is a dentry that has simply been renamed we >> + * use d_drop to remove it from the dcache lookup on its >> + * old parent. If this dentry persists later when a lookup >> + * is performed at its new name the dentry will be readded >> + * to the dcache hashes. >> */ >> is_dir = (sysfs_type(sd) == SYSFS_DIR); >> mutex_unlock(&sysfs_mutex); > > After this, if (is_dir) and (have_submounts(dentry)) then you'll > still goto out_valid and return 1. Is that what you want? It isn't what I want but it is what the VFS requires. If let the vfs continue on it's delusional state we will leak the vfs mount and everything mounted on top of it, with no way to remove the mounts. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hello, Eric W. Biederman wrote: > It isn't what I want but it is what the VFS requires. If let the vfs > continue on it's delusional state we will leak the vfs mount and > everything mounted on top of it, with no way to remove the mounts. This is caused by not having any way to prevent deletion on directories with submounts, right? How does other distributed filesystems deal with directories with submounts going away underneath it? Thanks.
Tejun Heo <tj@kernel.org> writes: > Hello, > > Eric W. Biederman wrote: >> It isn't what I want but it is what the VFS requires. If let the vfs >> continue on it's delusional state we will leak the vfs mount and >> everything mounted on top of it, with no way to remove the mounts. > > This is caused by not having any way to prevent deletion on > directories with submounts, right? How does other distributed > filesystems deal with directories with submounts going away underneath > it? NFS does exactly the same thing I am doing. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hello,
Eric W. Biederman wrote:
> NFS does exactly the same thing I am doing.
Oh... well, sysfs directories parenting other filesystems are pretty
rare and well defined. Although it's not very pretty, I don't think
we'll see any actual problem there. Thanks for the explanation.
Tejun Heo <tj@kernel.org> writes: > Hello, > > Eric W. Biederman wrote: >> NFS does exactly the same thing I am doing. > > Oh... well, sysfs directories parenting other filesystems are pretty > rare and well defined. Although it's not very pretty, I don't think > we'll see any actual problem there. Thanks for the explanation. To be perfectly clear, if we hit this ugly case. The internal sysfs_dirent tree is always what it should be. Only the VFS dentry cache doesn't get updated. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, 06 Nov 2009, ebiederm@xmission.com (Eric W. Biederman wrote: > Tejun Heo <tj@kernel.org> writes: > > > Hello, > > > > Eric W. Biederman wrote: > >> It isn't what I want but it is what the VFS requires. If let the vfs > >> continue on it's delusional state we will leak the vfs mount and > >> everything mounted on top of it, with no way to remove the mounts. "umount -l" on the whole thing will clear any submounts up too. > > > > This is caused by not having any way to prevent deletion on > > directories with submounts, right? How does other distributed > > filesystems deal with directories with submounts going away underneath > > it? > > NFS does exactly the same thing I am doing. Yes, this is a problem for NFS too. You cannot tell the NFS server "this directory is mounted on some client, don't let anything happen to it!". Basically the remaining choices are: a) let the old path leading up to the mount still be accessible, even though it doesn't exist anymore on the server (or has been replaced with something different) b) automatically dissolve any submounts if the path disappeard on the server I think Al was arguing in favor of b), while Linus said that mounts must never just disappear, so a) is better. I don't think an agreement was reached. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Miklos Szeredi <miklos@szeredi.hu> writes: > On Fri, 06 Nov 2009, ebiederm@xmission.com (Eric W. Biederman wrote: >> Tejun Heo <tj@kernel.org> writes: >> >> > Hello, >> > >> > Eric W. Biederman wrote: >> >> It isn't what I want but it is what the VFS requires. If let the vfs >> >> continue on it's delusional state we will leak the vfs mount and >> >> everything mounted on top of it, with no way to remove the mounts. > > "umount -l" on the whole thing will clear any submounts up too. > >> > >> > This is caused by not having any way to prevent deletion on >> > directories with submounts, right? How does other distributed >> > filesystems deal with directories with submounts going away underneath >> > it? >> >> NFS does exactly the same thing I am doing. > > Yes, this is a problem for NFS too. You cannot tell the NFS server > "this directory is mounted on some client, don't let anything happen > to it!". Basically the remaining choices are: > > a) let the old path leading up to the mount still be accessible, even > though it doesn't exist anymore on the server (or has been replaced > with something different) > > b) automatically dissolve any submounts if the path disappeard on the > server > > I think Al was arguing in favor of b), while Linus said that mounts > must never just disappear, so a) is better. I don't think an > agreement was reached. I haven't seen that conversation. I do know it is non-intutive and if you attempt to delete what is a mount point in another mount namespace and it won't go away. (What we do for non-distributed filesystems). So I would favor mount points dissolving if we had the infrastructure. Regardless the goal for now is to simply catch up with other distributed filesystems. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Quoting Tejun Heo (tj@kernel.org): > Hello, > > Eric W. Biederman wrote: > > It isn't what I want but it is what the VFS requires. If let the vfs > > continue on it's delusional state we will leak the vfs mount and > > everything mounted on top of it, with no way to remove the mounts. > > This is caused by not having any way to prevent deletion on > directories with submounts, right? How does other distributed > filesystems deal with directories with submounts going away underneath > it? Ooooh. I see, I was thinking only about the rename case, and forgot this was the path for deleted files, too. For the rename case it should be ok to let the dentry be put since the submounts will be accessible at the new location, right? Should that be handled separately? -serge -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Nov 9, 2009 at 6:14 AM, Serge E. Hallyn <serue@us.ibm.com> wrote: > Quoting Tejun Heo (tj@kernel.org): >> Hello, >> >> Eric W. Biederman wrote: >> > It isn't what I want but it is what the VFS requires. If let the vfs >> > continue on it's delusional state we will leak the vfs mount and >> > everything mounted on top of it, with no way to remove the mounts. >> >> This is caused by not having any way to prevent deletion on >> directories with submounts, right? How does other distributed >> filesystems deal with directories with submounts going away underneath >> it? > > Ooooh. I see, I was thinking only about the rename case, and forgot > this was the path for deleted files, too. For the rename case it > should be ok to let the dentry be put since the submounts will be > accessible at the new location, right? Should that be handled > separately? No in the rename case it isn't ok to let the dentry be discarded put as mounts are implemented using a hash of the struct dentry's address, and if you aren't the mount point you are referenced as d_parent. For rename I am slightly better than NFS. sysfs does not support hard links so if I am looking up the new name I can look for a preexisting dentry for my inode and if I find one I call d_move on it to lazily perform the rename. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Patch
diff --git a/fs/namei.c b/fs/namei.c index d11f404..d3c190c 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1279,28 +1279,6 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len) return __lookup_hash(&this, base, NULL); } -/** - * lookup_one_noperm - bad hack for sysfs - * @name: pathname component to lookup - * @base: base directory to lookup from - * - * This is a variant of lookup_one_len that doesn't perform any permission - * checks. It's a horrible hack to work around the braindead sysfs - * architecture and should not be used anywhere else. - * - * DON'T USE THIS FUNCTION EVER, thanks. - */ -struct dentry *lookup_one_noperm(const char *name, struct dentry *base) -{ - int err; - struct qstr this; - - err = __lookup_one_len(name, &this, base, strlen(name)); - if (err) - return ERR_PTR(err); - return __lookup_hash(&this, base, NULL); -} - int user_path_at(int dfd, const char __user *name, unsigned flags, struct path *path) { diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c index a05b027..0b60212 100644 --- a/fs/sysfs/dir.c +++ b/fs/sysfs/dir.c @@ -25,7 +25,6 @@ #include "sysfs.h" DEFINE_MUTEX(sysfs_mutex); -DEFINE_MUTEX(sysfs_rename_mutex); DEFINE_SPINLOCK(sysfs_assoc_lock); static DEFINE_SPINLOCK(sysfs_ino_lock); @@ -85,46 +84,6 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd) } /** - * sysfs_get_dentry - get dentry for the given sysfs_dirent - * @sd: sysfs_dirent of interest - * - * Get dentry for @sd. Dentry is looked up if currently not - * present. This function descends from the root looking up - * dentry for each step. - * - * LOCKING: - * mutex_lock(sysfs_rename_mutex) - * - * RETURNS: - * Pointer to found dentry on success, ERR_PTR() value on error. - */ -struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd) -{ - struct dentry *dentry = dget(sysfs_sb->s_root); - - while (dentry->d_fsdata != sd) { - struct sysfs_dirent *cur; - struct dentry *parent; - - /* find the first ancestor which hasn't been looked up */ - cur = sd; - while (cur->s_parent != dentry->d_fsdata) - cur = cur->s_parent; - - /* look it up */ - parent = dentry; - mutex_lock(&parent->d_inode->i_mutex); - dentry = lookup_one_noperm(cur->s_name, parent); - mutex_unlock(&parent->d_inode->i_mutex); - dput(parent); - - if (IS_ERR(dentry)) - break; - } - return dentry; -} - -/** * sysfs_get_active - get an active reference to sysfs_dirent * @sd: sysfs_dirent to get an active reference to * @@ -315,6 +274,14 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd) if (sd->s_flags & SYSFS_FLAG_REMOVED) goto out_bad; + /* The sysfs dirent has been moved? */ + if (dentry->d_parent->d_fsdata != sd->s_parent) + goto out_bad; + + /* The sysfs dirent has been renamed */ + if (strcmp(dentry->d_name.name, sd->s_name) != 0) + goto out_bad; + mutex_unlock(&sysfs_mutex); out_valid: return 1; @@ -322,6 +289,12 @@ out_bad: /* Remove the dentry from the dcache hashes. * If this is a deleted dentry we use d_drop instead of d_delete * so sysfs doesn't need to cope with negative dentries. + * + * If this is a dentry that has simply been renamed we + * use d_drop to remove it from the dcache lookup on its + * old parent. If this dentry persists later when a lookup + * is performed at its new name the dentry will be readded + * to the dcache hashes. */ is_dir = (sysfs_type(sd) == SYSFS_DIR); mutex_unlock(&sysfs_mutex); @@ -705,10 +678,15 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry, } /* instantiate and hash dentry */ - dentry->d_op = &sysfs_dentry_ops; - dentry->d_fsdata = sysfs_get(sd); - d_instantiate(dentry, inode); - d_rehash(dentry); + ret = d_find_alias(inode); + if (!ret) { + dentry->d_op = &sysfs_dentry_ops; + dentry->d_fsdata = sysfs_get(sd); + d_add(dentry, inode); + } else { + d_move(ret, dentry); + iput(inode); + } out_unlock: mutex_unlock(&sysfs_mutex); @@ -785,62 +763,32 @@ void sysfs_remove_dir(struct kobject * kobj) int sysfs_rename_dir(struct kobject * kobj, const char *new_name) { struct sysfs_dirent *sd = kobj->sd; - struct dentry *parent = NULL; - struct dentry *old_dentry = NULL, *new_dentry = NULL; const char *dup_name = NULL; int error; - mutex_lock(&sysfs_rename_mutex); + mutex_lock(&sysfs_mutex); error = 0; if (strcmp(sd->s_name, new_name) == 0) goto out; /* nothing to rename */ - /* get the original dentry */ - old_dentry = sysfs_get_dentry(sd); - if (IS_ERR(old_dentry)) { - error = PTR_ERR(old_dentry); - old_dentry = NULL; - goto out; - } - - parent = old_dentry->d_parent; - - /* lock parent and get dentry for new name */ - mutex_lock(&parent->d_inode->i_mutex); - mutex_lock(&sysfs_mutex); - error = -EEXIST; if (sysfs_find_dirent(sd->s_parent, new_name)) - goto out_unlock; - - error = -ENOMEM; - new_dentry = d_alloc_name(parent, new_name); - if (!new_dentry) - goto out_unlock; + goto out; /* rename sysfs_dirent */ error = -ENOMEM; new_name = dup_name = kstrdup(new_name, GFP_KERNEL); if (!new_name) - goto out_unlock; + goto out; dup_name = sd->s_name; sd->s_name = new_name; - /* rename */ - d_add(new_dentry, NULL); - d_move(old_dentry, new_dentry); - error = 0; - out_unlock: + out: mutex_unlock(&sysfs_mutex); - mutex_unlock(&parent->d_inode->i_mutex); kfree(dup_name); - dput(old_dentry); - dput(new_dentry); - out: - mutex_unlock(&sysfs_rename_mutex); return error; } @@ -848,12 +796,11 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj) { struct sysfs_dirent *sd = kobj->sd; struct sysfs_dirent *new_parent_sd; - struct dentry *old_parent, *new_parent = NULL; - struct dentry *old_dentry = NULL, *new_dentry = NULL; int error; - mutex_lock(&sysfs_rename_mutex); BUG_ON(!sd->s_parent); + + mutex_lock(&sysfs_mutex); new_parent_sd = (new_parent_kobj && new_parent_kobj->sd) ? new_parent_kobj->sd : &sysfs_root; @@ -861,61 +808,20 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj) if (sd->s_parent == new_parent_sd) goto out; /* nothing to move */ - /* get dentries */ - old_dentry = sysfs_get_dentry(sd); - if (IS_ERR(old_dentry)) { - error = PTR_ERR(old_dentry); - old_dentry = NULL; - goto out; - } - old_parent = old_dentry->d_parent; - - new_parent = sysfs_get_dentry(new_parent_sd); - if (IS_ERR(new_parent)) { - error = PTR_ERR(new_parent); - new_parent = NULL; - goto out; - } - -again: - mutex_lock(&old_parent->d_inode->i_mutex); - if (!mutex_trylock(&new_parent->d_inode->i_mutex)) { - mutex_unlock(&old_parent->d_inode->i_mutex); - goto again; - } - mutex_lock(&sysfs_mutex); - error = -EEXIST; if (sysfs_find_dirent(new_parent_sd, sd->s_name)) - goto out_unlock; - - error = -ENOMEM; - new_dentry = d_alloc_name(new_parent, sd->s_name); - if (!new_dentry) - goto out_unlock; - - error = 0; - d_add(new_dentry, NULL); - d_move(old_dentry, new_dentry); + goto out; /* Remove from old parent's list and insert into new parent's list. */ sysfs_unlink_sibling(sd); sysfs_get(new_parent_sd); - drop_nlink(old_parent->d_inode); sysfs_put(sd->s_parent); sd->s_parent = new_parent_sd; - inc_nlink(new_parent->d_inode); sysfs_link_sibling(sd); - out_unlock: + error = 0; +out: mutex_unlock(&sysfs_mutex); - mutex_unlock(&new_parent->d_inode->i_mutex); - mutex_unlock(&old_parent->d_inode->i_mutex); - out: - dput(new_parent); - dput(old_dentry); - dput(new_dentry); - mutex_unlock(&sysfs_rename_mutex); return error; } diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c index 2dcafe8..082a3eb 100644 --- a/fs/sysfs/inode.c +++ b/fs/sysfs/inode.c @@ -205,17 +205,6 @@ static inline void set_inode_attr(struct inode * inode, struct iattr * iattr) inode->i_ctime = iattr->ia_ctime; } - -/* - * sysfs has a different i_mutex lock order behavior for i_mutex than other - * filesystems; sysfs i_mutex is called in many places with subsystem locks - * held. At the same time, many of the VFS locking rules do not apply to - * sysfs at all (cross directory rename for example). To untangle this mess - * (which gives false positives in lockdep), we're giving sysfs inodes their - * own class for i_mutex. - */ -static struct lock_class_key sysfs_inode_imutex_key; - static int sysfs_count_nlink(struct sysfs_dirent *sd) { struct sysfs_dirent *child; @@ -268,7 +257,6 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode) inode->i_mapping->a_ops = &sysfs_aops; inode->i_mapping->backing_dev_info = &sysfs_backing_dev_info; inode->i_op = &sysfs_inode_operations; - lockdep_set_class(&inode->i_mutex, &sysfs_inode_imutex_key); set_default_inode_attr(inode, sd->s_mode); sysfs_refresh_inode(sd, inode); diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h index 90b3501..98a15bf 100644 --- a/fs/sysfs/sysfs.h +++ b/fs/sysfs/sysfs.h @@ -103,7 +103,6 @@ extern struct kmem_cache *sysfs_dir_cachep; * dir.c */ extern struct mutex sysfs_mutex; -extern struct mutex sysfs_rename_mutex; extern spinlock_t sysfs_assoc_lock; extern const struct file_operations sysfs_dir_operations; diff --git a/include/linux/namei.h b/include/linux/namei.h index ec0f607..0289467 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -76,7 +76,6 @@ extern struct file *nameidata_to_filp(struct nameidata *nd, int flags); extern void release_open_intent(struct nameidata *); extern struct dentry *lookup_one_len(const char *, struct dentry *, int); -extern struct dentry *lookup_one_noperm(const char *, struct dentry *); extern int follow_down(struct path *); extern int follow_up(struct path *);