| Message ID | 20220411093405.301667-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,resend] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check | expand |
On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > From the posix and the initial statx supporting commit comments, > the AT_STATX_DONT_SYNC is a lightweight stat flag and the > AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all > the other current usage about these two flags they are all doing > the same, that is only when the AT_STATX_FORCE_SYNC is not set > and the AT_STATX_DONT_SYNC is set will they skip sync retriving > the attributes from storage. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 6788a1f88eb6..1ee6685def83 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path, > return -ESTALE; > > /* Skip the getattr altogether if we're asked not to sync */ > - if (!(flags & AT_STATX_DONT_SYNC)) { > + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { > err = ceph_do_getattr(inode, > statx_to_caps(request_mask, inode->i_mode), > flags & AT_STATX_FORCE_SYNC); I don't get it. The only way I can see that this is a problem is if someone sent down a mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I don't see that ignoring FORCE_SYNC would be wrong...
On 4/18/22 6:15 PM, Jeff Layton wrote: > On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> From the posix and the initial statx supporting commit comments, >> the AT_STATX_DONT_SYNC is a lightweight stat flag and the >> AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all >> the other current usage about these two flags they are all doing >> the same, that is only when the AT_STATX_FORCE_SYNC is not set >> and the AT_STATX_DONT_SYNC is set will they skip sync retriving >> the attributes from storage. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/inode.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >> index 6788a1f88eb6..1ee6685def83 100644 >> --- a/fs/ceph/inode.c >> +++ b/fs/ceph/inode.c >> @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path, >> return -ESTALE; >> >> /* Skip the getattr altogether if we're asked not to sync */ >> - if (!(flags & AT_STATX_DONT_SYNC)) { >> + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { >> err = ceph_do_getattr(inode, >> statx_to_caps(request_mask, inode->i_mode), >> flags & AT_STATX_FORCE_SYNC); > I don't get it. > > The only way I can see that this is a problem is if someone sent down a > mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I > don't see that ignoring FORCE_SYNC would be wrong... > There has 3 cases for the flags: case1: flags & AT_STATX_SYNC_TYPE == 0 case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC | AT_STATX_FORCE_SYNC Only in case2, which is only the DONT_SYNC bit is set, will ignore calling ceph_do_getattr() here. And for case3 it will ignore the DONT_SYNC bit. -- Xiubo
On Mon, 2022-04-18 at 18:25 +0800, Xiubo Li wrote: > On 4/18/22 6:15 PM, Jeff Layton wrote: > > On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote: > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > From the posix and the initial statx supporting commit comments, > > > the AT_STATX_DONT_SYNC is a lightweight stat flag and the > > > AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all > > > the other current usage about these two flags they are all doing > > > the same, that is only when the AT_STATX_FORCE_SYNC is not set > > > and the AT_STATX_DONT_SYNC is set will they skip sync retriving > > > the attributes from storage. > > > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > > --- > > > fs/ceph/inode.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > > index 6788a1f88eb6..1ee6685def83 100644 > > > --- a/fs/ceph/inode.c > > > +++ b/fs/ceph/inode.c > > > @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path, > > > return -ESTALE; > > > > > > /* Skip the getattr altogether if we're asked not to sync */ > > > - if (!(flags & AT_STATX_DONT_SYNC)) { > > > + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { > > > err = ceph_do_getattr(inode, > > > statx_to_caps(request_mask, inode->i_mode), > > > flags & AT_STATX_FORCE_SYNC); > > I don't get it. > > > > The only way I can see that this is a problem is if someone sent down a > > mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I > > don't see that ignoring FORCE_SYNC would be wrong... > > > There has 3 cases for the flags: > > case1: flags & AT_STATX_SYNC_TYPE == 0 > > case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC > > case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC | > AT_STATX_FORCE_SYNC > > > Only in case2, which is only the DONT_SYNC bit is set, will ignore > calling ceph_do_getattr() here. And for case3 it will ignore the > DONT_SYNC bit. > Sure, but the patch doesn't functionally change the behavior of the code. It may make the condition more idiomatic to read, but I don't think there is a bug here.
On 4/18/22 6:29 PM, Jeff Layton wrote: > On Mon, 2022-04-18 at 18:25 +0800, Xiubo Li wrote: >> On 4/18/22 6:15 PM, Jeff Layton wrote: >>> On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote: >>>> From: Xiubo Li <xiubli@redhat.com> >>>> >>>> From the posix and the initial statx supporting commit comments, >>>> the AT_STATX_DONT_SYNC is a lightweight stat flag and the >>>> AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all >>>> the other current usage about these two flags they are all doing >>>> the same, that is only when the AT_STATX_FORCE_SYNC is not set >>>> and the AT_STATX_DONT_SYNC is set will they skip sync retriving >>>> the attributes from storage. >>>> >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>> --- >>>> fs/ceph/inode.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >>>> index 6788a1f88eb6..1ee6685def83 100644 >>>> --- a/fs/ceph/inode.c >>>> +++ b/fs/ceph/inode.c >>>> @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path, >>>> return -ESTALE; >>>> >>>> /* Skip the getattr altogether if we're asked not to sync */ >>>> - if (!(flags & AT_STATX_DONT_SYNC)) { >>>> + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { >>>> err = ceph_do_getattr(inode, >>>> statx_to_caps(request_mask, inode->i_mode), >>>> flags & AT_STATX_FORCE_SYNC); >>> I don't get it. >>> >>> The only way I can see that this is a problem is if someone sent down a >>> mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I >>> don't see that ignoring FORCE_SYNC would be wrong... >>> >> There has 3 cases for the flags: >> >> case1: flags & AT_STATX_SYNC_TYPE == 0 >> >> case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC >> >> case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC | >> AT_STATX_FORCE_SYNC >> >> >> Only in case2, which is only the DONT_SYNC bit is set, will ignore >> calling ceph_do_getattr() here. And for case3 it will ignore the >> DONT_SYNC bit. >> > Sure, but the patch doesn't functionally change the behavior of the > code. It may make the condition more idiomatic to read, but I don't > think there is a bug here. - if (!(flags & AT_STATX_DONT_SYNC)) { For example, in both case2 and case3 the above condition is false, right ? That means for case3 it will ignore the FORCE_SYNC always. + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { For exmaple in case2 the above condition is false and then it will skip calling the ceph_do_getattr(). And in case3 the above condition is true, it will call the ceph_do_getattr(..., flags & FORCE_SYNC). The logic changed, right ? -- Xiubo
On Mon, 2022-04-18 at 18:51 +0800, Xiubo Li wrote: > On 4/18/22 6:29 PM, Jeff Layton wrote: > > On Mon, 2022-04-18 at 18:25 +0800, Xiubo Li wrote: > > > On 4/18/22 6:15 PM, Jeff Layton wrote: > > > > On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote: > > > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > > > > > From the posix and the initial statx supporting commit comments, > > > > > the AT_STATX_DONT_SYNC is a lightweight stat flag and the > > > > > AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all > > > > > the other current usage about these two flags they are all doing > > > > > the same, that is only when the AT_STATX_FORCE_SYNC is not set > > > > > and the AT_STATX_DONT_SYNC is set will they skip sync retriving > > > > > the attributes from storage. > > > > > > > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > > > > --- > > > > > fs/ceph/inode.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > > > > index 6788a1f88eb6..1ee6685def83 100644 > > > > > --- a/fs/ceph/inode.c > > > > > +++ b/fs/ceph/inode.c > > > > > @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path, > > > > > return -ESTALE; > > > > > > > > > > /* Skip the getattr altogether if we're asked not to sync */ > > > > > - if (!(flags & AT_STATX_DONT_SYNC)) { > > > > > + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { > > > > > err = ceph_do_getattr(inode, > > > > > statx_to_caps(request_mask, inode->i_mode), > > > > > flags & AT_STATX_FORCE_SYNC); > > > > I don't get it. > > > > > > > > The only way I can see that this is a problem is if someone sent down a > > > > mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I > > > > don't see that ignoring FORCE_SYNC would be wrong... > > > > > > > There has 3 cases for the flags: > > > > > > case1: flags & AT_STATX_SYNC_TYPE == 0 > > > > > > case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC > > > > > > case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC | > > > AT_STATX_FORCE_SYNC > > > > > > > > > Only in case2, which is only the DONT_SYNC bit is set, will ignore > > > calling ceph_do_getattr() here. And for case3 it will ignore the > > > DONT_SYNC bit. > > > > > Sure, but the patch doesn't functionally change the behavior of the > > code. It may make the condition more idiomatic to read, but I don't > > think there is a bug here. > > - if (!(flags & AT_STATX_DONT_SYNC)) { > > > For example, in both case2 and case3 the above condition is false, right > ? That means for case3 it will ignore the FORCE_SYNC always. > > + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { > > For exmaple in case2 the above condition is false and then it will skip > calling the ceph_do_getattr(). And in case3 the above condition is true, > it will call the ceph_do_getattr(..., flags & FORCE_SYNC). > > The logic changed, right ? > Yes, but my argument is that sending down a mask that has AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC makes no sense whatsoever. You're simultaneously asking it to not fetch attributes and to forcibly fetch attributes. We're within our rights to just ignore FORCE_SYNC at that point. Arguably, this ought to be caught in vfs_statx before we ever call down into the fs driver with something like: if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC)) return -EINVAL; David, should we add something like this, or is this too risky a change?
Jeff Layton <jlayton@kernel.org> wrote:
> if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC))
You can't do that. DONT_SYNC and FORCE_SYNC aren't bit flags - they're an
enumeration in a bit field. There's a reserved value at 0x6000 that doesn't
have a symbol assigned.
David
On Tue, 2022-04-19 at 14:22 +0100, David Howells wrote: > Jeff Layton <jlayton@kernel.org> wrote: > > > if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC)) > > You can't do that. DONT_SYNC and FORCE_SYNC aren't bit flags - they're an > enumeration in a bit field. There's a reserved value at 0x6000 that doesn't > have a symbol assigned. > Right, but nothing prevents you from setting both flags at the same time. How should we interpret such a request?
Jeff Layton <jlayton@kernel.org> wrote: > On Tue, 2022-04-19 at 14:22 +0100, David Howells wrote: > > Jeff Layton <jlayton@kernel.org> wrote: > > > > > if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC)) > > > > You can't do that. DONT_SYNC and FORCE_SYNC aren't bit flags - they're an > > enumeration in a bit field. There's a reserved value at 0x6000 that doesn't > > have a symbol assigned. > > > > Right, but nothing prevents you from setting both flags at the same > time. How should we interpret such a request? A good question without a necessarily right answer. Possibly we should do: #define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */ #define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */ #define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */ #define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */ +#define AT_STATX_SYNC_RESERVED 0x6000 and give EINVAL if we see the reserved value. But also these values can be considered a hint, so possibly we should just ignore the reserved value. Oh for fsinfo()... David
On Tue, 2022-04-19 at 14:30 +0100, David Howells wrote: > Jeff Layton <jlayton@kernel.org> wrote: > > > On Tue, 2022-04-19 at 14:22 +0100, David Howells wrote: > > > Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC)) > > > > > > You can't do that. DONT_SYNC and FORCE_SYNC aren't bit flags - they're an > > > enumeration in a bit field. There's a reserved value at 0x6000 that doesn't > > > have a symbol assigned. > > > > > > > Right, but nothing prevents you from setting both flags at the same > > time. How should we interpret such a request? > > A good question without a necessarily right answer. > > Possibly we should do: > > #define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */ > #define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */ > #define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */ > #define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */ > +#define AT_STATX_SYNC_RESERVED 0x6000 > > and give EINVAL if we see the reserved value. But also these values can be > considered a hint, so possibly we should just ignore the reserved value. Oh > for fsinfo()... > > David > That was what the code (pre-patch) did. If someone set DONT_SYNC|FORCE_SYNC, it would just ignore FORCE_SYNC. It's not ideal, but I suppose we're within our rights to prefer either behaviour in that case if someone sets both flags. In hindsight, setting both should have probably caused the syscall to throw back -EINVAL, but changing that now is probably a bit dangerous.
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 6788a1f88eb6..1ee6685def83 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path, return -ESTALE; /* Skip the getattr altogether if we're asked not to sync */ - if (!(flags & AT_STATX_DONT_SYNC)) { + if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) { err = ceph_do_getattr(inode, statx_to_caps(request_mask, inode->i_mode), flags & AT_STATX_FORCE_SYNC);