| Message ID | 20220428121318.43125-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: don't retain the caps if they're being revoked and not used | expand |
On Thu, 2022-04-28 at 20:13 +0800, Xiubo Li wrote: > For example if the Frwcb caps are being revoked, but only the Fr > caps is still being used then the kclient will skip releasing them > all. But in next turn if the Fr caps is ready to be released the > Fw caps maybe just being used again. So in corner case, such as > heavy load IOs, the revocation maybe stuck for a long time. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 0c0c8f5ae3b3..7eb5238941fc 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, > > /* The ones we currently want to retain (may be adjusted below) */ > retain = file_wanted | used | CEPH_CAP_PIN; > + > + /* > + * Do not retain the capabilities if they are under revoking > + * but not used, this could help speed up the revoking. > + */ > + retain &= ~((revoking & retain) & ~used); > + > if (!mdsc->stopping && inode->i_nlink > 0) { > if (file_wanted) { > retain |= CEPH_CAP_ANY; /* be greedy */ Oh wow, I thought this was being done already! Curse the complex logic in this function. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Thu, 2022-04-28 at 20:13 +0800, Xiubo Li wrote: > For example if the Frwcb caps are being revoked, but only the Fr > caps is still being used then the kclient will skip releasing them > all. But in next turn if the Fr caps is ready to be released the > Fw caps maybe just being used again. So in corner case, such as > heavy load IOs, the revocation maybe stuck for a long time. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 0c0c8f5ae3b3..7eb5238941fc 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, > > /* The ones we currently want to retain (may be adjusted below) */ > retain = file_wanted | used | CEPH_CAP_PIN; > + > + /* > + * Do not retain the capabilities if they are under revoking > + * but not used, this could help speed up the revoking. > + */ > + retain &= ~((revoking & retain) & ~used); > + > if (!mdsc->stopping && inode->i_nlink > 0) { > if (file_wanted) { > retain |= CEPH_CAP_ANY; /* be greedy */ Actually maybe something like this would be simpler to understand? /* Retain any caps that are actively in use */ retain = used | CEPH_CAP_PIN; /* Also retain caps wanted by open files, if they aren't being revoked */ retain |= (file_wanted & ~revoking);
On 4/28/22 8:29 PM, Jeff Layton wrote: > On Thu, 2022-04-28 at 20:13 +0800, Xiubo Li wrote: >> For example if the Frwcb caps are being revoked, but only the Fr >> caps is still being used then the kclient will skip releasing them >> all. But in next turn if the Fr caps is ready to be released the >> Fw caps maybe just being used again. So in corner case, such as >> heavy load IOs, the revocation maybe stuck for a long time. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/caps.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 0c0c8f5ae3b3..7eb5238941fc 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, >> >> /* The ones we currently want to retain (may be adjusted below) */ >> retain = file_wanted | used | CEPH_CAP_PIN; >> + >> + /* >> + * Do not retain the capabilities if they are under revoking >> + * but not used, this could help speed up the revoking. >> + */ >> + retain &= ~((revoking & retain) & ~used); >> + >> if (!mdsc->stopping && inode->i_nlink > 0) { >> if (file_wanted) { >> retain |= CEPH_CAP_ANY; /* be greedy */ > Actually maybe something like this would be simpler to understand? > > /* Retain any caps that are actively in use */ > retain = used | CEPH_CAP_PIN; > > /* Also retain caps wanted by open files, if they aren't being revoked */ > retain |= (file_wanted & ~revoking); Yeah, sounds good, let me try to simplify it to make it more readable. THanks.
On 4/28/22 8:29 PM, Jeff Layton wrote: > On Thu, 2022-04-28 at 20:13 +0800, Xiubo Li wrote: >> For example if the Frwcb caps are being revoked, but only the Fr >> caps is still being used then the kclient will skip releasing them >> all. But in next turn if the Fr caps is ready to be released the >> Fw caps maybe just being used again. So in corner case, such as >> heavy load IOs, the revocation maybe stuck for a long time. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/caps.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 0c0c8f5ae3b3..7eb5238941fc 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, >> >> /* The ones we currently want to retain (may be adjusted below) */ >> retain = file_wanted | used | CEPH_CAP_PIN; >> + >> + /* >> + * Do not retain the capabilities if they are under revoking >> + * but not used, this could help speed up the revoking. >> + */ >> + retain &= ~((revoking & retain) & ~used); >> + >> if (!mdsc->stopping && inode->i_nlink > 0) { >> if (file_wanted) { >> retain |= CEPH_CAP_ANY; /* be greedy */ > Actually maybe something like this would be simpler to understand? > > /* Retain any caps that are actively in use */ > retain = used | CEPH_CAP_PIN; > > /* Also retain caps wanted by open files, if they aren't being revoked */ > retain |= (file_wanted & ~revoking); This won't work, because the caps maybe '|' back again in the following code: 1950 if (!mdsc->stopping && inode->i_nlink > 0) { 1951 if (file_wanted) { 1952 retain |= CEPH_CAP_ANY; /* be greedy */ ... 1979 } I have adjust the code and simplified it in V2. -- Xiubo >
On Thu, Apr 28, 2022 at 11:42 PM Xiubo Li <xiubli@redhat.com> wrote: > > For example if the Frwcb caps are being revoked, but only the Fr > caps is still being used then the kclient will skip releasing them > all. But in next turn if the Fr caps is ready to be released the > Fw caps maybe just being used again. So in corner case, such as > heavy load IOs, the revocation maybe stuck for a long time. > This does not make sense. If Frwcb are being revoked, writer can't get Fw again. Second, Frwcb are managed by single lock at mds side. Partial releasing caps does make lock state transition possible. > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 0c0c8f5ae3b3..7eb5238941fc 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, > > /* The ones we currently want to retain (may be adjusted below) */ > retain = file_wanted | used | CEPH_CAP_PIN; > + > + /* > + * Do not retain the capabilities if they are under revoking > + * but not used, this could help speed up the revoking. > + */ > + retain &= ~((revoking & retain) & ~used); > + > if (!mdsc->stopping && inode->i_nlink > 0) { > if (file_wanted) { > retain |= CEPH_CAP_ANY; /* be greedy */ > -- > 2.36.0.rc1 >
On Fri, Apr 29, 2022 at 10:46 AM Yan, Zheng <ukernel@gmail.com> wrote: > > On Thu, Apr 28, 2022 at 11:42 PM Xiubo Li <xiubli@redhat.com> wrote: > > > > For example if the Frwcb caps are being revoked, but only the Fr > > caps is still being used then the kclient will skip releasing them > > all. But in next turn if the Fr caps is ready to be released the > > Fw caps maybe just being used again. So in corner case, such as > > heavy load IOs, the revocation maybe stuck for a long time. > > > This does not make sense. If Frwcb are being revoked, writer can't get > Fw again. Second, Frwcb are managed by single lock at mds side. > Partial releasing caps does make lock state transition possible. > I mean, does not make lock state transition possible. > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > --- > > fs/ceph/caps.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > index 0c0c8f5ae3b3..7eb5238941fc 100644 > > --- a/fs/ceph/caps.c > > +++ b/fs/ceph/caps.c > > @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, > > > > /* The ones we currently want to retain (may be adjusted below) */ > > retain = file_wanted | used | CEPH_CAP_PIN; > > + > > + /* > > + * Do not retain the capabilities if they are under revoking > > + * but not used, this could help speed up the revoking. > > + */ > > + retain &= ~((revoking & retain) & ~used); > > + > > if (!mdsc->stopping && inode->i_nlink > 0) { > > if (file_wanted) { > > retain |= CEPH_CAP_ANY; /* be greedy */ > > -- > > 2.36.0.rc1 > >
On 4/29/22 10:46 AM, Yan, Zheng wrote: > On Thu, Apr 28, 2022 at 11:42 PM Xiubo Li <xiubli@redhat.com> wrote: >> For example if the Frwcb caps are being revoked, but only the Fr >> caps is still being used then the kclient will skip releasing them >> all. But in next turn if the Fr caps is ready to be released the >> Fw caps maybe just being used again. So in corner case, such as >> heavy load IOs, the revocation maybe stuck for a long time. >> > This does not make sense. If Frwcb are being revoked, writer can't get > Fw again. Second, Frwcb are managed by single lock at mds side. > Partial releasing caps does make lock state transition possible. > Yeah, you are right. Checked the __ceph_caps_issued() it really has removed the caps being revoked already. Thanks Zheng. >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/caps.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 0c0c8f5ae3b3..7eb5238941fc 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, >> >> /* The ones we currently want to retain (may be adjusted below) */ >> retain = file_wanted | used | CEPH_CAP_PIN; >> + >> + /* >> + * Do not retain the capabilities if they are under revoking >> + * but not used, this could help speed up the revoking. >> + */ >> + retain &= ~((revoking & retain) & ~used); >> + >> if (!mdsc->stopping && inode->i_nlink > 0) { >> if (file_wanted) { >> retain |= CEPH_CAP_ANY; /* be greedy */ >> -- >> 2.36.0.rc1 >>
On Fri, 2022-04-29 at 14:28 +0800, Xiubo Li wrote: > On 4/29/22 10:46 AM, Yan, Zheng wrote: > > On Thu, Apr 28, 2022 at 11:42 PM Xiubo Li <xiubli@redhat.com> wrote: > > > For example if the Frwcb caps are being revoked, but only the Fr > > > caps is still being used then the kclient will skip releasing them > > > all. But in next turn if the Fr caps is ready to be released the > > > Fw caps maybe just being used again. So in corner case, such as > > > heavy load IOs, the revocation maybe stuck for a long time. > > > > > This does not make sense. If Frwcb are being revoked, writer can't get > > Fw again. Second, Frwcb are managed by single lock at mds side. > > Partial releasing caps does make lock state transition possible. > > > Yeah, you are right. Checked the __ceph_caps_issued() it really has > removed the caps being revoked already. > > Thanks Zheng. > Based on this discussion, I'm going to drop this patch from the testing branch. > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > > --- > > > fs/ceph/caps.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > > index 0c0c8f5ae3b3..7eb5238941fc 100644 > > > --- a/fs/ceph/caps.c > > > +++ b/fs/ceph/caps.c > > > @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, > > > > > > /* The ones we currently want to retain (may be adjusted below) */ > > > retain = file_wanted | used | CEPH_CAP_PIN; > > > + > > > + /* > > > + * Do not retain the capabilities if they are under revoking > > > + * but not used, this could help speed up the revoking. > > > + */ > > > + retain &= ~((revoking & retain) & ~used); > > > + > > > if (!mdsc->stopping && inode->i_nlink > 0) { > > > if (file_wanted) { > > > retain |= CEPH_CAP_ANY; /* be greedy */ > > > -- > > > 2.36.0.rc1 > > > > Thanks,
On 5/3/22 8:49 PM, Jeff Layton wrote: > On Fri, 2022-04-29 at 14:28 +0800, Xiubo Li wrote: >> On 4/29/22 10:46 AM, Yan, Zheng wrote: >>> On Thu, Apr 28, 2022 at 11:42 PM Xiubo Li <xiubli@redhat.com> wrote: >>>> For example if the Frwcb caps are being revoked, but only the Fr >>>> caps is still being used then the kclient will skip releasing them >>>> all. But in next turn if the Fr caps is ready to be released the >>>> Fw caps maybe just being used again. So in corner case, such as >>>> heavy load IOs, the revocation maybe stuck for a long time. >>>> >>> This does not make sense. If Frwcb are being revoked, writer can't get >>> Fw again. Second, Frwcb are managed by single lock at mds side. >>> Partial releasing caps does make lock state transition possible. >>> >> Yeah, you are right. Checked the __ceph_caps_issued() it really has >> removed the caps being revoked already. >> >> Thanks Zheng. >> > Based on this discussion, I'm going to drop this patch from the testing > branch. Sure, thanks Jeff. Planed to drop it today. -- Xiubo >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>> --- >>>> fs/ceph/caps.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >>>> index 0c0c8f5ae3b3..7eb5238941fc 100644 >>>> --- a/fs/ceph/caps.c >>>> +++ b/fs/ceph/caps.c >>>> @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, >>>> >>>> /* The ones we currently want to retain (may be adjusted below) */ >>>> retain = file_wanted | used | CEPH_CAP_PIN; >>>> + >>>> + /* >>>> + * Do not retain the capabilities if they are under revoking >>>> + * but not used, this could help speed up the revoking. >>>> + */ >>>> + retain &= ~((revoking & retain) & ~used); >>>> + >>>> if (!mdsc->stopping && inode->i_nlink > 0) { >>>> if (file_wanted) { >>>> retain |= CEPH_CAP_ANY; /* be greedy */ >>>> -- >>>> 2.36.0.rc1 >>>> > Thanks,
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 0c0c8f5ae3b3..7eb5238941fc 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -1947,6 +1947,13 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags, /* The ones we currently want to retain (may be adjusted below) */ retain = file_wanted | used | CEPH_CAP_PIN; + + /* + * Do not retain the capabilities if they are under revoking + * but not used, this could help speed up the revoking. + */ + retain &= ~((revoking & retain) & ~used); + if (!mdsc->stopping && inode->i_nlink > 0) { if (file_wanted) { retain |= CEPH_CAP_ANY; /* be greedy */
For example if the Frwcb caps are being revoked, but only the Fr caps is still being used then the kclient will skip releasing them all. But in next turn if the Fr caps is ready to be released the Fw caps maybe just being used again. So in corner case, such as heavy load IOs, the revocation maybe stuck for a long time. Signed-off-by: Xiubo Li <xiubli@redhat.com> --- fs/ceph/caps.c | 7 +++++++ 1 file changed, 7 insertions(+)