statx(2) API and documentation

statx(2) API and documentation

[Miklos Szeredi]

I'm trying to implement statx for fuse and ran into the following issues:

- Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
for stx_attribute; otherwise if querying has non-zero cost, then
filesystem cannot do it without regressing performance.

- STATX_ALL definition is unclear, can this change, or is it fixed?
If it's the former, than that's a backward compatibility nightmare.
If it's the latter, then what's the point?

- STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
also cleared on MNT_NOATIME, but not on MNT_RDONLY.  We need some sort
of guideline in the documentation about what constitutes
"unsupported": does atime become unsupported because filesystem is
remounted r/o?  If so, why isn't this case handled consistently in the
VFS and filesystems?

- What about fields that are not cached when statx() is called with
AT_STATX_DONT_SYNC?  E.g. stx_btime is supported by the filesystem,
but getting it requires a roundtrip to the server.  Requesting
STATX_BTIME in the mask and adding  AT_STATX_DONT_SYNC to the flags
means the filesystem has to decide which it will honor.   My feeling
is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
stx_mask.   Documentation has no word about this case.

Thanks,
Miklos

Re: statx(2) API and documentation

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2372 bytes --]

On Oct 17, 2018, at 12:24 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> I'm trying to implement statx for fuse and ran into the following issues:
> 
> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
> for stx_attribute; otherwise if querying has non-zero cost, then
> filesystem cannot do it without regressing performance.

Seems reasonable.

> - STATX_ALL definition is unclear, can this change, or is it fixed?
> If it's the former, than that's a backward compatibility nightmare.
> If it's the latter, then what's the point?

The value can change over time.  It is intended to reflect the current
state of affairs at the time the userspace program and kernel are compiled.
The value sent from userspace lets the kernel know what fields are in
the userspace struct, so it doesn't try to set fields that aren't there.
The value in the kernel allows masking off new fields from userspace that
it doesn't understand.

> - STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
> also cleared on MNT_NOATIME, but not on MNT_RDONLY.  We need some sort
> of guideline in the documentation about what constitutes
> "unsupported": does atime become unsupported because filesystem is
> remounted r/o?  If so, why isn't this case handled consistently in the
> VFS and filesystems?

Strange.  I'd think that if userspace is requesting atime, it should
get an atime value.  The fact that the kernel is not updating atime
due to mount options just means that atime might be old.  That doesn't
mean (IMHO) that atime doesn't exist.

> - What about fields that are not cached when statx() is called with
> AT_STATX_DONT_SYNC?  E.g. stx_btime is supported by the filesystem,
> but getting it requires a roundtrip to the server.  Requesting
> STATX_BTIME in the mask and adding  AT_STATX_DONT_SYNC to the flags
> means the filesystem has to decide which it will honor.   My feeling
> is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
> stx_mask.   Documentation has no word about this case.

The btime value shouldn't change over the lifetime of a file, so
DONT_SYNC shouldn't have any effect on its validity?

I think DONT_SYNC applies to values like size, blocks, mtime, ctime
that may change rapidly over the lifetime of the file, so a totally
up-to-date value is not needed from the server in that case.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: statx(2) API and documentation

[Miklos Szeredi]

On Wed, Oct 17, 2018 at 8:45 PM, Andreas Dilger <adilger@dilger.ca> wrote:
> On Oct 17, 2018, at 12:24 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> I'm trying to implement statx for fuse and ran into the following issues:
>>
>> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
>> for stx_attribute; otherwise if querying has non-zero cost, then
>> filesystem cannot do it without regressing performance.
>
> Seems reasonable.
>
>> - STATX_ALL definition is unclear, can this change, or is it fixed?
>> If it's the former, than that's a backward compatibility nightmare.
>> If it's the latter, then what's the point?
>
> The value can change over time.  It is intended to reflect the current
> state of affairs at the time the userspace program and kernel are compiled.
> The value sent from userspace lets the kernel know what fields are in
> the userspace struct, so it doesn't try to set fields that aren't there.

What's the point of a userspace program specifying STATX_ALL?  Without
a way to programmatically query the interface definition it's useless:
there's no way to guess which mask bit corresponds to which field, and
what that field represents.

And there will be programs out there which specify STATX_ALL without
considering that in the future it may become slower as it is now due
to a recompile.

So what's the point exactly?

> The value in the kernel allows masking off new fields from userspace that
> it doesn't understand.

Okay, but that has nothing to do with the UAPI.  Even as an internal
flag we should be careful, as it might grow uses which can have
similar issues as the userspace one above.

