[PATCH] ceph: wake waiters on any IMPORT that grants new caps

[PATCH] ceph: wake waiters on any IMPORT that grants new caps

[Jeff Layton]

I've noticed an intermittent hang waiting for caps in some testing. What
I see is that the client will try to get caps for an operation (e.g. a
read), and ends up waiting on the waitqueue forever. The caps debugfs
file however shows that the caps it's waiting on have already been
granted.

The current grant handling code will wake the waitqueue when it sees
that there are newly-granted caps in the issued set. On an import
however, we'll end up adding a new cap first, which fools the logic into
thinking that nothing has changed. A later hack in the code works around
this, but only for auth caps.

Ensure we wake the waiters whenever we get an IMPORT that grants new
caps for the inode.

URL: https://tracker.ceph.com/issues/54044
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/caps.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

I'm still testing this patch, but I think this may be the cause of some
mysterious hangs I've hit in testing.

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index e668cdb9c99e..06b65a68e920 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -3541,21 +3541,22 @@ static void handle_cap_grant(struct inode *inode,
 			fill_inline = true;
 	}
 
-	if (ci->i_auth_cap == cap &&
-	    le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
+	if (le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
 		if (newcaps & ~extra_info->issued)
 			wake = true;
 
-		if (ci->i_requested_max_size > max_size ||
-		    !(le32_to_cpu(grant->wanted) & CEPH_CAP_ANY_FILE_WR)) {
-			/* re-request max_size if necessary */
-			ci->i_requested_max_size = 0;
-			wake = true;
-		}
+		if (ci->i_auth_cap == cap) {
+			if (ci->i_requested_max_size > max_size ||
+			    !(le32_to_cpu(grant->wanted) & CEPH_CAP_ANY_FILE_WR)) {
+				/* re-request max_size if necessary */
+				ci->i_requested_max_size = 0;
+				wake = true;
+			}
 
-		ceph_kick_flushing_inode_caps(session, ci);
-		spin_unlock(&ci->i_ceph_lock);
-		up_read(&session->s_mdsc->snap_rwsem);
+			ceph_kick_flushing_inode_caps(session, ci);
+			spin_unlock(&ci->i_ceph_lock);
+			up_read(&session->s_mdsc->snap_rwsem);
+		}
 	} else {
 		spin_unlock(&ci->i_ceph_lock);
 	}
-- 
2.34.1

Re: [PATCH] ceph: wake waiters on any IMPORT that grants new caps

[Yan, Zheng]

On Sat, Jan 29, 2022 at 2:32 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> I've noticed an intermittent hang waiting for caps in some testing. What
> I see is that the client will try to get caps for an operation (e.g. a
> read), and ends up waiting on the waitqueue forever. The caps debugfs
> file however shows that the caps it's waiting on have already been
> granted.
>
> The current grant handling code will wake the waitqueue when it sees
> that there are newly-granted caps in the issued set. On an import
> however, we'll end up adding a new cap first, which fools the logic into
> thinking that nothing has changed. A later hack in the code works around
> this, but only for auth caps.

not right. handle_cap_import() saves old issued to extra_info->issued.

>
> Ensure we wake the waiters whenever we get an IMPORT that grants new
> caps for the inode.
>
> URL: https://tracker.ceph.com/issues/54044
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/caps.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> I'm still testing this patch, but I think this may be the cause of some
> mysterious hangs I've hit in testing.
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index e668cdb9c99e..06b65a68e920 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3541,21 +3541,22 @@ static void handle_cap_grant(struct inode *inode,
>                         fill_inline = true;
>         }
>
> -       if (ci->i_auth_cap == cap &&
> -           le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
> +       if (le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
>                 if (newcaps & ~extra_info->issued)
>                         wake = true;
>
> -               if (ci->i_requested_max_size > max_size ||
> -                   !(le32_to_cpu(grant->wanted) & CEPH_CAP_ANY_FILE_WR)) {
> -                       /* re-request max_size if necessary */
> -                       ci->i_requested_max_size = 0;
> -                       wake = true;
> -               }
> +               if (ci->i_auth_cap == cap) {
> +                       if (ci->i_requested_max_size > max_size ||
> +                           !(le32_to_cpu(grant->wanted) & CEPH_CAP_ANY_FILE_WR)) {
> +                               /* re-request max_size if necessary */
> +                               ci->i_requested_max_size = 0;
> +                               wake = true;
> +                       }
>
> -               ceph_kick_flushing_inode_caps(session, ci);
> -               spin_unlock(&ci->i_ceph_lock);
> -               up_read(&session->s_mdsc->snap_rwsem);
> +                       ceph_kick_flushing_inode_caps(session, ci);
> +                       spin_unlock(&ci->i_ceph_lock);
> +                       up_read(&session->s_mdsc->snap_rwsem);
> +               }
>         } else {
>                 spin_unlock(&ci->i_ceph_lock);
>         }
> --
> 2.34.1
>

Re: [PATCH] ceph: wake waiters on any IMPORT that grants new caps

[Jeff Layton]

On Sat, 2022-01-29 at 11:14 +0800, Yan, Zheng wrote:
> On Sat, Jan 29, 2022 at 2:32 AM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > I've noticed an intermittent hang waiting for caps in some testing. What
> > I see is that the client will try to get caps for an operation (e.g. a
> > read), and ends up waiting on the waitqueue forever. The caps debugfs
> > file however shows that the caps it's waiting on have already been
> > granted.
> > 
> > The current grant handling code will wake the waitqueue when it sees
> > that there are newly-granted caps in the issued set. On an import
> > however, we'll end up adding a new cap first, which fools the logic into
> > thinking that nothing has changed. A later hack in the code works around
> > this, but only for auth caps.
> 
> not right. handle_cap_import() saves old issued to extra_info->issued.
> 

It does save the old issued value to extra_info->issued, but handle_cap
grant consults cap->issued for most of its logic. It does check against
extra_info->issued for auth caps to determine whether to wake waiters,
but doesn't do that for non-auth caps. This patch corrects that.

If you think this patch isn't right, can you elaborate on why and what
would make it correct?

Thanks,
Jeff

> > 
> > Ensure we wake the waiters whenever we get an IMPORT that grants new
> > caps for the inode.
> > 
> > URL: https://tracker.ceph.com/issues/54044
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/caps.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > I'm still testing this patch, but I think this may be the cause of some
> > mysterious hangs I've hit in testing.
> > 
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index e668cdb9c99e..06b65a68e920 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -3541,21 +3541,22 @@ static void handle_cap_grant(struct inode *inode,
> >                         fill_inline = true;
> >         }
> > 
> > -       if (ci->i_auth_cap == cap &&
> > -           le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
> > +       if (le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
> >                 if (newcaps & ~extra_info->issued)
> >                         wake = true;
> > 
> > -               if (ci->i_requested_max_size > max_size ||
> > -                   !(le32_to_cpu(grant->wanted) & CEPH_CAP_ANY_FILE_WR)) {
> > -                       /* re-request max_size if necessary */
> > -                       ci->i_requested_max_size = 0;
> > -                       wake = true;
> > -               }
> > +               if (ci->i_auth_cap == cap) {
> > +                       if (ci->i_requested_max_size > max_size ||
> > +                           !(le32_to_cpu(grant->wanted) & CEPH_CAP_ANY_FILE_WR)) {
> > +                               /* re-request max_size if necessary */
> > +                               ci->i_requested_max_size = 0;
> > +                               wake = true;
> > +                       }
> > 
> > -               ceph_kick_flushing_inode_caps(session, ci);
> > -               spin_unlock(&ci->i_ceph_lock);
> > -               up_read(&session->s_mdsc->snap_rwsem);
> > +                       ceph_kick_flushing_inode_caps(session, ci);
> > +                       spin_unlock(&ci->i_ceph_lock);
> > +                       up_read(&session->s_mdsc->snap_rwsem);
> > +               }
> >         } else {
> >                 spin_unlock(&ci->i_ceph_lock);
> >         }
> > --
> > 2.34.1
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: wake waiters on any IMPORT that grants new caps

[Jeff Layton]

On Wed, 2022-02-02 at 03:36 +0800, Yan, Zheng wrote:
> 
> 
> Jeff Layton <jlayton@kernel.org>于2022年1月29日 周六18:36写道：
> > On Sat, 2022-01-29 at 11:14 +0800, Yan, Zheng wrote:
> > > On Sat, Jan 29, 2022 at 2:32 AM Jeff Layton <jlayton@kernel.org>
> > > wrote:
> > > > 
> > > > I've noticed an intermittent hang waiting for caps in some
> > > > testing. What
> > > > I see is that the client will try to get caps for an operation
> > > > (e.g. a
> > > > read), and ends up waiting on the waitqueue forever. The caps
> > > > debugfs
> > > > file however shows that the caps it's waiting on have already
> > > > been
> > > > granted.
> > > > 
> > > > The current grant handling code will wake the waitqueue when it
> > > > sees
> > > > that there are newly-granted caps in the issued set. On an
> > > > import
> > > > however, we'll end up adding a new cap first, which fools the
> > > > logic into
> > > > thinking that nothing has changed. A later hack in the code
> > > > works around
> > > > this, but only for auth caps.
> > > 
> > > not right. handle_cap_import() saves old issued to extra_info-
> > > >issued.
> > > 
> > 
> > It does save the old issued value to extra_info->issued, but
> > handle_cap
> > grant consults cap->issued for most of its logic. It does check
> > against
> > extra_info->issued for auth caps to determine whether to wake
> > waiters,
> > but doesn't do that for non-auth caps. This patch corrects that.
> > 
> 
> 
> non auth mds never imports caps. ‘ci->i_auth_cap != cap’ happened only
> when client receives out-of-order messages for successive cap
> import/export. It’s unlikely in your case because you saw caps were
> there.
> 

Ok! I'll drop this patch, and keep hunting for how we're not getting
awoken. Let me know if you see any gaps in how "wake" gets set.

Thanks,
Jeff

> 
> > 
> > If you think this patch isn't right, can you elaborate on why and
> > what
> > would make it correct?
> > 
> > Thanks,
> > Jeff
> > 
> > > > 
> > > > Ensure we wake the waiters whenever we get an IMPORT that grants
> > > > new
> > > > caps for the inode.
> > > > 
> > > > URL: https://tracker.ceph.com/issues/54044
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >   fs/ceph/caps.c | 23 ++++++++++++-----------
> > > >   1 file changed, 12 insertions(+), 11 deletions(-)
> > > > 
> > > > I'm still testing this patch, but I think this may be the cause
> > > > of some
> > > > mysterious hangs I've hit in testing.
> > > > 
> > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > > index e668cdb9c99e..06b65a68e920 100644
> > > > --- a/fs/ceph/caps.c
> > > > +++ b/fs/ceph/caps.c
> > > > @@ -3541,21 +3541,22 @@ static void handle_cap_grant(struct
> > > > inode *inode,
> > > >                          fill_inline = true;
> > > >          }
> > > > 
> > > > -       if (ci->i_auth_cap == cap &&
> > > > -           le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
> > > > +       if (le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
> > > >                  if (newcaps & ~extra_info->issued)
> > > >                          wake = true;
> > > > 
> > > > -               if (ci->i_requested_max_size > max_size ||
> > > > -                   !(le32_to_cpu(grant->wanted) &
> > > > CEPH_CAP_ANY_FILE_WR)) {
> > > > -                       /* re-request max_size if necessary */
> > > > -                       ci->i_requested_max_size = 0;
> > > > -                       wake = true;
> > > > -               }
> > > > +               if (ci->i_auth_cap == cap) {
> > > > +                       if (ci->i_requested_max_size > max_size
> > > > ||
> > > > +                           !(le32_to_cpu(grant->wanted) &
> > > > CEPH_CAP_ANY_FILE_WR)) {
> > > > +                               /* re-request max_size if
> > > > necessary */
> > > > +                               ci->i_requested_max_size = 0;
> > > > +                               wake = true;
> > > > +                       }
> > > > 
> > > > -               ceph_kick_flushing_inode_caps(session, ci);
> > > > -               spin_unlock(&ci->i_ceph_lock);
> > > > -               up_read(&session->s_mdsc->snap_rwsem);
> > > > +                       ceph_kick_flushing_inode_caps(session,
> > > > ci);
> > > > +                       spin_unlock(&ci->i_ceph_lock);
> > > > +                       up_read(&session->s_mdsc->snap_rwsem);
> > > > +               }
> > > >          } else {
> > > >                  spin_unlock(&ci->i_ceph_lock);
> > > >          }
> > > > --
> > > > 2.34.1
> > > > 
> > 

-- 
Jeff Layton <jlayton@kernel.org>