Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1391088 > unrolled thread

[RFC][PATCH 0/6] Enhanced file stat system call

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/6] Enhanced file stat system call David Howells <dhowells@redhat.com> - 2016-04-29 15:00 +0200
    [PATCH 4/6] statx: NFS: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-04-29 15:00 +0200
      Re: [PATCH 4/6] statx: NFS: Return enhanced file attributes Andreas Dilger <adilger@dilger.ca> - 2016-05-03 00:50 +0200
    [PATCH 6/6] statx: CIFS: Return enhanced attributes David Howells <dhowells@redhat.com> - 2016-04-29 15:00 +0200
    [PATCH 3/6] statx: Ext4: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-04-29 15:00 +0200
      Re: [PATCH 3/6] statx: Ext4: Return enhanced file attributes Andreas Dilger <adilger@dilger.ca> - 2016-05-03 00:50 +0200
        Re: [PATCH 3/6] statx: Ext4: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-05-03 22:30 +0200
      Re: [PATCH 3/6] statx: Ext4: Return enhanced file attributes Christoph Hellwig <hch@infradead.org> - 2016-05-08 10:40 +0200
    [PATCH 5/6] statx: Make windows attributes available for CIFS,  NTFS and FAT to use David Howells <dhowells@redhat.com> - 2016-04-29 15:10 +0200
      Re: [PATCH 5/6] statx: Make windows attributes available for CIFS, NTFS and FAT to use Andreas Dilger <adilger@dilger.ca> - 2016-05-03 01:00 +0200
        Re: [PATCH 5/6] statx: Make windows attributes available for CIFS, NTFS and FAT to use David Howells <dhowells@redhat.com> - 2016-05-03 22:30 +0200
      Re: [PATCH 5/6] statx: Make windows attributes available for CIFS,  NTFS and FAT to use Christoph Hellwig <hch@infradead.org> - 2016-05-08 10:40 +0200
    [PATCH 2/6] statx: AFS: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-04-29 15:20 +0200
    Re: [RFC][PATCH 0/6] Enhanced file stat system call Jeff Layton <jlayton@poochiereds.net> - 2016-04-30 23:10 +0200
    Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-03 18:00 +0200
    Re: [RFC][PATCH 0/6] Enhanced file stat system call Arnd Bergmann <arnd@arndb.de> - 2016-05-04 15:50 +0200
      Re: [RFC][PATCH 0/6] Enhanced file stat system call Steve French <smfrench@gmail.com> - 2016-05-06 04:10 +0200
        Re: [RFC][PATCH 0/6] Enhanced file stat system call Arnd Bergmann <arnd@arndb.de> - 2016-05-09 15:10 +0200
    Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-05-05 01:00 +0200
      Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available NeilBrown <nfbrown@novell.com> - 2016-05-05 02:20 +0200
        Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Jeff Layton <jlayton@poochiereds.net> - 2016-05-05 21:50 +0200
          Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-05 22:10 +0200
            Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-05-06 03:50 +0200
          Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available bfields@fieldses.org (J. Bruce Fields) - 2016-05-06 20:10 +0200
      Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available bfields@fieldses.org (J. Bruce Fields) - 2016-05-06 20:30 +0200
        Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-05-09 03:50 +0200
          Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available "J. Bruce Fields" <bfields@fieldses.org> - 2016-05-09 04:50 +0200
    Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available NeilBrown <nfbrown@novell.com> - 2016-05-05 02:00 +0200
    Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Christoph Hellwig <hch@infradead.org> - 2016-05-08 10:40 +0200
      Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Jeff Layton <jlayton@poochiereds.net> - 2016-05-09 14:10 +0200
        Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Christoph Hellwig <hch@infradead.org> - 2016-05-10 09:10 +0200
          Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Jeff Layton <jlayton@poochiereds.net> - 2016-05-10 15:30 +0200
      Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-09 15:00 +0200
        Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-09 15:10 +0200
        Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Trond Myklebust <trondmy@primarydata.com> - 2016-05-09 15:40 +0200
        Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Christoph Hellwig <hch@infradead.org> - 2016-05-10 09:10 +0200
          Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-10 10:30 +0200
            Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Christoph Hellwig <hch@infradead.org> - 2016-05-12 11:20 +0200
      Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-09 15:40 +0200
        Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Christoph Hellwig <hch@infradead.org> - 2016-05-10 09:10 +0200
          Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-10 10:50 +0200
            Re: [PATCH 1/6] statx: Add a system call to make enhanced file info  available Christoph Hellwig <hch@infradead.org> - 2016-05-12 11:20 +0200
      Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-05-09 15:50 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1395327 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Thu, 2016-05-05 at 10:09 +1000, NeilBrown wrote:
> On Thu, May 05 2016, Dave Chinner wrote:
> 
> > 
> > On Fri, Apr 29, 2016 at 01:57:43PM +0100, David Howells wrote:
> > > 
> > >  (4) File creation time (st_btime*), data version (st_version), inode
> > >      generation number (st_gen).
> > > 
> > >      These will be returned if available whether the caller asked for them or
> > >      not.  The corresponding bits in st_mask will be set or cleared as
> > >      appropriate to indicate a valid value.
> > IMO, exposing the inode generation number to anyone is a potential
> > security problem because they are used in file handles.
> "security through obscurity".  We have Kerberos working really nicely
> for NFS these days.  Do we still care?
> 
> What if the generation number were only made available to "root"?  Would
> that allay your concerns?
> Would that still be useful?
> We already have name_to_handle_at().  Exposing the generation number
> could/should follow the same rules at that.  Or maybe the exposure of
> each field should be guided by the filesystem, depending on (for
> example) whether it is used to provide uniqueness to the filehandle.
> 
> > 
> > 
> > > 
> > >      If the caller didn't ask for them, then they may be approximated.  For
> > >      example, NFS won't waste any time updating them from the server, unless
> > >      as a byproduct of updating something requested.
> > I would suggest that exposing them from the NFS server is something
> > we most definitely don't want to do because they are the only thing
> > that keeps remote users from guessing filehandles with ease....
> Given that the NFS protocol does not define a "generation number"
> attribute, I think there is no risk for them being exposed from the NFS
> server ... except implicitly within the filehandle of course.
> 
> NeilBrown



I don't see a real attack vector here either, but OTOH is there a
potential user of this at the moment? An earlier chunk of the patch
description says:

(7) Inode generation number: Useful for FUSE and userspace NFS servers
     [Bernd Schubert].  This was asked for but later deemed unnecessary
     with the open-by-handle capability available