>
>> - STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
>> also cleared on MNT_NOATIME, but not on MNT_RDONLY.  We need some sort
>> of guideline in the documentation about what constitutes
>> "unsupported": does atime become unsupported because filesystem is
>> remounted r/o?  If so, why isn't this case handled consistently in the
>> VFS and filesystems?
>
> Strange.  I'd think that if userspace is requesting atime, it should
> get an atime value.  The fact that the kernel is not updating atime
> due to mount options just means that atime might be old.  That doesn't
> mean (IMHO) that atime doesn't exist.

Right, OTOH I sort of see the value in  NFS: no roundtrip to server if
atime value is useless anyway.

>> - What about fields that are not cached when statx() is called with
>> AT_STATX_DONT_SYNC?  E.g. stx_btime is supported by the filesystem,
>> but getting it requires a roundtrip to the server.  Requesting
>> STATX_BTIME in the mask and adding  AT_STATX_DONT_SYNC to the flags
>> means the filesystem has to decide which it will honor.   My feeling
>> is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
>> stx_mask.   Documentation has no word about this case.
>
> The btime value shouldn't change over the lifetime of a file, so
> DONT_SYNC shouldn't have any effect on its validity?

Not validity, but presence in the cache.  Network filesystem or fuse
may or may not have it available in the cache at the time the inode is
first initialized on loookup.  So when statx(..., AT_STATX_DONT_SYNC,
STATX_BTIME) comes along, it may need a roundtrip to the server.  What
should it do?

Thanks,
Miklos

Re: statx(2) API and documentation

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 5096 bytes --]

On Oct 17, 2018, at 1:04 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> On Wed, Oct 17, 2018 at 8:45 PM, Andreas Dilger <adilger@dilger.ca> wrote:
>> On Oct 17, 2018, at 12:24 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> 
>>> I'm trying to implement statx for fuse and ran into the following issues:
>>> 
>>> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
>>> for stx_attribute; otherwise if querying has non-zero cost, then
>>> filesystem cannot do it without regressing performance.
>> 
>> Seems reasonable.
>> 
>>> - STATX_ALL definition is unclear, can this change, or is it fixed?
>>> If it's the former, than that's a backward compatibility nightmare.
>>> If it's the latter, then what's the point?
>> 
>> The value can change over time.  It is intended to reflect the current
>> state of affairs at the time the userspace program and kernel are compiled.
>> The value sent from userspace lets the kernel know what fields are in
>> the userspace struct, so it doesn't try to set fields that aren't there.
> 
> What's the point of a userspace program specifying STATX_ALL?  Without
> a way to programmatically query the interface definition it's useless:
> there's no way to guess which mask bit corresponds to which field, and
> what that field represents.
> 
> And there will be programs out there which specify STATX_ALL without
> considering that in the future it may become slower as it is now due
> to a recompile.

The whole point of statx is that applications should only be requesting
fields that they care about, so "ls --color=never" shouldn't be asking for
the file mode/size/etc., and size/blocks should only be requested with
"ls -ls", etc.

> So what's the point exactly?

Ah, I see your point...  STATX_ALL seems to be mostly useful for the kernel
to mask off flags that it doesn't currently understand.  It doesn't make
much sense for applications to specify STATX_ALL, since they don't have any
way to know what each flag means unless they are hard-coded to check each of
the STATX_* flags individually.  They should build up a mask of STATX_* flags
based on what they care about (e.g. "find" should only request attributes
based on the command-line options given).

>> The value in the kernel allows masking off new fields from userspace that
>> it doesn't understand.
> 
> Okay, but that has nothing to do with the UAPI.  Even as an internal
> flag we should be careful, as it might grow uses which can have
> similar issues as the userspace one above.
> 
>>> - STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
>>> also cleared on MNT_NOATIME, but not on MNT_RDONLY.  We need some sort
>>> of guideline in the documentation about what constitutes
>>> "unsupported": does atime become unsupported because filesystem is
>>> remounted r/o?  If so, why isn't this case handled consistently in the
>>> VFS and filesystems?
>> 
>> Strange.  I'd think that if userspace is requesting atime, it should
>> get an atime value.  The fact that the kernel is not updating atime
>> due to mount options just means that atime might be old.  That doesn't
>> mean (IMHO) that atime doesn't exist.
> 
> Right, OTOH I sort of see the value in  NFS: no roundtrip to server if
> atime value is useless anyway.

