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)