...the last bit seems to indicate that we don't really need this
anyway, as most userland servers now work with filehandles from the
kernel.

Maybe leave it out for now? It can always be added later.

-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1395347 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

Jeff Layton <jlayton@poochiereds.net> wrote:

> I don't see a real attack vector here either, but OTOH is there a
> potential user of this at the moment?

I'm not sure.  BSD stat has an st_gen, so it's possible something out there
will use it if it exists.

> An earlier chunk of the patch description says:
> 
> (7) Inode generation number: Useful for FUSE and userspace NFS servers
>      [Bernd Schubert].  This was asked for but later deemed unnecessary
>      with the open-by-handle capability available
> 
> ...the last bit seems to indicate that we don't really need this
> anyway, as most userland servers now work with filehandles from the
> kernel.
> 
> Maybe leave it out for now? It can always be added later.

Yeah...  probably a good idea.

David

[toc] | [prev] | [next] | [standalone]

#1395573 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Thu, May 05, 2016 at 09:04:09PM +0100, David Howells wrote:
> Jeff Layton <jlayton@poochiereds.net> wrote:
> 
> > I don't see a real attack vector here either, but OTOH is there a
> > potential user of this at the moment?
> 
> I'm not sure.  BSD stat has an st_gen, so it's possible something out there
> will use it if it exists.

Oh, I know of several userspace applications that use the inode
generation number for some purpose. However, all of them are so
tightly tied to the XFS internal structure, implementation and the
XFS specific bulkstat interface that they cannot be considered
generic applications.

> > An earlier chunk of the patch description says:
> > 
> > (7) Inode generation number: Useful for FUSE and userspace NFS servers
> >      [Bernd Schubert].  This was asked for but later deemed unnecessary
> >      with the open-by-handle capability available
> > 
> > ...the last bit seems to indicate that we don't really need this
> > anyway, as most userland servers now work with filehandles from the
> > kernel.
> > 
> > Maybe leave it out for now? It can always be added later.
> 
> Yeah...  probably a good idea.

Fine by me.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1396020 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Thu, May 05, 2016 at 03:48:16PM -0400, Jeff Layton wrote:
> On Thu, 2016-05-05 at 10:09 +1000, NeilBrown wrote:
> > On Thu, May 05 2016, Dave Chinner wrote:
> > 
> > > 
> > > On Fri, Apr 29, 2016 at 01:57:43PM +0100, David Howells wrote:
> > > > 
> > > >  (4) File creation time (st_btime*), data version (st_version), inode
> > > >      generation number (st_gen).
> > > > 
> > > >      These will be returned if available whether the caller asked for them or
> > > >      not.  The corresponding bits in st_mask will be set or cleared as
> > > >      appropriate to indicate a valid value.
> > > IMO, exposing the inode generation number to anyone is a potential
> > > security problem because they are used in file handles.
> > "security through obscurity".  We have Kerberos working really nicely
> > for NFS these days.  Do we still care?
> > 
> > What if the generation number were only made available to "root"?  Would
> > that allay your concerns?
> > Would that still be useful?
> > We already have name_to_handle_at().  Exposing the generation number
> > could/should follow the same rules at that.  Or maybe the exposure of
> > each field should be guided by the filesystem, depending on (for
> > example) whether it is used to provide uniqueness to the filehandle.
> > 
> > > 
> > > 
> > > > 
> > > >      If the caller didn't ask for them, then they may be approximated.  For
> > > >      example, NFS won't waste any time updating them from the server, unless
> > > >      as a byproduct of updating something requested.
> > > I would suggest that exposing them from the NFS server is something
> > > we most definitely don't want to do because they are the only thing
> > > that keeps remote users from guessing filehandles with ease....
> > Given that the NFS protocol does not define a "generation number"
> > attribute, I think there is no risk for them being exposed from the NFS
> > server ... except implicitly within the filehandle of course.
> > 
> > NeilBrown
> 
> 
> 
> I don't see a real attack vector here either, but OTOH is there a
> potential user of this at the moment? An earlier chunk of the patch
> description says:
> 
> (7) Inode generation number: Useful for FUSE and userspace NFS servers
>      [Bernd Schubert].  This was asked for but later deemed unnecessary
>      with the open-by-handle capability available
> 
> ...the last bit seems to indicate that we don't really need this
> anyway, as most userland servers now work with filehandles from the
> kernel.
> 
> Maybe leave it out for now? It can always be added later.

Sounds like a good compromise to me!

That said, filehandles can never be changed, and generally have to be
exposed on the network, so I don't think it's worth going to great
lengths to try keep them secret.

--b.

[toc] | [prev] | [next] | [standalone]

#1396057 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Thu, May 05, 2016 at 08:56:02AM +1000, Dave Chinner wrote:
> IMO, exposing the inode generation number to anyone is a potential
> security problem because they are used in file handles.
> 
> Most file handles provided by filesystems are simply an encoding of
> the inode number + generation number, plus maybe the ino+gen of the
> parent dir if the NFS server is configured to do this. This makes it
> trivial for an attacker to guess what the likely generation numbers
> are going to be for inode numbers surrounding any given inode, hence
> greatly reducing the search space for guessing valid file handles.
> 
> We've known this to be a problem for a long time - file handles are
> not cryptographically secure,

I once thought signing filehandles cryptographically to prevent spoofing
would be interesting.  But once you've handed out a filehandle, it's
hard to keep it secret.  And the server's required to respect a file's
filehandle for the lifetime of the file, so it only has to leak once.
So I'm no longer convinced it's worth going to that kind of trouble.

Just choosing the generation number randomly, OK, maybe that's a
reasonable measure.

> so exposing information like this by
> default make guessing handles successfully almost trivial for many
> filesystems.
> 
> In the latest XFS filesystem format, we randomise the generation
> value during every inode allocation to make it hard to guess the
> handle of adjacent inodes from an existing ino+gen pair, or even
> from life time to life time of the same inode.

The one thing I wonder about is whether that increases the probability
of a filehandle collision (where you accidentally generate the same
filehandle for two different files).

If the generation number is a 32-bit counter per inode number (is that
actually the way filesystems work?), then it takes 2^32 reuses of the
inode number to hit the same filehandle.  If you choose it randomly then
you expect a collision after about 2^16 reuses.

I don't know, maybe this is still unlikely enough to be academic.

