Re: [PATCH 1/3] fs: cleanup to hide some details of delegation logic

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

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

> From: "J. Bruce Fields" <bfields@redhat.com>
>
> 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@redhat.com>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]