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@xxxxxxxxxx wrote:
> > > > > From: Xiubo Li <xiubli@xxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxx>