> We don't use a secure
> random number generator (prandom_u32()) so it's still possible to
> guess with enough trial and observation. However, it makes it
> several orders of magnitude harder to guess and requires knowledge
> of inode allocation order to guess correctly once the random number
> sequence has been deduced and so makes brute force the only real
> option for guessing a valid handle for an inode.
> 
> However, this is definitely a problem for the older format where
> each cluster of inodes was initialised with the same seed at cluster
> allocation time and the generation number was simply incremented for
> each life time. Most filesystems use a similar method for seeding
> and incrementing generation numbers, so once the generation numbers
> are exposed it makes handles trivial to calculate successfully.
> 
> >      If the caller didn't ask for them, then they may be approximated.  For
> >      example, NFS won't waste any time updating them from the server, unless
> >      as a byproduct of updating something requested.
> 
> I would suggest that exposing them from the NFS server is something
> we most definitely don't want to do because they are the only thing
> that keeps remote users from guessing filehandles with ease....

The first line of defense is not to depend on unguessable filehandles.
(Don't export sudirectories unless you're willing to export the whole
filesystem; and don't depend on directory permissions to keep children
secret.)

--b.

[toc] | [prev] | [next] | [standalone]

#1396537 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

[ OT, but I'll reply anyway :P ]

On Fri, May 06, 2016 at 02:29:23PM -0400, J. Bruce Fields wrote:
> On Thu, May 05, 2016 at 08:56:02AM +1000, Dave Chinner wrote:
> > In the latest XFS filesystem format, we randomise the generation
> > value during every inode allocation to make it hard to guess the
> > handle of adjacent inodes from an existing ino+gen pair, or even
> > from life time to life time of the same inode.
> 
> The one thing I wonder about is whether that increases the probability
> of a filehandle collision (where you accidentally generate the same
> filehandle for two different files).

Not possible - inode number is still different between the two
files. i.e. ino+gen makes the handle unique, not gen.

> If the generation number is a 32-bit counter per inode number (is that
> actually the way filesystems work?), then it takes 2^32 reuses of the
> inode number to hit the same filehandle.

4 billion unlink/create operations that hit the same inode number
are going to take some time. I suspect someone will notice the load
generated by an attmept to brute force this sort of thing ;)

> If you choose it randomly then
> you expect a collision after about 2^16 reuses.

I'm pretty sure that a random search will need to, on average,
search half the keyspace before a match is found (i.e. 2^31
attempts, not 2^16).

> > >      If the caller didn't ask for them, then they may be approximated.  For
> > >      example, NFS won't waste any time updating them from the server, unless
> > >      as a byproduct of updating something requested.
> > 
> > I would suggest that exposing them from the NFS server is something
> > we most definitely don't want to do because they are the only thing
> > that keeps remote users from guessing filehandles with ease....
> 
> The first line of defense is not to depend on unguessable filehandles.
> (Don't export sudirectories unless you're willing to export the whole
> filesystem; and don't depend on directory permissions to keep children
> secret.)

Defense in depth also says "don't make it easy to guess filehandles"
because not everyone knows this is a problem. In many cases, users
may not even know what consitutes a "filesystem" because their NFS
server appliance only defines "exports". The underlying
implementation may, in fact, be "everything exported from a single
filesystem" and so the user has no choice in the matter....


Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1396557 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Mon, May 09, 2016 at 11:45:43AM +1000, Dave Chinner wrote:
> [ OT, but I'll reply anyway :P ]
> 
> On Fri, May 06, 2016 at 02:29:23PM -0400, J. Bruce Fields wrote:
> > On Thu, May 05, 2016 at 08:56:02AM +1000, Dave Chinner wrote:
> > > In the latest XFS filesystem format, we randomise the generation
> > > value during every inode allocation to make it hard to guess the
> > > handle of adjacent inodes from an existing ino+gen pair, or even
> > > from life time to life time of the same inode.
> > 
> > The one thing I wonder about is whether that increases the probability
> > of a filehandle collision (where you accidentally generate the same
> > filehandle for two different files).
> 
> Not possible - inode number is still different between the two
> files. i.e. ino+gen makes the handle unique, not gen.
> 
> > If the generation number is a 32-bit counter per inode number (is that
> > actually the way filesystems work?), then it takes 2^32 reuses of the
> > inode number to hit the same filehandle.
> 
> 4 billion unlink/create operations that hit the same inode number
> are going to take some time. I suspect someone will notice the load
> generated by an attmept to brute force this sort of thing ;)
> 
> > If you choose it randomly then
> > you expect a collision after about 2^16 reuses.
> 
> I'm pretty sure that a random search will need to, on average,
> search half the keyspace before a match is found (i.e. 2^31
> attempts, not 2^16).

Yeah, but I was wondering whether you could somehow get into the
situation where clients between then are caching N distinct filehandles
with the same inode number.  Then a collision becomes likely around
2^16, by the usual birthday paradox rule-of-thumb.

Uh, but now that I think of it that's irrelevant.  At most one of those
filehandles actually refers to a still-existing file.  Any attempt to
use the other 2^16-1 should return -ESTALE.  So collisions among that
set don't matter, it's only collisions involving the existing file that
are interesting.  So, nevermind, I can't see a practical way to hit a
problem here....

--b.

[toc] | [prev] | [next] | [standalone]

#1394798 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 29 2016, David Howells wrote:

> Add a system call to make extended file information available, including
> file creation time, inode version and data version where available through
> the underlying filesystem.
>
>
> ========
> OVERVIEW
> ========

I think all this documentation is invaluable - thanks.
I would really like to see much of it in
    Documentation/filesystems/something.txt
rather than just in the commit log.

>
> The defined bits in the st_information field give local system data on a
> file, how it is accessed, where it is and what it does:

These bits form a channel for communication between the filesystem
developer and the application writer.  As such we should be sure that
channel actually communicates meaning...


>
> 	STATX_INFO_ENCRYPTED		File is encrypted
> 	STATX_INFO_TEMPORARY		File is temporary

What is "temporary"?  Is it a statement about quality of storage
technology (will be destroyed by reboot) or intention of creator
(created with O_TMPFILE) or something else?


> 	STATX_INFO_FABRICATED		File was made up by filesystem
> 	STATX_INFO_KERNEL_API		File is kernel API (eg: procfs/sysfs)

What is the difference between these two?  Both are synthesized by the
kernel.
Maybe the "KERNEL_API" is declared never to change its meaning, while the
fabricated one doesn't make a "stable API" promise?

What is the difference between fabricating a file from a bunch of blocks
spread over a storage device, and fabricating a file from a single field
in the super-block?


