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

[Jeff Layton <jlayton@kernel.org>]

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>