[PATCH 0/4] fix ceph_get_caps() bugs

[PATCH 0/4] fix ceph_get_caps() bugs

[Yan, Zheng]

Yan, Zheng (4):
  ceph: cleanup return error of try_get_cap_refs()
  ceph: request new max size only when there is auth cap
  ceph: don't skip updating wanted caps when cap is stale
  ceph: wait for async creating inode before requesting new max size

 fs/ceph/caps.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

-- 
2.21.1

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

[Yan, Zheng]

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;
 		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);
-- 
2.21.1

[PATCH 2/4] ceph: request new max size only when there is auth cap

[Yan, Zheng]

When there is no auth cap, check_max_size() can't do anything, may
cause infinite loop inside ceph_get_caps().

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/caps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 804f4c65251a..295b61201d85 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -2582,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 = -EFBIG;
+				ret = ci->i_auth_cap ? -EFBIG : -ESTALE;
 			goto out_unlock;
 		}
 		/*
-- 
2.21.1

[PATCH 3/4] ceph: don't skip updating wanted caps when cap is stale

[Yan, Zheng]

1. try_get_cap_refs() fails to get caps and finds that mds_wanted
   does not include what it wants. It return -ESTALE.
2. ceph_get_caps() calls ceph_renew_caps(). ceph_renew_caps() finds
   that inode has cap, so it calls ceph_check_caps().
3. ceph_check_caps() finds that issued caps (without checking if it's
   stale) already includes caps wanted by open file, so it skips
   updating wanted caps.

Above events can cause infinite loop inside ceph_get_caps()

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/caps.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 295b61201d85..f63db3cb9c4f 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -2008,8 +2008,12 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags,
 		}
 
 		/* want more caps from mds? */
-		if (want & ~(cap->mds_wanted | cap->issued))
-			goto ack;
+		if (want & ~cap->mds_wanted) {
+			if (want & ~(cap->mds_wanted | cap->issued))
+				goto ack;
+			if (!__cap_is_valid(cap))
+				goto ack;
+		}
 
 		/* things we might delay */
 		if ((cap->issued & ~retain) == 0)
-- 
2.21.1

[PATCH 4/4] ceph: wait for async creating inode before requesting new max size

[Yan, Zheng]

ceph_check_caps() can't request new max size for async creating inode.
This may make ceph_get_caps() loop busily until getting reply of the
async create.

This patch also waits for async creating inode before calling
ceph_renew_caps()

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/caps.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index f63db3cb9c4f..14bedf2c16cd 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -2832,6 +2832,11 @@ int ceph_get_caps(struct file *filp, int need, int want,
 		}
 
 		if (ret < 0) {
+			if (ret == -EFBIG || ret == -ESTALE) {
+				int ret2 = ceph_wait_on_async_create(inode);
+				if (ret2 < 0)
+					return ret2;
+			}
 			if (ret == -EFBIG) {
 				check_max_size(inode, endoff);
 				continue;
-- 
2.21.1

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

[Jeff Layton]

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>

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

[Yan, Zheng]

On Mon, Mar 16, 2020 at 8:50 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> 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?
>

make sense. Please edit the patch if you don't have other comments.

Regards
Yan, Zheng

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