> 	STATX_INFO_REMOTE		File is remote

How far is "remote"?  Does Infiniband count?  Fibre channel?  iSCSI?
Is a file on a loop-back mounted NFS filesystem more remote than a
fibre-channel connection to the next town?

Or is this relative?  Within a filesystem there are "remote" files and
"non-remote" files and the distinction is filesystem-dependant??

> 	STATX_INFO_AUTOMOUNT		Dir is automount trigger
> 	STATX_INFO_AUTODIR		Dir provides unlisted automounts

I think this last one means that there are names in the directory which
may not appear in "readdir" but will respond to "stat".  I would prefer
the description to match the behavior without necessarily implying that
those names will be automounts.  e.g
        STATX_INFO_INCOMPLETE_READDIR   getdents may not report all
                                        names that respond to stat

> 	STATX_INFO_NONSYSTEM_OWNERSHIP	File has non-system ownership details

This probably is a well defined meaning that I just don't have the
context to understand.  For me, more words would help here.


I don't object to any of these flag.  I just want to be sure that I
understand them.
I am generally in favour this functionality going in promptly.

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1396393 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

> 	int ret = statx(int dfd,
> 			const char *filename,
> 			unsigned int flags,
> 			unsigned int mask,
> 			struct statx *buffer);

Please move the flags and mask after the buffer, similar to how all
the AT_ flags were added to the end for the statat calls.

> AT_FORCE_ATTR_SYNC can be set in flags.  This will require a network
> filesystem to synchronise its attributes with the server.
> 
> AT_NO_ATTR_SYNC can be set in flags.  This will suppress synchronisation
> with the server in a network filesystem.  The resulting values should be
> considered approximate.

And what happens if neither is set?

> mask is a bitmask indicating the fields in struct statx that are of
> interest to the caller.  The user should set this to STATX_BASIC_STATS to
> get the basic set returned by stat().

No a very good name for the constant.  I don't really see how this macro
is useful to start with.  And _ALL? sure, but what's basic?

> buffer points to the destination for the data.  This must be 256 bytes in
> size.

256 bytes or sizeof(struct statx)?  Even if they end up the same the
latter is a much more useful value.

> where st_information is local system information about the file,

What the heck is "local system information"?  Please define each
newly added field in detail.

> st_gen is
> the inode generation number, st_btime is the file creation time, st_version
> is the data version number (i_version),

Please define semantics for st_gen and st_version.

> Time fields are split into separate seconds and nanoseconds fields to make
> packing easier and the granularities can be queried with the filesystem
> info system call.  Note that times will be negative if before 1970; in such
> a case, the nanosecond fields should also be negative if not zero.

Please coordinate with Arnd on the timespamp format - I'd hate to have
a different encoding than he plans for all y2028/64-bit-time_t syscalls
to be added soon.

> 	STATX_MTIME		Want/got st_mtime
> 	STATX_CTIME		Want/got st_ctime
> 	STATX_INO		Want/got st_ino
> 	STATX_SIZE		Want/got st_size
> 	STATX_BLOCKS		Want/got st_blocks
> 	STATX_BASIC_STATS	[The stuff in the normal stat struct]
> 	STATX_BTIME		Want/got st_btime
> 	STATX_VERSION		Want/got st_data_version

What is st_data_version?

> 	STATX_GEN		Want/got st_gen
> 	STATX_ALL_STATS		[All currently available stuff]

Where does the STATS_ come from?  Why no simply _ALL?

How are the semantics defined when userspace asks for fields not
available?  I'd expect them to be ignored, but we should documentat that
fact.

> The defined bits in the st_information field give local system data on a
> file, how it is accessed, where it is and what it does:

Oh, here we get st_information.  The name sounds very wrong for these
flags, though.

> 	STATX_INFO_ENCRYPTED		File is encrypted

How do you define "encrypted", and what can the user do with this
information?

> 	STATX_INFO_TEMPORARY		File is temporary

How do you define "temporary", and what can the user do with this
information?

> 	STATX_INFO_FABRICATED		File was made up by filesystem

How do you define "fabricated", and what can the user do with this
information?

> 	STATX_INFO_KERNEL_API		File is kernel API (eg: procfs/sysfs)

How do you define "kernel API" and what can the user do with this
information?

> 	STATX_INFO_REMOTE		File is remote

How do you define "remote" and what can the user do with this
information?

> 	STATX_INFO_AUTOMOUNT		Dir is automount trigger

How do you define "automount trigger" and what can the user do with this
information?

> 	STATX_INFO_AUTODIR		Dir provides unlisted automounts

How do you define "unlisted automount" and what can the user do with this
information?

> 	STATX_INFO_NONSYSTEM_OWNERSHIP	File has non-system ownership details

How do you define "non-system ownership" and what can the user do with this
information?

> 
> These are for the use of GUI tools that might want to mark files specially,
> depending on what they are.

So far I don't see good definition of either flag, nor a good reason
to add.

> Fields in struct statx come in a number of classes:

I really disagree with all these special cases.  You should get
what you ask for, or rather what you ask for IFF the fs can provide it.
And we need to document for each field if it's optional if we want
to treat it as option.  A hodge podge bag of special cases is not an
API that a normal person can use.

> The following test program can be used to test the statx system call:
> 
> 	samples/statx/test-statx.c

Please add xfstests test cases that test all the corner cases.

And please prepare a man page to document this system call properly.

[toc] | [prev] | [next] | [standalone]

#1396982 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Sun, 2016-05-08 at 01:35 -0700, Christoph Hellwig wrote:
> > 
> > 	int ret = statx(int dfd,
> > 			const char *filename,
> > 			unsigned int flags,
> > 			unsigned int mask,
> > 			struct statx *buffer);
> 
> Please move the flags and mask after the buffer, similar to how all
> the AT_ flags were added to the end for the statat calls.
> 
> > AT_FORCE_ATTR_SYNC can be set in flags.  This will require a network
> > filesystem to synchronise its attributes with the server.
> > 
> > AT_NO_ATTR_SYNC can be set in flags.  This will suppress synchronisation
> > with the server in a network filesystem.  The resulting values should be
> > considered approximate.
> 
> And what happens if neither is set?
> 

I'd suggest we have the documentation state that the lack of either
flag leaves it up to the filesystem. In the case of NFS, you'd get
"normal" attribute cache behavior, for instance which is governed by
the ac* attributes.

We should also note that in the case of something like AT_NO_ATTR_SYNC
on NFS, you might _still_ end up talking to the server if the client
has nothing in-core for that inode.

