| Message ID | 20230627235709.201132-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ceph: don't let check_caps skip sending responses for revoke msgs | expand |
Looks good to me. Reviewed-by: Milind Changire <mchangir@redhat.com> On Wed, Jun 28, 2023 at 5:29 AM <xiubli@redhat.com> wrote: > > From: Xiubo Li <xiubli@redhat.com> > > If a client sends out a cap-update request with the old 'seq' just > before a pending cap revoke request, then the MDS might miscalculate > the 'seqs' and caps. It's therefore always a good idea to ack the > cap revoke request with the bumped up 'seq'. > > Cc: stable@vger.kernel.org > Cc: Patrick Donnelly <pdonnell@redhat.com> > URL: https://tracker.ceph.com/issues/61782 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > > V2: > - Rephrased the commit comment for better understanding from Milind > > > fs/ceph/caps.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 1052885025b3..eee2fbca3430 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -3737,6 +3737,15 @@ static void handle_cap_grant(struct inode *inode, > } > BUG_ON(cap->issued & ~cap->implemented); > > + /* don't let check_caps skip sending a response to MDS for revoke msgs */ > + if (le32_to_cpu(grant->op) == CEPH_CAP_OP_REVOKE) { > + cap->mds_wanted = 0; > + if (cap == ci->i_auth_cap) > + check_caps = 1; /* check auth cap only */ > + else > + check_caps = 2; /* check all caps */ > + } > + > if (extra_info->inline_version > 0 && > extra_info->inline_version >= ci->i_inline_version) { > ci->i_inline_version = extra_info->inline_version; > -- > 2.40.1 >
Patch looks good to me. Sorry I must nitpick the commit message wording: If a client sends out a cap update dropping caps with the prior 'seq' just before an incoming cap revoke request, then the client may drop the revoke because it believes it's already released the requested capabilities. This causes the MDS to wait indefinitely for the client to respond to the revoke. It's therefore always a good idea to ack the cap revoke request with the bumped up 'seq'. On Tue, Jun 27, 2023 at 7:59 PM <xiubli@redhat.com> wrote: > > From: Xiubo Li <xiubli@redhat.com> > > If a client sends out a cap-update request with the old 'seq' just > before a pending cap revoke request, then the MDS might miscalculate > the 'seqs' and caps. It's therefore always a good idea to ack the > cap revoke request with the bumped up 'seq'. > > Cc: stable@vger.kernel.org > Cc: Patrick Donnelly <pdonnell@redhat.com> > URL: https://tracker.ceph.com/issues/61782 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > > V2: > - Rephrased the commit comment for better understanding from Milind > > > fs/ceph/caps.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 1052885025b3..eee2fbca3430 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -3737,6 +3737,15 @@ static void handle_cap_grant(struct inode *inode, > } > BUG_ON(cap->issued & ~cap->implemented); > > + /* don't let check_caps skip sending a response to MDS for revoke msgs */ > + if (le32_to_cpu(grant->op) == CEPH_CAP_OP_REVOKE) { > + cap->mds_wanted = 0; > + if (cap == ci->i_auth_cap) > + check_caps = 1; /* check auth cap only */ > + else > + check_caps = 2; /* check all caps */ > + } > + > if (extra_info->inline_version > 0 && > extra_info->inline_version >= ci->i_inline_version) { > ci->i_inline_version = extra_info->inline_version; > -- > 2.40.1 >
On 6/29/23 09:01, Patrick Donnelly wrote: > Patch looks good to me. Sorry I must nitpick the commit message wording: > > If a client sends out a cap update dropping caps with the prior 'seq' just > before an incoming cap revoke request, then the client may drop the revoke > because it believes it's already released the requested capabilities. > This causes > the MDS to wait indefinitely for the client to respond to the revoke. > It's therefore always a good idea to ack the cap revoke request with > the bumped up 'seq'. Updated it by sending out the V3. Thanks - Xiubo > On Tue, Jun 27, 2023 at 7:59 PM <xiubli@redhat.com> wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> If a client sends out a cap-update request with the old 'seq' just >> before a pending cap revoke request, then the MDS might miscalculate >> the 'seqs' and caps. It's therefore always a good idea to ack the >> cap revoke request with the bumped up 'seq'. >> >> Cc: stable@vger.kernel.org >> Cc: Patrick Donnelly <pdonnell@redhat.com> >> URL: https://tracker.ceph.com/issues/61782 >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> >> V2: >> - Rephrased the commit comment for better understanding from Milind >> >> >> fs/ceph/caps.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 1052885025b3..eee2fbca3430 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -3737,6 +3737,15 @@ static void handle_cap_grant(struct inode *inode, >> } >> BUG_ON(cap->issued & ~cap->implemented); >> >> + /* don't let check_caps skip sending a response to MDS for revoke msgs */ >> + if (le32_to_cpu(grant->op) == CEPH_CAP_OP_REVOKE) { >> + cap->mds_wanted = 0; >> + if (cap == ci->i_auth_cap) >> + check_caps = 1; /* check auth cap only */ >> + else >> + check_caps = 2; /* check all caps */ >> + } >> + >> if (extra_info->inline_version > 0 && >> extra_info->inline_version >= ci->i_inline_version) { >> ci->i_inline_version = extra_info->inline_version; >> -- >> 2.40.1 >> >
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 1052885025b3..eee2fbca3430 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -3737,6 +3737,15 @@ static void handle_cap_grant(struct inode *inode, } BUG_ON(cap->issued & ~cap->implemented); + /* don't let check_caps skip sending a response to MDS for revoke msgs */ + if (le32_to_cpu(grant->op) == CEPH_CAP_OP_REVOKE) { + cap->mds_wanted = 0; + if (cap == ci->i_auth_cap) + check_caps = 1; /* check auth cap only */ + else + check_caps = 2; /* check all caps */ + } + if (extra_info->inline_version > 0 && extra_info->inline_version >= ci->i_inline_version) { ci->i_inline_version = extra_info->inline_version;