Well, "noatime" might be a client-only option, which doesn't mean that the
server or some other client isn't updating the atime.  Returning an old
atime isn't the same as not returning anything at all.

>>> - What about fields that are not cached when statx() is called with
>>> AT_STATX_DONT_SYNC?  E.g. stx_btime is supported by the filesystem,
>>> but getting it requires a roundtrip to the server.  Requesting
>>> STATX_BTIME in the mask and adding  AT_STATX_DONT_SYNC to the flags
>>> means the filesystem has to decide which it will honor.   My feeling
>>> is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
>>> stx_mask.   Documentation has no word about this case.
>> 
>> The btime value shouldn't change over the lifetime of a file, so
>> DONT_SYNC shouldn't have any effect on its validity?
> 
> Not validity, but presence in the cache.  Network filesystem or fuse
> may or may not have it available in the cache at the time the inode is
> first initialized on loookup.  So when statx(..., AT_STATX_DONT_SYNC,
> STATX_BTIME) comes along, it may need a roundtrip to the server.  What
> should it do?

To my thinking, if the user/app requests btime/ctime/size/blocks the kernel
should return these fields.  If DONT_SYNC is set, it means the kernel can
return potentially stale values, but if the kernel doesn't have *any*
values for these fields at all it still should query the server instead of
just returning random garbage.

That said, it seems that it isn't even possible to get into nfs_getattr()
without the VFS having instantiated a dentry and inode (i.e. queried the
server via nfs_lookup() at some point), so the inode fields have already
been filled with *something*.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: statx(2) API and documentation

[Amir Goldstein]

On Wed, Oct 17, 2018 at 10:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:

> >> - STATX_ALL definition is unclear, can this change, or is it fixed?
> >> If it's the former, than that's a backward compatibility nightmare.
> >> If it's the latter, then what's the point?
> >
> > The value can change over time.  It is intended to reflect the current
> > state of affairs at the time the userspace program and kernel are compiled.
> > The value sent from userspace lets the kernel know what fields are in
> > the userspace struct, so it doesn't try to set fields that aren't there.
>
> What's the point of a userspace program specifying STATX_ALL?  Without
> a way to programmatically query the interface definition it's useless:
> there's no way to guess which mask bit corresponds to which field, and
> what that field represents.
>
> And there will be programs out there which specify STATX_ALL without
> considering that in the future it may become slower as it is now due
> to a recompile.
>
> So what's the point exactly?
>
> > The value in the kernel allows masking off new fields from userspace that
> > it doesn't understand.
>
> Okay, but that has nothing to do with the UAPI.  Even as an internal
> flag we should be careful, as it might grow uses which can have
> similar issues as the userspace one above.
>

FYI, I identified a similar anti-pattern in fanotify UAPI when I wanted to
add new flags and did not want to change the UAPI _ALL_ constants.
This is how we plan to solve it:
https://github.com/amir73il/linux/commit/8c2b1acadb88ee4505ccc8bfdc665863111fb4cc

Thanks,
Amir.

Re: statx(2) API and documentation

[Florian Weimer]

* Andreas Dilger:

>> So what's the point exactly?
>
> Ah, I see your point...  STATX_ALL seems to be mostly useful for the kernel
> to mask off flags that it doesn't currently understand.  It doesn't make
> much sense for applications to specify STATX_ALL, since they don't have any
> way to know what each flag means unless they are hard-coded to check each of
> the STATX_* flags individually.  They should build up a mask of STATX_* flags
> based on what they care about (e.g. "find" should only request attributes
> based on the command-line options given).

Could you remove it from the UAPI header?  I didn't want to put it
into the glibc header, but was overruled.

Re: statx(2) API and documentation

[Miklos Szeredi]

On Wed, Oct 17, 2018 at 10:22 PM, Andreas Dilger <adilger@dilger.ca> wrote:
> On Oct 17, 2018, at 1:04 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:

>> So what's the point exactly?
>
> Ah, I see your point...  STATX_ALL seems to be mostly useful for the kernel
> to mask off flags that it doesn't currently understand.

And even there it's quite pointless, since no sane implementation
would put the request mask into the result mask (aka. stx_mask)
verbatim, without picking out bits which correspond to *supported*
fields.

So it's pointless across the board.  But since it's already published
in a userspace API, the best we can do is leave it at its current
value, but with a comment to warn that it  should not be updated when
new flags are added.  And remove all traces of it from the kernel.

