On Mon, Aug 28, 2017 at 01:54:12PM +1000, NeilBrown wrote:

quoted

On Fri, Aug 25 2017, J. Bruce Fields wrote:

quoted

From: "J. Bruce Fields" <redacted>

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 <redacted>
---
 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)