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

[Jeff Layton <jlayton@kernel.org>]

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>