On Tue, 2017-08-29 at 17:37 -0400, J. Bruce Fields wrote:
> On Mon, Aug 28, 2017 at 01:54:12PM +1000, NeilBrown wrote:
> > On Fri, Aug 25 2017, J. Bruce Fields wrote:
> >
> > > From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
> > >
> > > Pull the checks for delegated_inode into break_deleg_wait() to
> > > simplify
> > > the callers a little.
> > >
> > > No change in behavior.
> > >
> > > Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> > > ---
> > > fs/namei.c | 26 ++++++++++----------------
> > > fs/open.c | 16 ++++++----------
> > > fs/utimes.c | 8 +++-----
> > > include/linux/fs.h | 12 +++++++-----
> > > 4 files changed, 26 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index ddb6a7c2b3d4..5a93be7b2c9c 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -4048,11 +4048,9 @@ static long do_unlinkat(int dfd, const
> > > char __user *pathname)
> > > if (inode)
> > > iput(inode); /* truncate the inode here
> > > */
> > > inode = NULL;
> > > - if (delegated_inode) {
> > > - error = break_deleg_wait(&delegated_inode);
> > > - if (!error)
> > > - goto retry_deleg;
> > > - }
> > > + error = break_deleg_wait(&delegated_inode, error);
> > > + if (error == DELEG_RETRY)
> > > + goto retry_deleg;
> >
> > <mode=bikeshed>
> >
> > I don't like the "DELEG_RETRY". You are comparing it against an
> > 'error', but it doesn't start with '-E', so I get confused (happens
> > often).
> >
> > If this read:
> >
> > if (error > 0)
> > goto retry_deleg;
> >
> > it would be must more obvious to me what was happening. Clearly
> > the
> > return value isn't an error, and it isn't "success" either. This
> > is a
> > pattern I've seen elsewhere.
> >
> > Alternately you could use "-EAGAIN", but I suspect there is a risk
> > of
> > unwanted side-effects if you re-use and existing code.
>
> Yes. OK, I think I like your suggestion. The change would look like
> the following (untested).
>
> --b.
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 5a93be7b2c9c..e8688498aff7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4049,7 +4049,7 @@ static long do_unlinkat(int dfd, const char
> __user *pathname)
> iput(inode); /* truncate the inode here */
> inode = NULL;
> error = break_deleg_wait(&delegated_inode, error);
> - if (error == DELEG_RETRY)
> + if (error > 0)
> goto retry_deleg;
> mnt_drop_write(path.mnt);
> exit1:
> @@ -4282,7 +4282,7 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char
> __user *, oldname,
> out_dput:
> done_path_create(&new_path, new_dentry);
> error = break_deleg_wait(&delegated_inode, error);
> - if (error == DELEG_RETRY) {
> + if (error > 0) {
> path_put(&old_path);
> goto retry;
> }
> @@ -4598,7 +4598,7 @@ SYSCALL_DEFINE5(renameat2, int, olddfd, const
> char __user *, oldname,
> exit3:
> unlock_rename(new_path.dentry, old_path.dentry);
> error = break_deleg_wait(&delegated_inode, error);
> - if (error == DELEG_RETRY)
> + if (error > 0)
> goto retry_deleg;
> mnt_drop_write(old_path.mnt);
> exit2:
> diff --git a/fs/open.c b/fs/open.c
> index d49e9385e45d..80975c4dd146 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -533,7 +533,7 @@ static int chmod_common(const struct path *path,
> umode_t mode)
> out_unlock:
> inode_unlock(inode);
> error = break_deleg_wait(&delegated_inode, error);
> - if (error == DELEG_RETRY)
> + if (error > 0)
> goto retry_deleg;
> mnt_drop_write(path->mnt);
> return error;
> @@ -610,7 +610,7 @@ static int chown_common(const struct path *path,
> uid_t user, gid_t group)
> error = notify_change(path->dentry, &newattrs,
> &delegated_inode);
> inode_unlock(inode);
> error = break_deleg_wait(&delegated_inode, error);
> - if (error == DELEG_RETRY)
> + if (error > 0)
> goto retry_deleg;
> return error;
> }
> diff --git a/fs/utimes.c b/fs/utimes.c
> index 75467b7ebfce..4dc6717638e6 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -90,7 +90,7 @@ static int utimes_common(const struct path *path,
> struct timespec *times)
> error = notify_change(path->dentry, &newattrs,
> &delegated_inode);
> inode_unlock(inode);
> error = break_deleg_wait(&delegated_inode, error);
> - if (error == DELEG_RETRY)
> + if (error > 0)
> goto retry_deleg;
>
> mnt_drop_write(path->mnt);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1d0d2fde1766..1c7f7be3f26d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2288,8 +2288,6 @@ static inline int try_break_deleg(struct inode
> *inode, struct inode **delegated_
> return ret;
> }
>
> -#define DELEG_RETRY 1
> -
> static inline int break_deleg_wait(struct inode **delegated_inode,
> int error)
> {
> if (!*delegated_inode)
> @@ -2297,7 +2295,13 @@ static inline int break_deleg_wait(struct
> inode **delegated_inode, int error)
> error = break_deleg(*delegated_inode, O_WRONLY);
> iput(*delegated_inode);
> *delegated_inode = NULL;
> - return error ? error : DELEG_RETRY;
> + if (error)
> + return error;
> + /*
> + * Signal to the caller that it can retry the original
> operation
> + * now that the delegation is broken:
> + */
> + return 1;
> }
>
> static inline int break_layout(struct inode *inode, bool wait)
ACK, I like that better too. I think a kerneldoc header is probably
warranted here too, since this is a bit of an odd return situation.
--
Jeff Layton <jlayton@xxxxxxxxxx>