>> Right, OTOH I sort of see the value in  NFS: no roundtrip to server if
>> atime value is useless anyway.
>
> Well, "noatime" might be a client-only option, which doesn't mean that the
> server or some other client isn't updating the atime.  Returning an old
> atime isn't the same as not returning anything at all.

Fs is allowed (and in case of basic stats, required) to fill in dummy
values for even unsupported fields.

I understand your argument, since that was my first thought, but I
also understand the reason behind clearing STATX_ATIME.  And atime is
something that very few applications care about anyway.   But I do
think such behavior should be clarified in the documentation and
handled more consistently in the code.

> To my thinking, if the user/app requests btime/ctime/size/blocks the kernel
> should return these fields.  If DONT_SYNC is set, it means the kernel can
> return potentially stale values, but if the kernel doesn't have *any*
> values for these fields at all it still should query the server instead of
> just returning random garbage.

I'm not saying it should return garbage, instead it can just not set
the bit in the result mask to indicate that that field cannot be
filled (without network traffic, in this case).

This requires generalizing the definition of the result mask, because
the above (clearing STATX_ATIME on ro/noatim or clearing STATX_BTIME
on DONT_SYNC) don't fit the current "unsupported or unrepresentable"
definition.

> That said, it seems that it isn't even possible to get into nfs_getattr()
> without the VFS having instantiated a dentry and inode (i.e. queried the
> server via nfs_lookup() at some point), so the inode fields have already
> been filled with *something*.

There's no "i_btime" field.

But it's hard to argue this point with btime, so lets instead think
about a theoretical attribute that is:

 - hard to calculate, so filesystem will not want to cache it on inode
