* [PATCH] ceph: wake waiters on any IMPORT that grants new caps @ 2022-01-27 20:08 Jeff Layton 2022-01-29 3:14 ` Yan, Zheng 0 siblings, 1 reply; 4+ messages in thread From: Jeff Layton @ 2022-01-27 20:08 UTC (permalink / raw) To: ceph-devel; +Cc: idryomov 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ceph: wake waiters on any IMPORT that grants new caps 2022-01-27 20:08 [PATCH] ceph: wake waiters on any IMPORT that grants new caps Jeff Layton @ 2022-01-29 3:14 ` Yan, Zheng 2022-01-29 10:36 ` Jeff Layton 0 siblings, 1 reply; 4+ messages in thread From: Yan, Zheng @ 2022-01-29 3:14 UTC (permalink / raw) To: Jeff Layton; +Cc: ceph-devel, Ilya Dryomov 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ceph: wake waiters on any IMPORT that grants new caps 2022-01-29 3:14 ` Yan, Zheng @ 2022-01-29 10:36 ` Jeff Layton [not found] ` <CAAM7YAn+8H5nv_Y3hBDtEGA5=jbtsG8-RX=dGU6wB3RRH-HGmw@mail.gmail.com> 0 siblings, 1 reply; 4+ messages in thread From: Jeff Layton @ 2022-01-29 10:36 UTC (permalink / raw) To: Yan, Zheng; +Cc: ceph-devel, Ilya Dryomov 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> ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <CAAM7YAn+8H5nv_Y3hBDtEGA5=jbtsG8-RX=dGU6wB3RRH-HGmw@mail.gmail.com>]
* Re: [PATCH] ceph: wake waiters on any IMPORT that grants new caps [not found] ` <CAAM7YAn+8H5nv_Y3hBDtEGA5=jbtsG8-RX=dGU6wB3RRH-HGmw@mail.gmail.com> @ 2022-02-02 12:40 ` Jeff Layton 0 siblings, 0 replies; 4+ messages in thread From: Jeff Layton @ 2022-02-02 12:40 UTC (permalink / raw) To: Yan, Zheng; +Cc: Ilya Dryomov, ceph-devel 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> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-02 12:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27 20:08 [PATCH] ceph: wake waiters on any IMPORT that grants new caps Jeff Layton
2022-01-29 3:14 ` Yan, Zheng
2022-01-29 10:36 ` Jeff Layton
[not found] ` <CAAM7YAn+8H5nv_Y3hBDtEGA5=jbtsG8-RX=dGU6wB3RRH-HGmw@mail.gmail.com>
2022-02-02 12:40 ` Jeff Layton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.