Re: [PATCH 1/4] ceph: cleanup return error of try_get_cap_refs()

From: Jeff Layton <jlayton@kernel.org>
To: "Yan, Zheng" <zyan@redhat.com>, ceph-devel@vger.kernel.org
Subject: Re: [PATCH 1/4] ceph: cleanup return error of try_get_cap_refs()
Date: Mon, 16 Mar 2020 08:48:23 -0400	[thread overview]
Message-ID: <25eaece7ad299eef0e7418f2b9acce900460baed.camel@kernel.org> (raw)
In-Reply-To: <20200310113421.174873-2-zyan@redhat.com>

On Tue, 2020-03-10 at 19:34 +0800, Yan, Zheng wrote:
> Returns 0 if caps were not able to be acquired (yet), 1 if succeed,
> or a negative error code. There are 3 speical error codes:
> 
> -EAGAIN: need to sleep but non-blocking is specified
> -EFBIG:  ask caller to call check_max_size() and try again.
> -ESTALE: ask caller to call ceph_renew_caps() and try again.
> 
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> ---
>  fs/ceph/caps.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 342a32c74c64..804f4c65251a 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -2530,10 +2530,11 @@ void ceph_take_cap_refs(struct ceph_inode_info *ci, int got,
>   * Note that caller is responsible for ensuring max_size increases are
>   * requested from the MDS.
>   *
> - * Returns 0 if caps were not able to be acquired (yet), a 1 if they were,
> - * or a negative error code.
> - *
> - * FIXME: how does a 0 return differ from -EAGAIN?
> + * Returns 0 if caps were not able to be acquired (yet), 1 if succeed,
> + * or a negative error code. There are 3 speical error codes:
> + *  -EAGAIN: need to sleep but non-blocking is specified
> + *  -EFBIG:  ask caller to call check_max_size() and try again.
> + *  -ESTALE: ask caller to call ceph_renew_caps() and try again.
>   */
>  enum {
>  	/* first 8 bits are reserved for CEPH_FILE_MODE_FOO */
> @@ -2581,7 +2582,7 @@ static int try_get_cap_refs(struct inode *inode, int need, int want,
>  			dout("get_cap_refs %p endoff %llu > maxsize %llu\n",
>  			     inode, endoff, ci->i_max_size);
>  			if (endoff > ci->i_requested_max_size)
> -				ret = -EAGAIN;
> +				ret = -EFBIG;
>  			goto out_unlock;
>  		}
>  		/*
> @@ -2743,7 +2744,10 @@ int ceph_try_get_caps(struct inode *inode, int need, int want,
>  		flags |= NON_BLOCKING;
>  
>  	ret = try_get_cap_refs(inode, need, want, 0, flags, got);
> -	return ret == -EAGAIN ? 0 : ret;
> +	/* three special error codes */
> +	if (ret == -EAGAIN || ret == -EFBIG || ret == -EAGAIN)
> +		ret = 0;
> +	return ret;
>  }
>  
>  /*
> @@ -2771,17 +2775,12 @@ int ceph_get_caps(struct file *filp, int need, int want,
>  	flags = get_used_fmode(need | want);
>  
>  	while (true) {
> -		if (endoff > 0)
> -			check_max_size(inode, endoff);
> -
>  		flags &= CEPH_FILE_MODE_MASK;
>  		if (atomic_read(&fi->num_locks))
>  			flags |= CHECK_FILELOCK;
>  		_got = 0;
>  		ret = try_get_cap_refs(inode, need, want, endoff,
>  				       flags, &_got);
> -		if (ret == -EAGAIN)
> -			continue;

Ok, so I guess we don't expect to see this error here since we didn't
set NON_BLOCKING. The error returns from try_get_cap_refs are pretty
complex, and I worry a little about future changes subtly breaking some
of these assumptions.

Maybe a WARN_ON_ONCE(ret == -EAGAIN) here would be good? 

>  		if (!ret) {
>  			struct ceph_mds_client *mdsc = fsc->mdsc;
>  			struct cap_wait cw;
> @@ -2829,6 +2828,10 @@ int ceph_get_caps(struct file *filp, int need, int want,
>  		}
>  
>  		if (ret < 0) {
> +			if (ret == -EFBIG) {
> +				check_max_size(inode, endoff);
> +				continue;
> +			}
>  			if (ret == -ESTALE) {
>  				/* session was killed, try renew caps */
>  				ret = ceph_renew_caps(inode, flags);

-- 
Jeff Layton <jlayton@kernel.org>