initialization (because it's very unlikely to be actually needed)

 - isn't constant and can be cached, so all sync types make sense.

I think the following semantics would be the most useful and sane:
FORCE_SYNC or SYNC_AS_STAT would always retrieve the attribute if
supported, and set the bit in the result mask; DONT_SYNC would only
set the bit in the result mask if the attribute is already cached.

Thanks,
Miklos

Re: statx(2) API and documentation

[Miklos Szeredi]

On Thu, Oct 18, 2018 at 12:22 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Andreas Dilger:
>
>>> So what's the point exactly?
>>
>> Ah, I see your point...  STATX_ALL seems to be mostly useful for the kernel
>> to mask off flags that it doesn't currently understand.  It doesn't make
>> much sense for applications to specify STATX_ALL, since they don't have any
>> way to know what each flag means unless they are hard-coded to check each of
>> the STATX_* flags individually.  They should build up a mask of STATX_* flags
>> based on what they care about (e.g. "find" should only request attributes
>> based on the command-line options given).
>
> Could you remove it from the UAPI header?  I didn't want to put it
> into the glibc header, but was overruled.

To summarize Linus' rule of backward incompatibility: you can do it as
long as nobody notices.  So yeah, we could try removing STATX_ALL from
the uapi header, but we'd have to put it back in, once somebody
complains.

Thanks,
Miklos

Re: statx(2) API and documentation

[Florian Weimer]

* Miklos Szeredi:

> On Thu, Oct 18, 2018 at 12:22 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * Andreas Dilger:
>>
>>>> So what's the point exactly?
>>>
>>> Ah, I see your point...  STATX_ALL seems to be mostly useful for the kernel
>>> to mask off flags that it doesn't currently understand.  It doesn't make
>>> much sense for applications to specify STATX_ALL, since they don't have any
>>> way to know what each flag means unless they are hard-coded to check each of
>>> the STATX_* flags individually.  They should build up a mask of STATX_* flags
>>> based on what they care about (e.g. "find" should only request attributes
>>> based on the command-line options given).
>>
>> Could you remove it from the UAPI header?  I didn't want to put it
>> into the glibc header, but was overruled.
>
> To summarize Linus' rule of backward incompatibility: you can do it as
> long as nobody notices.  So yeah, we could try removing STATX_ALL from
> the uapi header, but we'd have to put it back in, once somebody
> complains.

I don't recall a rule about backwards-incompatible API changes.  This
wouldn't impact ABI at all.

Re: statx(2) API and documentation

[Jan Kara]

On Thu 18-10-18 01:15:13, Amir Goldstein wrote:
> On Wed, Oct 17, 2018 at 10:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> > >> - STATX_ALL definition is unclear, can this change, or is it fixed?
> > >> If it's the former, than that's a backward compatibility nightmare.
> > >> If it's the latter, then what's the point?
> > >
> > > The value can change over time.  It is intended to reflect the current
> > > state of affairs at the time the userspace program and kernel are compiled.
> > > The value sent from userspace lets the kernel know what fields are in
> > > the userspace struct, so it doesn't try to set fields that aren't there.
> >
> > What's the point of a userspace program specifying STATX_ALL?  Without
> > a way to programmatically query the interface definition it's useless:
> > there's no way to guess which mask bit corresponds to which field, and
> > what that field represents.
> >
> > And there will be programs out there which specify STATX_ALL without
> > considering that in the future it may become slower as it is now due
> > to a recompile.
> >
> > So what's the point exactly?
> >
> > > The value in the kernel allows masking off new fields from userspace that
> > > it doesn't understand.
> >
> > Okay, but that has nothing to do with the UAPI.  Even as an internal
> > flag we should be careful, as it might grow uses which can have
> > similar issues as the userspace one above.
> >
> 
> FYI, I identified a similar anti-pattern in fanotify UAPI when I wanted to
> add new flags and did not want to change the UAPI _ALL_ constants.
> This is how we plan to solve it:
> https://github.com/amir73il/linux/commit/8c2b1acadb88ee4505ccc8bfdc665863111fb4cc

Yeah, after fanotify experience I find foo_ALL constants useless if not
dangerous for userspace.  Kernel internal constants like this are IMO
useful.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: statx(2) API and documentation

[Miklos Szeredi]

On Thu, Oct 18, 2018 at 9:39 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Miklos Szeredi:
>
>> On Thu, Oct 18, 2018 at 12:22 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * Andreas Dilger:
>>>
>>>>> So what's the point exactly?
>>>>
>>>> Ah, I see your point...  STATX_ALL seems to be mostly useful for the kernel
>>>> to mask off flags that it doesn't currently understand.  It doesn't make
>>>> much sense for applications to specify STATX_ALL, since they don't have any
>>>> way to know what each flag means unless they are hard-coded to check each of
>>>> the STATX_* flags individually.  They should build up a mask of STATX_* flags
>>>> based on what they care about (e.g. "find" should only request attributes
>>>> based on the command-line options given).
>>>
>>> Could you remove it from the UAPI header?  I didn't want to put it
>>> into the glibc header, but was overruled.
>>
>> To summarize Linus' rule of backward incompatibility: you can do it as
>> long as nobody notices.  So yeah, we could try removing STATX_ALL from
>> the uapi header, but we'd have to put it back in, once somebody
>> complains.
>
> I don't recall a rule about backwards-incompatible API changes.  This
> wouldn't impact ABI at all.

Right, API rules maybe are softer.   I'll do some patches...

Thanks,
Miklos

Re: statx(2) API and documentation

[Florian Weimer]

* Jan Kara:

> On Thu 18-10-18 01:15:13, Amir Goldstein wrote:
>> FYI, I identified a similar anti-pattern in fanotify UAPI when I wanted to
>> add new flags and did not want to change the UAPI _ALL_ constants.
>> This is how we plan to solve it:
>> https://github.com/amir73il/linux/commit/8c2b1acadb88ee4505ccc8bfdc665863111fb4cc
>
> Yeah, after fanotify experience I find foo_ALL constants useless if not
> dangerous for userspace.  Kernel internal constants like this are IMO
> useful.

There are also various *_MAX constants which are increased regularly.
Some of them are part of the UAPI headers (INET_DIAG_MAX appears to be
a relevant example), some are no longer in UAPI, but still in custom
glibc headers (AF_MAX/PF_MAX).  These appear to be equally useless.

Re: statx(2) API and documentation

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> I'm trying to implement statx for fuse and ran into the following issues:
> 
> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
> for stx_attribute; otherwise if querying has non-zero cost, then
> filesystem cannot do it without regressing performance.

Okay, though the way your patch implements it makes it superfluous.  I presume
you have further patches that will actually make use of it from the fuse side?

> - STATX_ALL definition is unclear, can this change, or is it fixed?
> If it's the former, than that's a backward compatibility nightmare.
> If it's the latter, then what's the point?

It's the set of supported attributes known by the headers, and such can
only be added to over time.  But yes, it's probably unnecessary.  Asking
fsinfo() will be a better way of doing things.

> - STATX_ATIME is cleared from stx_mask on SB_RDONLY,

Ummm...  Where?  It's cleared on IS_NOATIME() in generic_fillattr().  I made
the assumption that IS_NOATIME() == false indicates that there isn't an atime
to be had.

> and on NFS it is also cleared on MNT_NOATIME, but not on MNT_RDONLY.  We
> need some sort of guideline in the documentation about what constitutes
> "unsupported": does atime become unsupported because filesystem is remounted
> r/o?  If so, why isn't this case handled consistently in the VFS and
> filesystems?

STATX_ATIME should mean there is an actual atime from the "medium" in
stx_atime, rather than something made up by the filesystem driver; it doesn't
necessarily promise that this will be updated.

There can still be an atime if the medium is read-only.

atime is even more complicated with MNT_NOATIME or MNT_RDONLY because that
doesn't stop the atime from actually being updated through another mountpoint
on the same system.

Note that stx_atime should always contain something that can be used directly
to fill in st_atime if emulating stat() - even if STATX_ATIME is cleared.

> - What about fields that are not cached when statx() is called with
> AT_STATX_DONT_SYNC?  E.g. stx_btime is supported by the filesystem,
> but getting it requires a roundtrip to the server.

Not necessarily.  It's not cached in *struct inode*, but that doesn't mean
that the filesystem can't cache it elsewhere.

> Requesting STATX_BTIME in the mask and adding AT_STATX_DONT_SYNC to the
> flags means the filesystem has to decide which it will honor.  My feeling is
> that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in stx_mask.
> Documentation has no word about this case.

From the manpage:

       AT_STATX_DONT_SYNC
              Don't synchronize anything, but rather just  take  whatever  the
              system  has cached if possible. ...
 
Note the "if possible".  If it's not possible, you still need to go get it if
they explicitly asked for it.

David

Re: statx(2) API and documentation

[Miklos Szeredi]

On Thu, Oct 18, 2018 at 6:04 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
>> I'm trying to implement statx for fuse and ran into the following issues:
>>
>> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
>> for stx_attribute; otherwise if querying has non-zero cost, then
>> filesystem cannot do it without regressing performance.
>
> Okay, though the way your patch implements it makes it superfluous.  I presume
> you have further patches that will actually make use of it from the fuse side?

Being worked on, yes.

>
>> - STATX_ALL definition is unclear, can this change, or is it fixed?
>> If it's the former, than that's a backward compatibility nightmare.
>> If it's the latter, then what's the point?
>
> It's the set of supported attributes known by the headers, and such can
> only be added to over time.  But yes, it's probably unnecessary.  Asking
> fsinfo() will be a better way of doing things.
>
>> - STATX_ATIME is cleared from stx_mask on SB_RDONLY,
>
> Ummm...  Where?  It's cleared on IS_NOATIME() in generic_fillattr().  I made
> the assumption that IS_NOATIME() == false indicates that there isn't an atime
> to be had.

Look at IS_NOATIME definition in <linux/fs.h>

You probably wanted inode->i_sb->s_flags & SB_NOATIME instead.

>> and on NFS it is also cleared on MNT_NOATIME, but not on MNT_RDONLY.  We
>> need some sort of guideline in the documentation about what constitutes
>> "unsupported": does atime become unsupported because filesystem is remounted
>> r/o?  If so, why isn't this case handled consistently in the VFS and
>> filesystems?
>
> STATX_ATIME should mean there is an actual atime from the "medium" in
> stx_atime, rather than something made up by the filesystem driver; it doesn't
> necessarily promise that this will be updated.

In this case generic_fillattr() and nfs_getattr() are simply buggy.

>
> There can still be an atime if the medium is read-only.
>
> atime is even more complicated with MNT_NOATIME or MNT_RDONLY because that
> doesn't stop the atime from actually being updated through another mountpoint
> on the same system.
>
> Note that stx_atime should always contain something that can be used directly
> to fill in st_atime if emulating stat() - even if STATX_ATIME is cleared.
>
>> - What about fields that are not cached when statx() is called with
>> AT_STATX_DONT_SYNC?  E.g. stx_btime is supported by the filesystem,
>> but getting it requires a roundtrip to the server.
>
> Not necessarily.  It's not cached in *struct inode*, but that doesn't mean
> that the filesystem can't cache it elsewhere.
>
>> Requesting STATX_BTIME in the mask and adding AT_STATX_DONT_SYNC to the
>> flags means the filesystem has to decide which it will honor.  My feeling is
>> that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in stx_mask.
>> Documentation has no word about this case.
>
> From the manpage:
>
>        AT_STATX_DONT_SYNC
>               Don't synchronize anything, but rather just  take  whatever  the
>               system  has cached if possible. ...
>
> Note the "if possible".  If it's not possible, you still need to go get it if
> they explicitly asked for it.

Okay.

Thanks,
Miklos