Re: [PATCH 1/3] fs: cleanup to hide some details of delegation logic
From: NeilBrown <hidden>
Date: 2017-08-28 03:54:23
Also in:
linux-fsdevel
On Fri, Aug 25 2017, J. Bruce Fields wrote:
quoted hunk ↗ jump to hunk
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.
Thanks,
NeilBrown Attachments
- signature.asc [application/pgp-signature] 832 bytes