> > mask is a bitmask indicating the fields in struct statx that are of
> > interest to the caller.  The user should set this to STATX_BASIC_STATS to
> > get the basic set returned by stat().
> 
> No a very good name for the constant.  I don't really see how this macro
> is useful to start with.  And _ALL? sure, but what's basic?
> 
> > buffer points to the destination for the data.  This must be 256 bytes in
> > size.
> 
> 256 bytes or sizeof(struct statx)?  Even if they end up the same the
> latter is a much more useful value.
> 

ACK. We should also consider that while we have a fair bit of padding
in this structure now, we could end up running out of space in it at
some point. We should at least have a clear idea of how we'll handle
such a situation.

The obvious solution would be to add a new flag that says that we're
passing in an extended statx structure. The kernel would know not to
touch stuff in the extended part unless the flag was set. Userland
would know that that part had not been touched by the kernel if the
outbound flag wasn't set.


> > where st_information is local system information about the file,
> 
> What the heck is "local system information"?  Please define each
> newly added field in detail.
> 
> > st_gen is
> > the inode generation number, st_btime is the file creation time, st_version
> > is the data version number (i_version),
> 
> Please define semantics for st_gen and st_version.
> 
> > Time fields are split into separate seconds and nanoseconds fields to make
> > packing easier and the granularities can be queried with the filesystem
> > info system call.  Note that times will be negative if before 1970; in such
> > a case, the nanosecond fields should also be negative if not zero.
> 
> Please coordinate with Arnd on the timespamp format - I'd hate to have
> a different encoding than he plans for all y2028/64-bit-time_t syscalls
> to be added soon.
> 
> > 	STATX_MTIME		Want/got st_mtime
> > 	STATX_CTIME		Want/got st_ctime
> > 	STATX_INO		Want/got st_ino
> > 	STATX_SIZE		Want/got st_size
> > 	STATX_BLOCKS		Want/got st_blocks
> > 	STATX_BASIC_STATS	[The stuff in the normal stat struct]
> > 	STATX_BTIME		Want/got st_btime
> > 	STATX_VERSION		Want/got st_data_version
> 
> What is st_data_version?
> 
> > 	STATX_GEN		Want/got st_gen
> > 	STATX_ALL_STATS		[All currently available stuff]
> 
> Where does the STATS_ come from?  Why no simply _ALL?
> 
> How are the semantics defined when userspace asks for fields not
> available?  I'd expect them to be ignored, but we should documentat that
> fact.
> 
> > The defined bits in the st_information field give local system data on a
> > file, how it is accessed, where it is and what it does:
> 
> Oh, here we get st_information.  The name sounds very wrong for these
> flags, though.
> 
> > 	STATX_INFO_ENCRYPTED		File is encrypted
> 
> How do you define "encrypted", and what can the user do with this
> information?
> 
> > 	STATX_INFO_TEMPORARY		File is temporary
> 
> How do you define "temporary", and what can the user do with this
> information?
> 
> > 	STATX_INFO_FABRICATED		File was made up by filesystem
> 
> How do you define "fabricated", and what can the user do with this
> information?
> 
> > 	STATX_INFO_KERNEL_API		File is kernel API (eg: procfs/sysfs)
> 
> How do you define "kernel API" and what can the user do with this
> information?
> 
> > 	STATX_INFO_REMOTE		File is remote
> 
> How do you define "remote" and what can the user do with this
> information?
> 
> > 	STATX_INFO_AUTOMOUNT		Dir is automount trigger
> 
> How do you define "automount trigger" and what can the user do with this
> information?
> 
> > 	STATX_INFO_AUTODIR		Dir provides unlisted automounts
> 
> How do you define "unlisted automount" and what can the user do with this
> information?
> 
> > 	STATX_INFO_NONSYSTEM_OWNERSHIP	File has non-system ownership details
> 
> How do you define "non-system ownership" and what can the user do with this
> information?
> 

Good questions all around.

My personal opinion is that if we have any attrs that are of
questionable value or that don't have a clear definition, that we
should just leave them out for now. This interface is designed to be
extendable, so there's no need to add it all in in the first pass. We
should focus on getting the API right and sort out the gory details of
specific attributes on a case-by-case basis.

> > 
> > These are for the use of GUI tools that might want to mark files specially,
> > depending on what they are.
> 
> So far I don't see good definition of either flag, nor a good reason
> to add.
> 
> > Fields in struct statx come in a number of classes:
> 
> I really disagree with all these special cases.  You should get
> what you ask for, or rather what you ask for IFF the fs can provide it.
> And we need to document for each field if it's optional if we want
> to treat it as option.  A hodge podge bag of special cases is not an
> API that a normal person can use.
> 

Agreed. In fact, the required attributes might be a good place to draw
the line on the initial submission of this patchset. Maybe just say "no
optional attributes yet" and we'll add them in later patches?

> > The following test program can be used to test the statx system call:
> > 
> > 	samples/statx/test-statx.c
> 
> Please add xfstests test cases that test all the corner cases.
> 
> And please prepare a man page to document this system call properly.

Nothing wrong with preparing that ahead of time, but I see that as
something that should go along with the userland submission. In fact,
what's the plan for userland here? Should this be added to glibc or do
would it be better/simpler to have a new library for this?

Either way, what would be best for now though is to do What Neil
suggested, and lift most of this commit log into a file under
Documentation/.

Furthermore, it'd probably be nice to document each mask bit in the
header file that userland will end up including. It's often the case
that the manpage may not reflect what the currently installed kernel
actually supports. The kernel headers are often more authoritative.
Being able to look at the header file for would be ideal.

-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1397741 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Mon, May 09, 2016 at 08:02:58AM -0400, Jeff Layton wrote:
> > > AT_FORCE_ATTR_SYNC can be set in flags.????This will require a network
> > > filesystem to synchronise its attributes with the server.
> > > 
> > > AT_NO_ATTR_SYNC can be set in flags.????This will suppress synchronisation
> > > with the server in a network filesystem.????The resulting values should be
> > > considered approximate.
> > 
> > And what happens if neither is set?
> > 
> 
> I'd suggest we have the documentation state that the lack of either
> flag leaves it up to the filesystem. In the case of NFS, you'd get
> "normal" attribute cache behavior, for instance which is governed by
> the ac* attributes.
> 
> We should also note that in the case of something like AT_NO_ATTR_SYNC
> on NFS, you might _still_ end up talking to the server if the client
> has nothing in-core for that inode.

