On Wed, 23 Aug 2023 at 16:51, Bernd Schubert <bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 8/23/23 08:18, Miklos Szeredi wrote:
> > On Tue, 22 Aug 2023 at 18:55, Bernd Schubert <bernd.schubert@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 8/22/23 17:33, Miklos Szeredi wrote:
> >>> On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@xxxxxxxxxxx> wrote:
> >>>>
> >>>> Hi Miklos,
> >>>>
> >>>> sorry for late review.
> >>>>
> >>>> On 8/10/23 12:55, Miklos Szeredi wrote:
> >>>> [...]
> >>>>> +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;
> >>>>
> >>>>
> >>>>
> >>>> What is actually the reason not to pass through flags from
> >>>> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
> >>>> required mask and then server side can decide if it wants to fill in more?
> >>>
> >>> This and following commit is about btime and btime only. It's about
> >>> adding just this single attribute, otherwise the logic is unchanged.
> >>>
> >>> But the flexibility is there in the interface definition, and
> >>> functionality can be added later.
> >>
> >> Sure, though what speaks against setting (limiting the mask) right away?
> >
> > But then the result is basically uncacheable, until we have separate
> > validity timeouts for each attribute. Maybe we need that, maybe not,
> > but it does definitely have side effects.
>
> Ah right, updating the cache timeout shouldn't be done unless the reply
> contains all attributes. Although you already handle that in fuse_do_statx
Yes, correctness is guaranteed.
However not setting the full mask might easily result in a performance
regression. At this point just avoid such issues by not allowing
partial masks to reach the server.
Thanks,
Miklos
>
>
> if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
> fuse_change_attributes(inode, &attr, &outarg.stat,
> ATTR_TIMEOUT(&outarg), attr_version);
> }
>
>
>
> Thanks,
> Bernd
