Re: [PATCH 4/5] fuse: implement statx

[Miklos Szeredi <miklos@szeredi.hu>]

On Tue, 22 Aug 2023 at 18:39, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/10/23 12:55, Miklos Szeredi wrote:
> > Allow querying btime.  When btime is requested in mask, then FUSE_STATX
> > request is sent.  Otherwise keep using FUSE_GETATTR.
> >
> > The userspace interface for statx matches that of the statx(2) API.
> > However there are limitations on how this interface is used:
> >
> >   - returned basic stats and btime are used, stx_attributes, etc. are
> >     ignored
> >
> >   - always query basic stats and btime, regardless of what was requested
> >
> >   - requested sync type is ignored, the default is passed to the server
> >
> >   - if server returns with some attributes missing from the result_mask,
> >     then no attributes will be cached
> >
> >   - btime is not cached yet (next patch will fix that)
> >
> > For new inodes initialize fi->inval_mask to "all invalid", instead of "all
> > valid" as previously.  Also only clear basic stats from inval_mask when
> > caching attributes.  This will result in the caching logic not thinking
> > that btime is cached.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >   fs/fuse/dir.c    | 106 ++++++++++++++++++++++++++++++++++++++++++++---
> >   fs/fuse/fuse_i.h |   3 ++
> >   fs/fuse/inode.c  |   5 ++-
> >   3 files changed, 107 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 04006db6e173..552157bd6a4d 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -350,10 +350,14 @@ int fuse_valid_type(int m)
> >               S_ISBLK(m) || S_ISFIFO(m) || S_ISSOCK(m);
> >   }
> >
> > +bool fuse_valid_size(u64 size)
> > +{
> > +     return size <= LLONG_MAX;
> > +}
> > +
> >   bool fuse_invalid_attr(struct fuse_attr *attr)
> >   {
> > -     return !fuse_valid_type(attr->mode) ||
> > -             attr->size > LLONG_MAX;
> > +     return !fuse_valid_type(attr->mode) || !fuse_valid_size(attr->size);
> >   }
> >
> >   int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name,
> > @@ -1143,6 +1147,84 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
> >       stat->blksize = 1 << blkbits;
> >   }
> >
> > +static void fuse_statx_to_attr(struct fuse_statx *sx, struct fuse_attr *attr)
> > +{
> > +     memset(attr, 0, sizeof(*attr));
> > +     attr->ino = sx->ino;
> > +     attr->size = sx->size;
> > +     attr->blocks = sx->blocks;
> > +     attr->atime = sx->atime.tv_sec;
> > +     attr->mtime = sx->mtime.tv_sec;
> > +     attr->ctime = sx->ctime.tv_sec;
> > +     attr->atimensec = sx->atime.tv_nsec;
> > +     attr->mtimensec = sx->mtime.tv_nsec;
> > +     attr->ctimensec = sx->ctime.tv_nsec;
> > +     attr->mode = sx->mode;
> > +     attr->nlink = sx->nlink;
> > +     attr->uid = sx->uid;
> > +     attr->gid = sx->gid;
> > +     attr->rdev = new_encode_dev(MKDEV(sx->rdev_major, sx->rdev_minor));
> > +     attr->blksize = sx->blksize;
> > +}
> > +
> > +static int fuse_do_statx(struct inode *inode, struct file *file,
> > +                      struct kstat *stat)
> > +{
> > +     int err;
> > +     struct fuse_attr attr;
> > +     struct fuse_statx *sx;
> > +     struct fuse_statx_in inarg;
> > +     struct fuse_statx_out outarg;
> > +     struct fuse_mount *fm = get_fuse_mount(inode);
> > +     u64 attr_version = fuse_get_attr_version(fm->fc);
> > +     FUSE_ARGS(args);
> > +
> > +     memset(&inarg, 0, sizeof(inarg));
> > +     memset(&outarg, 0, sizeof(outarg));
> > +     /* Directories have separate file-handle space */
> > +     if (file && S_ISREG(inode->i_mode)) {
> > +             struct fuse_file *ff = file->private_data;
> > +
> > +             inarg.getattr_flags |= FUSE_GETATTR_FH;
> > +             inarg.fh = ff->fh;
> > +     }
> > +     /* For now leave sync hints as the default, request all stats. */
> > +     inarg.sx_flags = 0;
> > +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
> > +     args.opcode = FUSE_STATX;
> > +     args.nodeid = get_node_id(inode);
> > +     args.in_numargs = 1;
> > +     args.in_args[0].size = sizeof(inarg);
> > +     args.in_args[0].value = &inarg;
> > +     args.out_numargs = 1;
> > +     args.out_args[0].size = sizeof(outarg);
> > +     args.out_args[0].value = &outarg;
> > +     err = fuse_simple_request(fm, &args);
> > +     if (err)
> > +             return err;
> > +
> > +     sx = &outarg.stat;
> > +     if (((sx->mask & STATX_SIZE) && !fuse_valid_size(sx->size)) ||
> > +         ((sx->mask & STATX_TYPE) && (!fuse_valid_type(sx->mode) ||
> > +                                      inode_wrong_type(inode, sx->mode)))) {
> > +             make_bad_inode(inode);
> > +             return -EIO;
> > +     }
> > +
> > +     fuse_statx_to_attr(&outarg.stat, &attr);
> > +     if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
> > +             fuse_change_attributes(inode, &attr, ATTR_TIMEOUT(&outarg),
> > +                                    attr_version);
> > +     }
> > +     stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
> > +     stat->btime.tv_sec = sx->btime.tv_sec;
> > +     stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
> > +     fuse_fillattr(inode, &attr, stat);
> > +     stat->result_mask |= STATX_TYPE;
> > +
> > +     return 0;
> > +}
>
>
> Hmm, unconditionally using stat is potentially a NULL ptr with future
> updates. I think not right now, as fuse_update_get_attr() has the
> (request_mask & ~STATX_BASIC_STATS) condition and no caller
> that passes 'stat = NULL' requests anything beyond STATX_BASIC_STATS,
> but wouldn't it be more safe to access stat only conditionally?

Yes, makes sense.

Thanks,
Miklos