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

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 8/23/23 16:58, Miklos Szeredi wrote:
> On Wed, 23 Aug 2023 at 16:51, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>>
>>
>> On 8/23/23 08:18, Miklos Szeredi wrote:
>>> On Tue, 22 Aug 2023 at 18:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>>>
>>>>
>>>>
>>>> On 8/22/23 17:33, Miklos Szeredi wrote:
>>>>> On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> 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.


Hmm ok, I see a bit more like "these flags are absolutely needed, if you 
(daemon/server) can provide more/all, I can update my cache timeout".


Thanks,
Bernd