| Message ID | 20191127104549.33305-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ceph: fix cap revoke race | expand |
On Wed, 2019-11-27 at 05:45 -0500, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > The cap->implemented is one subset of the cap->issued, the logic > here want to exclude the revoking caps, but the following code > will be (~cap->implemented | cap->issued) == 0xFFFF, so it will > make no sense when we doing the "have &= 0xFFFF". > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index c62e88da4fee..a9335402c2a5 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info *ci, int *implemented) > */ > if (ci->i_auth_cap) { > cap = ci->i_auth_cap; > - have &= ~cap->implemented | cap->issued; > + have &= ~(cap->implemented & ~cap->issued); > } > return have; > } Nice catch. This patch looks correct to me. I'll merge it into the testing branch and we'll see if anything breaks.
On 11/27/19 6:45 PM, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > The cap->implemented is one subset of the cap->issued, the logic > here want to exclude the revoking caps, but the following code > will be (~cap->implemented | cap->issued) == 0xFFFF, so it will > make no sense when we doing the "have &= 0xFFFF". > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index c62e88da4fee..a9335402c2a5 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info *ci, int *implemented) > */ > if (ci->i_auth_cap) { > cap = ci->i_auth_cap; > - have &= ~cap->implemented | cap->issued; > + have &= ~(cap->implemented & ~cap->issued); The end result is the same. See https://en.wikipedia.org/wiki/De_Morgan%27s_laws > } > return have; > } >
On 2019/11/28 10:25, Yan, Zheng wrote: > On 11/27/19 6:45 PM, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> The cap->implemented is one subset of the cap->issued, the logic >> here want to exclude the revoking caps, but the following code >> will be (~cap->implemented | cap->issued) == 0xFFFF, so it will >> make no sense when we doing the "have &= 0xFFFF". >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/caps.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index c62e88da4fee..a9335402c2a5 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info >> *ci, int *implemented) >> */ >> if (ci->i_auth_cap) { >> cap = ci->i_auth_cap; >> - have &= ~cap->implemented | cap->issued; >> + have &= ~(cap->implemented & ~cap->issued); > > The end result is the same. > > See https://en.wikipedia.org/wiki/De_Morgan%27s_laws > Yeah, right, it is. BRs >> } >> return have; >> } >> >
On Thu, 2019-11-28 at 15:53 +0800, Xiubo Li wrote: > On 2019/11/28 10:25, Yan, Zheng wrote: > > On 11/27/19 6:45 PM, xiubli@redhat.com wrote: > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > The cap->implemented is one subset of the cap->issued, the logic > > > here want to exclude the revoking caps, but the following code > > > will be (~cap->implemented | cap->issued) == 0xFFFF, so it will > > > make no sense when we doing the "have &= 0xFFFF". > > > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > > --- > > > fs/ceph/caps.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > > index c62e88da4fee..a9335402c2a5 100644 > > > --- a/fs/ceph/caps.c > > > +++ b/fs/ceph/caps.c > > > @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info > > > *ci, int *implemented) > > > */ > > > if (ci->i_auth_cap) { > > > cap = ci->i_auth_cap; > > > - have &= ~cap->implemented | cap->issued; > > > + have &= ~(cap->implemented & ~cap->issued); > > > > The end result is the same. > > > > See https://en.wikipedia.org/wiki/De_Morgan%27s_laws > > > Yeah, right, it is. > > BRs > Dropping this patch. Cheers,
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index c62e88da4fee..a9335402c2a5 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info *ci, int *implemented) */ if (ci->i_auth_cap) { cap = ci->i_auth_cap; - have &= ~cap->implemented | cap->issued; + have &= ~(cap->implemented & ~cap->issued); } return have; }