* statx(2) API and documentation @ 2018-10-17 18:24 Miklos Szeredi 2018-10-17 18:45 ` Andreas Dilger 2018-10-18 16:04 ` David Howells 0 siblings, 2 replies; 14+ messages in thread From: Miklos Szeredi @ 2018-10-17 18:24 UTC (permalink / raw) To: Michael Kerrisk, David Howells; +Cc: linux-fsdevel, linux-kernel, Linux API 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 18:24 statx(2) API and documentation Miklos Szeredi @ 2018-10-17 18:45 ` Andreas Dilger 2018-10-17 19:04 ` Miklos Szeredi 2018-10-18 16:04 ` David Howells 1 sibling, 1 reply; 14+ messages in thread From: Andreas Dilger @ 2018-10-17 18:45 UTC (permalink / raw) To: Miklos Szeredi Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 18:45 ` Andreas Dilger @ 2018-10-17 19:04 ` Miklos Szeredi 2018-10-17 20:22 ` Andreas Dilger 2018-10-17 22:15 ` Amir Goldstein 0 siblings, 2 replies; 14+ messages in thread From: Miklos Szeredi @ 2018-10-17 19:04 UTC (permalink / raw) To: Andreas Dilger Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 19:04 ` Miklos Szeredi @ 2018-10-17 20:22 ` Andreas Dilger 2018-10-17 22:22 ` Florian Weimer 2018-10-18 7:23 ` Miklos Szeredi 2018-10-17 22:15 ` Amir Goldstein 1 sibling, 2 replies; 14+ messages in thread From: Andreas Dilger @ 2018-10-17 20:22 UTC (permalink / raw) To: Miklos Szeredi Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 20:22 ` Andreas Dilger @ 2018-10-17 22:22 ` Florian Weimer 2018-10-18 7:37 ` Miklos Szeredi 2018-10-18 7:23 ` Miklos Szeredi 1 sibling, 1 reply; 14+ messages in thread From: Florian Weimer @ 2018-10-17 22:22 UTC (permalink / raw) To: Andreas Dilger Cc: Miklos Szeredi, Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API * 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 22:22 ` Florian Weimer @ 2018-10-18 7:37 ` Miklos Szeredi 2018-10-18 7:39 ` Florian Weimer 0 siblings, 1 reply; 14+ messages in thread From: Miklos Szeredi @ 2018-10-18 7:37 UTC (permalink / raw) To: Florian Weimer Cc: Andreas Dilger, Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-18 7:37 ` Miklos Szeredi @ 2018-10-18 7:39 ` Florian Weimer 2018-10-18 7:42 ` Miklos Szeredi 0 siblings, 1 reply; 14+ messages in thread From: Florian Weimer @ 2018-10-18 7:39 UTC (permalink / raw) To: Miklos Szeredi Cc: Andreas Dilger, Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API * 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-18 7:39 ` Florian Weimer @ 2018-10-18 7:42 ` Miklos Szeredi 0 siblings, 0 replies; 14+ messages in thread From: Miklos Szeredi @ 2018-10-18 7:42 UTC (permalink / raw) To: Florian Weimer Cc: Andreas Dilger, Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 20:22 ` Andreas Dilger 2018-10-17 22:22 ` Florian Weimer @ 2018-10-18 7:23 ` Miklos Szeredi 1 sibling, 0 replies; 14+ messages in thread From: Miklos Szeredi @ 2018-10-18 7:23 UTC (permalink / raw) To: Andreas Dilger Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 19:04 ` Miklos Szeredi 2018-10-17 20:22 ` Andreas Dilger @ 2018-10-17 22:15 ` Amir Goldstein 2018-10-18 7:41 ` Jan Kara 1 sibling, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2018-10-17 22:15 UTC (permalink / raw) To: Miklos Szeredi Cc: Andreas Dilger, Michael Kerrisk (man-pages), David Howells, linux-fsdevel, linux-kernel, linux-api, Jan Kara 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 22:15 ` Amir Goldstein @ 2018-10-18 7:41 ` Jan Kara 2018-10-18 7:49 ` Florian Weimer 0 siblings, 1 reply; 14+ messages in thread From: Jan Kara @ 2018-10-18 7:41 UTC (permalink / raw) To: Amir Goldstein Cc: Miklos Szeredi, Andreas Dilger, Michael Kerrisk (man-pages), David Howells, linux-fsdevel, linux-kernel, linux-api, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-18 7:41 ` Jan Kara @ 2018-10-18 7:49 ` Florian Weimer 0 siblings, 0 replies; 14+ messages in thread From: Florian Weimer @ 2018-10-18 7:49 UTC (permalink / raw) To: Jan Kara Cc: Amir Goldstein, Miklos Szeredi, Andreas Dilger, Michael Kerrisk (man-pages), David Howells, linux-fsdevel, linux-kernel, linux-api * 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-17 18:24 statx(2) API and documentation Miklos Szeredi 2018-10-17 18:45 ` Andreas Dilger @ 2018-10-18 16:04 ` David Howells 2018-10-18 20:21 ` Miklos Szeredi 1 sibling, 1 reply; 14+ messages in thread From: David Howells @ 2018-10-18 16:04 UTC (permalink / raw) To: Miklos Szeredi Cc: dhowells, Michael Kerrisk, linux-fsdevel, linux-kernel, Linux API 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: statx(2) API and documentation 2018-10-18 16:04 ` David Howells @ 2018-10-18 20:21 ` Miklos Szeredi 0 siblings, 0 replies; 14+ messages in thread From: Miklos Szeredi @ 2018-10-18 20:21 UTC (permalink / raw) To: David Howells; +Cc: Michael Kerrisk, linux-fsdevel, linux-kernel, Linux API 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-10-18 20:21 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-17 18:24 statx(2) API and documentation Miklos Szeredi 2018-10-17 18:45 ` Andreas Dilger 2018-10-17 19:04 ` Miklos Szeredi 2018-10-17 20:22 ` Andreas Dilger 2018-10-17 22:22 ` Florian Weimer 2018-10-18 7:37 ` Miklos Szeredi 2018-10-18 7:39 ` Florian Weimer 2018-10-18 7:42 ` Miklos Szeredi 2018-10-18 7:23 ` Miklos Szeredi 2018-10-17 22:15 ` Amir Goldstein 2018-10-18 7:41 ` Jan Kara 2018-10-18 7:49 ` Florian Weimer 2018-10-18 16:04 ` David Howells 2018-10-18 20:21 ` Miklos Szeredi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).