File systems specific "legacy" defaults are a bad idea.  If we can't
describe the semantics we should not allow them, never mind making
the the default.  I'd strongly suggest picking one of the above flags
as the default behavior and only allowing the other as optional flag.
I suspect NO_SYNC is the better one for the flag, as otherwise people
will be surprised once they test their default case on a network
filesystem.

[toc] | [prev] | [next] | [standalone]

#1398095 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Tue, 2016-05-10 at 00:00 -0700, Christoph Hellwig wrote:
> On Mon, May 09, 2016 at 08:02:58AM -0400, Jeff Layton wrote:
> > > > AT_FORCE_ATTR_SYNC can be set in flags.????This will require a
> > > > network
> > > > filesystem to synchronise its attributes with the server.
> > > > 
> > > > AT_NO_ATTR_SYNC can be set in flags.????This will suppress
> > > > synchronisation
> > > > with the server in a network filesystem.????The resulting
> > > > values should be
> > > > considered approximate.
> > > 
> > > And what happens if neither is set?
> > > 
> > 
> > I'd suggest we have the documentation state that the lack of either
> > flag leaves it up to the filesystem. In the case of NFS, you'd get
> > "normal" attribute cache behavior, for instance which is governed
> > by
> > the ac* attributes.
> > 
> > We should also note that in the case of something like
> > AT_NO_ATTR_SYNC
> > on NFS, you might _still_ end up talking to the server if the
> > client
> > has nothing in-core for that inode.
> 
> File systems specific "legacy" defaults are a bad idea.  If we can't
> describe the semantics we should not allow them, never mind making
> the the default.  I'd strongly suggest picking one of the above flags
> as the default behavior and only allowing the other as optional flag.
> I suspect NO_SYNC is the better one for the flag, as otherwise people
> will be surprised once they test their default case on a network
> filesystem.

Ok, that's a good point. So basically you suggest that xstat should
always have FORCE_SYNC semantics unless the NO_SYNC flag is set? Given
that we don't need to worry about legacy users with this interface,
that seems like a reasonable approach to me.
 
-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1397023 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

Christoph Hellwig <hch@infradead.org> wrote:

> > 	int ret = statx(int dfd,
> > 			const char *filename,
> > 			unsigned int flags,
> > 			unsigned int mask,
> > 			struct statx *buffer);
> 
> Please move the flags and mask after the buffer, similar to how all
> the AT_ flags were added to the end for the statat calls.

Sure, if you really want.

> > AT_FORCE_ATTR_SYNC can be set in flags.  This will require a network
> > filesystem to synchronise its attributes with the server.
> > 
> > AT_NO_ATTR_SYNC can be set in flags.  This will suppress synchronisation
> > with the server in a network filesystem.  The resulting values should be
> > considered approximate.
> 
> And what happens if neither is set?

It does what stat() does now, whatever that is for each fs.  The assumption is
that this might be used to emulate stat() from userspace.  However, we want to
be able to make sure we get the two behaviours above.

> > mask is a bitmask indicating the fields in struct statx that are of
> > interest to the caller.  The user should set this to STATX_BASIC_STATS to
> > get the basic set returned by stat().
> 
> No a very good name for the constant.  I don't really see how this macro
> is useful to start with.

It's the bits that correspond to all the data in the the current stat struct.
So if you want to emulate stat(), you should pass this in mask.

> And _ALL? sure, but what's basic?

Actually, _ALL is perhaps the less useful of the two since the bitset is not
closed.  OTOH - anything not in _ALL won't be listed explicitly in the
structure, but would rather consume space space.

> > st_gen is
> > the inode generation number, st_btime is the file creation time, st_version
> > is the data version number (i_version),
> 
> Please define semantics for st_gen and st_version.

I've been asked to drop st_gen for security reasons.

I can't offhand think of a way to define st_version (or i_version, for that
matter) that would be consistent across all filesystems.  I would lean towards
"gets incremented monotonically by 1 for each data write operation committed,
but not for any metadata operations", but I'm fairly certain this won't jibe
with disk operations.  So I can leave it out for now and bring it back if we
find a real user for it.

> > Time fields are split into separate seconds and nanoseconds fields to make
> > packing easier and the granularities can be queried with the filesystem
> > info system call.  Note that times will be negative if before 1970; in such
> > a case, the nanosecond fields should also be negative if not zero.
> 
> Please coordinate with Arnd on the timespamp format - I'd hate to have
> a different encoding than he plans for all y2028/64-bit-time_t syscalls
> to be added soon.

I have discussed this with him previously.

> > 	STATX_VERSION		Want/got st_data_version
> 
> What is st_data_version?

Sorry, that should've been st_version.  It got renamed.

> > 	STATX_GEN		Want/got st_gen
> > 	STATX_ALL_STATS		[All currently available stuff]
> 
> Where does the STATS_ come from?  Why no simply _ALL?

Why not STATX_ALL_STATS?

David

[toc] | [prev] | [next] | [standalone]

#1397029 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

David Howells <dhowells@redhat.com> wrote:

> > > st_gen is
> > > the inode generation number, st_btime is the file creation time, st_version
> > > is the data version number (i_version),
> > 
> > Please define semantics for st_gen and st_version.
> 
> I've been asked to drop st_gen for security reasons.
> 
> I can't offhand think of a way to define st_version (or i_version, for that
> matter) that would be consistent across all filesystems.  I would lean towards
> "gets incremented monotonically by 1 for each data write operation committed,
> but not for any metadata operations", but I'm fairly certain this won't jibe
> with disk operations.

I meant disk filesystems that we have now, not disk operations.

David

[toc] | [prev] | [next] | [standalone]

#1397065 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On 5/9/16, 08:57, "linux-nfs-owner@vger.kernel.org on behalf of David Howells" <linux-nfs-owner@vger.kernel.org on behalf of dhowells@redhat.com> wrote:

