| Message ID | 20210712123649.1102392-2-dkadashev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | namei: clean up retry logic in various do_* functions | expand |
On Mon, Jul 12, 2021 at 07:36:43PM +0700, Dmitry Kadashev wrote: > Moving the main logic to a helper function makes the whole thing much > easier to follow. > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Christian Brauner <christian.brauner@ubuntu.com> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Link: https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/ > Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com> > --- > fs/namei.c | 44 +++++++++++++++++++++++++------------------- > 1 file changed, 25 insertions(+), 19 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index b5adfd4f7de6..ae6cde7dc91e 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3947,7 +3947,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir, > } > EXPORT_SYMBOL(vfs_rmdir); > > -int do_rmdir(int dfd, struct filename *name) > +static int rmdir_helper(int dfd, struct filename *name, > + unsigned int lookup_flags) > { > struct user_namespace *mnt_userns; > int error; > @@ -3955,54 +3956,59 @@ int do_rmdir(int dfd, struct filename *name) > struct path path; > struct qstr last; > int type; > - unsigned int lookup_flags = 0; > -retry: > + > error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type); > if (error) > - goto exit1; > + return error; > > switch (type) { > case LAST_DOTDOT: > error = -ENOTEMPTY; > - goto exit2; > + goto exit1; > case LAST_DOT: > error = -EINVAL; > - goto exit2; > + goto exit1; > case LAST_ROOT: > error = -EBUSY; > - goto exit2; > + goto exit1; > } > > error = mnt_want_write(path.mnt); > if (error) > - goto exit2; > + goto exit1; > > inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT); > dentry = __lookup_hash(&last, path.dentry, lookup_flags); > error = PTR_ERR(dentry); > if (IS_ERR(dentry)) > - goto exit3; > + goto exit2; > if (!dentry->d_inode) { > error = -ENOENT; > - goto exit4; > + goto exit3; > } > error = security_path_rmdir(&path, dentry); > if (error) > - goto exit4; > + goto exit3; > mnt_userns = mnt_user_ns(path.mnt); > error = vfs_rmdir(mnt_userns, path.dentry->d_inode, dentry); > -exit4: > - dput(dentry); > exit3: > + dput(dentry); > +exit2: > inode_unlock(path.dentry->d_inode); > mnt_drop_write(path.mnt); > -exit2: > - path_put(&path); > - if (retry_estale(error, lookup_flags)) { > - lookup_flags |= LOOKUP_REVAL; > - goto retry; > - } > exit1: > + path_put(&path); > + return error; > +} > + > +int do_rmdir(int dfd, struct filename *name) > +{ > + int error; > + > + error = rmdir_helper(dfd, name, 0); > + if (retry_estale(error, 0)) > + error = rmdir_helper(dfd, name, LOOKUP_REVAL); Instead of naming all these $something_helper I would follow the underscore naming pattern we usually do, i.e. instead of e.g. rmdir_helper do __rmdir() or __do_rmdir().
On Tue, Jul 13, 2021 at 7:53 AM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > Instead of naming all these $something_helper I would follow the > underscore naming pattern we usually do, i.e. instead of e.g. > rmdir_helper do __rmdir() or __do_rmdir(). That's certainly a pattern we have, but I don't necessarily love it. It would be even better if we'd have names that actually explain what/why the abstraction exists. In this case, it's the "possibly retry due to ESTALE", but I have no idea how to sanely name that. Making it "try_rmdir()" or something like that is the best I can come up with right now. On a similar note, the existing "do_rmdir()" and friends aren't wonderful names either, but we expose that name out so changing it is probably not worth it. But right now we have "vfs_rmdir()" and "do_rmdir()", and they are just different levels of the "rmdir stack", without the name really describing where in the stack they are. Naming is hard, and I don't think the double underscores have been wonderful either. Linus
On Tue, Jul 13, 2021 at 11:58 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, Jul 13, 2021 at 7:53 AM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > > > Instead of naming all these $something_helper I would follow the > > underscore naming pattern we usually do, i.e. instead of e.g. > > rmdir_helper do __rmdir() or __do_rmdir(). > > That's certainly a pattern we have, but I don't necessarily love it. > > It would be even better if we'd have names that actually explain > what/why the abstraction exists. In this case, it's the "possibly > retry due to ESTALE", but I have no idea how to sanely name that. > Making it "try_rmdir()" or something like that is the best I can come > up with right now. > > On a similar note, the existing "do_rmdir()" and friends aren't > wonderful names either, but we expose that name out so changing it is > probably not worth it. But right now we have "vfs_rmdir()" and > "do_rmdir()", and they are just different levels of the "rmdir stack", > without the name really describing where in the stack they are. > > Naming is hard, and I don't think the double underscores have been > wonderful either. Naming *is* hard, I do not have any good ideas here, so I just went with try_rmdir(). Christian, Linus, let me know if that is not good enough.
diff --git a/fs/namei.c b/fs/namei.c index b5adfd4f7de6..ae6cde7dc91e 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3947,7 +3947,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir, } EXPORT_SYMBOL(vfs_rmdir); -int do_rmdir(int dfd, struct filename *name) +static int rmdir_helper(int dfd, struct filename *name, + unsigned int lookup_flags) { struct user_namespace *mnt_userns; int error; @@ -3955,54 +3956,59 @@ int do_rmdir(int dfd, struct filename *name) struct path path; struct qstr last; int type; - unsigned int lookup_flags = 0; -retry: + error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type); if (error) - goto exit1; + return error; switch (type) { case LAST_DOTDOT: error = -ENOTEMPTY; - goto exit2; + goto exit1; case LAST_DOT: error = -EINVAL; - goto exit2; + goto exit1; case LAST_ROOT: error = -EBUSY; - goto exit2; + goto exit1; } error = mnt_want_write(path.mnt); if (error) - goto exit2; + goto exit1; inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT); dentry = __lookup_hash(&last, path.dentry, lookup_flags); error = PTR_ERR(dentry); if (IS_ERR(dentry)) - goto exit3; + goto exit2; if (!dentry->d_inode) { error = -ENOENT; - goto exit4; + goto exit3; } error = security_path_rmdir(&path, dentry); if (error) - goto exit4; + goto exit3; mnt_userns = mnt_user_ns(path.mnt); error = vfs_rmdir(mnt_userns, path.dentry->d_inode, dentry); -exit4: - dput(dentry); exit3: + dput(dentry); +exit2: inode_unlock(path.dentry->d_inode); mnt_drop_write(path.mnt); -exit2: - path_put(&path); - if (retry_estale(error, lookup_flags)) { - lookup_flags |= LOOKUP_REVAL; - goto retry; - } exit1: + path_put(&path); + return error; +} + +int do_rmdir(int dfd, struct filename *name) +{ + int error; + + error = rmdir_helper(dfd, name, 0); + if (retry_estale(error, 0)) + error = rmdir_helper(dfd, name, LOOKUP_REVAL); + putname(name); return error; }
Moving the main logic to a helper function makes the whole thing much easier to follow. Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <christian.brauner@ubuntu.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Link: https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/ Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com> --- fs/namei.c | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-)