>Christoph Hellwig <hch@infradead.org> wrote:
>
>> > 	int ret = statx(int dfd,
>> > 			const char *filename,
>> > 			unsigned int flags,
>> > 			unsigned int mask,
>> > 			struct statx *buffer);
>> 
>> Please move the flags and mask after the buffer, similar to how all
>> the AT_ flags were added to the end for the statat calls.
>
>Sure, if you really want.
>
>> > AT_FORCE_ATTR_SYNC can be set in flags.  This will require a network
>> > filesystem to synchronise its attributes with the server.
>> > 
>> > AT_NO_ATTR_SYNC can be set in flags.  This will suppress synchronisation
>> > with the server in a network filesystem.  The resulting values should be
>> > considered approximate.
>> 
>> And what happens if neither is set?
>
>It does what stat() does now, whatever that is for each fs.  The assumption is
>that this might be used to emulate stat() from userspace.  However, we want to
>be able to make sure we get the two behaviours above.
>
>> > mask is a bitmask indicating the fields in struct statx that are of
>> > interest to the caller.  The user should set this to STATX_BASIC_STATS to
>> > get the basic set returned by stat().
>> 
>> No a very good name for the constant.  I don't really see how this macro
>> is useful to start with.
>
>It's the bits that correspond to all the data in the the current stat struct.
>So if you want to emulate stat(), you should pass this in mask.
>
>> And _ALL? sure, but what's basic?
>
>Actually, _ALL is perhaps the less useful of the two since the bitset is not
>closed.  OTOH - anything not in _ALL won't be listed explicitly in the
>structure, but would rather consume space space.
>
>> > st_gen is
>> > the inode generation number, st_btime is the file creation time, st_version
>> > is the data version number (i_version),
>> 
>> Please define semantics for st_gen and st_version.
>
>I've been asked to drop st_gen for security reasons.
>
>I can't offhand think of a way to define st_version (or i_version, for that
>matter) that would be consistent across all filesystems.  I would lean towards
>"gets incremented monotonically by 1 for each data write operation committed,
>but not for any metadata operations", but I'm fairly certain this won't jibe
>with disk operations.  So I can leave it out for now and bring it back if we
>find a real user for it.

The NFSv4 definition for the change attribute (which is mapped to i_version when IS_I_VERSION(inode) is true) is

"A value created by the server that the client can use to determine if
   file data, directory contents, or attributes of the object have been
   modified.  The server may return the object's time_metadata attribute
   for this attribute's value, but only if the file system object cannot
   be updated more frequently than the resolution of time_metadata."


IOW: it is a value that changes on all data and metadata operations, and with no monotonicity requirement. I’m pretty sure all userspace NFSv4 servers out there would like to access it (e.g. Ganesha, dCache).

>
>> > Time fields are split into separate seconds and nanoseconds fields to make
>> > packing easier and the granularities can be queried with the filesystem
>> > info system call.  Note that times will be negative if before 1970; in such
>> > a case, the nanosecond fields should also be negative if not zero.
>> 
>> Please coordinate with Arnd on the timespamp format - I'd hate to have
>> a different encoding than he plans for all y2028/64-bit-time_t syscalls
>> to be added soon.
>
>I have discussed this with him previously.
>
>> > 	STATX_VERSION		Want/got st_data_version
>> 
>> What is st_data_version?
>
>Sorry, that should've been st_version.  It got renamed.
>
>> > 	STATX_GEN		Want/got st_gen
>> > 	STATX_ALL_STATS		[All currently available stuff]
>> 
>> Where does the STATS_ come from?  Why no simply _ALL?
>
>Why not STATX_ALL_STATS?
>
>David
>--
>To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1397745 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Mon, May 09, 2016 at 01:57:32PM +0100, David Howells wrote:
> > > AT_FORCE_ATTR_SYNC can be set in flags.  This will require a network
> > > filesystem to synchronise its attributes with the server.
> > > 
> > > AT_NO_ATTR_SYNC can be set in flags.  This will suppress synchronisation
> > > with the server in a network filesystem.  The resulting values should be
> > > considered approximate.
> > 
> > And what happens if neither is set?
> 
> It does what stat() does now, whatever that is for each fs.  The assumption is
> that this might be used to emulate stat() from userspace.  However, we want to
> be able to make sure we get the two behaviours above.

And why would you emulate stat if we already have a perfectly working
version of it?  Either way we need to document what that behavior is,
and why userspace would chose one of the three options.

> > > mask is a bitmask indicating the fields in struct statx that are of
> > > interest to the caller.  The user should set this to STATX_BASIC_STATS to
> > > get the basic set returned by stat().
> > 
> > No a very good name for the constant.  I don't really see how this macro
> > is useful to start with.
> 
> It's the bits that correspond to all the data in the the current stat struct.
> So if you want to emulate stat(), you should pass this in mask.

which of the many stat version supported by Linux or glibc (nevermind other
OSes)?  And why would you care about that, as you could just use stat in
that case?  And even if you really cared how do you know what attributes
are "basic" if we don't properly document that?  And last but not least
why would the caller of the syscall (various libcs) care to get this
constant instead of defining it on it's own based on what ABI it
exports?

> > > 	STATX_GEN		Want/got st_gen
> > > 	STATX_ALL_STATS		[All currently available stuff]
> > 
> > Where does the STATS_ come from?  Why no simply _ALL?
> 
> Why not STATX_ALL_STATS?

Because it's the only places STATS or stats is used in the whole
interface.  It also doesn't match our common use for stats (as in
statistics, not fields of a stat-like structure)

[toc] | [prev] | [next] | [standalone]

#1397843 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

Christoph Hellwig <hch@infradead.org> wrote:

> > It does what stat() does now, whatever that is for each fs.  The
> > assumption is that this might be used to emulate stat() from userspace.
> > However, we want to be able to make sure we get the two behaviours above.
> 
> And why would you emulate stat if we already have a perfectly working
> version of it?  Either way we need to document what that behavior is,
> and why userspace would chose one of the three options.

Because it's not necessarily a perfectly working version of it.  See the Y2037
problem for example.

I was assuming that C libraries might want to update the struct stat and the
stat call() to provide fields that aren't currently there in Linux but are in
other OS's.  We could even dispense with older stat syscalls on new arches.

Admittedly, this means that you would have backwardly incompatible versions of
the C library and would have to version your interface, so it might be too
much effort.

However, if we're going to discard this possibility, we can make these
features available only to direct calls of extended stat.

David

[toc] | [prev] | [next] | [standalone]

#1399817 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Tue, May 10, 2016 at 09:25:55AM +0100, David Howells wrote:
> Because it's not necessarily a perfectly working version of it.  See the Y2037
> problem for example.
> 
> I was assuming that C libraries might want to update the struct stat and the
> stat call() to provide fields that aren't currently there in Linux but are in
> other OS's.  We could even dispense with older stat syscalls on new arches.

Please stop this whole let's get rid of old syscalls on new
architectures stuff.  This just means we have to do the translation
multiple, and the one in userspace is more costly as we it needs to be
in every copy of the library.  And times where we had a single libc
instance (nevermind implementation) are long over if we ever actually
had them.

> However, if we're going to discard this possibility, we can make these
> features available only to direct calls of extended stat.

And even if we want to to do a stat emulation despite that above
argument let's add the flag once a major libc implementation actually
wants to use it and taylor it towards the use case.  Don't just add
it just because, and even more importantly don't make it the default.

[toc] | [prev] | [next] | [standalone]

#1397060 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

Christoph Hellwig <hch@infradead.org> wrote:

> How are the semantics defined when userspace asks for fields not
> available?  I'd expect them to be ignored, but we should documentat that
> fact.

I went into this in some detail.

> > Fields in struct statx come in a number of classes:
> 
> I really disagree with all these special cases.  You should get
> what you ask for, or rather what you ask for IFF the fs can provide it.
> And we need to document for each field if it's optional if we want
> to treat it as option.

I did document this.  You saw it as a bunch of special cases.  It is not.
stat() fabricates some of the data it returns under certain circumstances.

> A hodge podge bag of special cases is not an API that a normal person can
> use.

Let's look at the list, and please bear in mind I'm trying to make it so that
you can emulate stat() through this interface.  If you want to waive that
requirement - or push the emulation out to userspace - then I can forego
providing unsupported data from the basic stat set.

| (0) st_information, st_dev_*, st_blksize.

st_dev_* and st_blksize must always be available from whatever we stat.
Because this is the case, there's no point providing mask bits for them.

My implementation defines st_information to be in this class, but it doesn't
have to be.  Note that st_information really needs a way to ask the filesystem
what flags it actually supports so that you can distinguish being 0 -> not set
from 0 -> not supported, hence the fsinfo() interface that I've dropped for
now.

| (1) st_nlinks, st_uid, st_gid, st_[amc]time*, st_ino, st_size, st_blocks.

These data are all in the bog standard struct stat.  As it is, they must all
be given values as for stat().  However, mask bits are provided to indicate
when the value presented here is actually fabricated so that the user can
decide not to use them.

| (2) st_mode.

This is actually in two parts.  There's the file type (which must always be
set correctly) and the mode bits (which may be fabricated).  STATX_MODE covers
the mode bits only.

| (3) st_rdev_*.

This datum is part of the bog standard struct stat, and as such must be set to
something.  However, the value is only relevant in the case that the mode
indicates a blockdev or chardev.  STATX_RDEV can be considered redundant in
such a case.

| (4) File creation time (st_btime*), data version (st_version), inode
|     generation number (st_gen).

These are all new data and have no counterpart in the Linux struct stat.
However, they do in the struct stat on other Unix variants (st_birthtime and
st_gen, for example, exist on BSD).  Not all filesystems provide them so if
they are requested but are not actually supported by a filesystem, the bit in
the mask is cleared upon returning.

However, even if you didn't ask for a datum, it may still be available - and I
am permitting a filesystem to give you the datum and mark the mask to indicate
the value's availability, even if you didn't ask for it.  You are free to
ignore it.

At this time, I think it likely that all new attributes would be in this
class.  One could argue that something like st_win_attrs (in patch 5) could be
in class 0 if added immediately, but anything added later *must* have a mask
bit to indicate its presence.


So, barring st_information, classes (0) - (3) are all current stat stuff.
That is how they work *now*.  All I'm doing is defining which data have mask
bits, and under what conditions the mask bit might not be set.

David

[toc] | [prev] | [next] | [standalone]

#1397744 — Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available

On Mon, May 09, 2016 at 02:38:21PM +0100, David Howells wrote:
> Let's look at the list, and please bear in mind I'm trying to make it so that
> you can emulate stat() through this interface.  If you want to waive that
> requirement - or push the emulation out to userspace - then I can forego
> providing unsupported data from the basic stat set.

why would you want to emulate stat?

> | (0) st_information, st_dev_*, st_blksize.
> 
> st_dev_* and st_blksize must always be available from whatever we stat.
> Because this is the case, there's no point providing mask bits for them.
> 
> My implementation defines st_information to be in this class, but it doesn't
> have to be.  Note that st_information really needs a way to ask the filesystem
> what flags it actually supports so that you can distinguish being 0 -> not set
> from 0 -> not supported, hence the fsinfo() interface that I've dropped for
> now.

All of these are easily available.  But why special case them so that
userspace must not ask for them?  This makes an otherwise totally
regular interface special now.  Note that filesystems could always fill
it out anyway and set it in the return mask.

> | (1) st_nlinks, st_uid, st_gid, st_[amc]time*, st_ino, st_size, st_blocks.
> 
> These data are all in the bog standard struct stat.  As it is, they must all
> be given values as for stat().  However, mask bits are provided to indicate
> when the value presented here is actually fabricated so that the user can
> decide not to use them.

Next special case..

> | (2) st_mode.
> 
> This is actually in two parts.  There's the file type (which must always be
> set correctly) and the mode bits (which may be fabricated).  STATX_MODE covers
> the mode bits only.

Next special case.

> 
> | (3) st_rdev_*.
> 
> This datum is part of the bog standard struct stat, and as such must be set to
> something.  However, the value is only relevant in the case that the mode
> indicates a blockdev or chardev.  STATX_RDEV can be considered redundant in
> such a case.
> 
> | (4) File creation time (st_btime*), data version (st_version), inode
> |     generation number (st_gen).
> 
> These are all new data and have no counterpart in the Linux struct stat.
> However, they do in the struct stat on other Unix variants (st_birthtime and
> st_gen, for example, exist on BSD).  Not all filesystems provide them so if
> they are requested but are not actually supported by a filesystem, the bit in
> the mask is cleared upon returning.
> 
> However, even if you didn't ask for a datum, it may still be available - and I
> am permitting a filesystem to give you the datum and mark the mask to indicate
> the value's availability, even if you didn't ask for it.  You are free to
> ignore it.

I think the fs may return it anyway case is fine, but let's make that
a 100% generic thing and not special case fields.

> At this time, I think it likely that all new attributes would be in this
> class.  One could argue that something like st_win_attrs (in patch 5) could be
> in class 0 if added immediately, but anything added later *must* have a mask
> bit to indicate its presence.
> 
> 
> So, barring st_information, classes (0) - (3) are all current stat stuff.
> That is how they work *now*.  All I'm doing is defining which data have mask
> bits, and under what conditions the mask bit might not be set.

Who cares about stat?  You are adding a new system call and are not
bound by what certain versions of stat did at specific points in time.

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web