Re: [RFC][PATCH 0/4] Enhanced file stat system call

Re: [RFC][PATCH 0/4] Enhanced file stat system call

[Michael Kerrisk]

Hi David,

Can you please CC linux-api@ on all future iterations of this patch!

Thanks,

Michael


On Thu, Nov 17, 2016 at 2:34 PM, David Howells <dhowells@redhat.com> wrote:
>
> Implement a new system call to provide enhanced file stats.  The patches can
> be found here:
>
>         http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=xstat
>
>
> ===========
> DESCRIPTION
> ===========
>
> The first patch provides this new system call:
>
>         long ret = statx(int dfd,
>                          const char *filename,
>                          unsigned atflag,
>                          unsigned mask,
>                          struct statx *buffer);
>
> This is an enhanced file stat function that provides a number of useful
> features, in summary:
>
>  (1) More information: creation time, data version number and attributes.
>      A subset of these is available through a number of filesystems (such
>      as CIFS, NFS, AFS, Ext4 and BTRFS).
>
>  (2) Lightweight stat (AT_STATX_DONT_SYNC): Ask for just those details of
>      interest, and allow a network fs to approximate anything not of
>      interest, without going to the server.
>
>  (3) Heavyweight stat (AT_STATX_FORCE_SYNC): Force a network fs to flush
>      buffers and go to the server, even if it thinks its cached attributes
>      are up to date.
>
>  (4) Allow the filesystem to indicate what it can/cannot provide: A
>      filesystem can now say it doesn't support a standard stat feature if
>      that isn't available.
>
>  (5) Make the fields a consistent size on all arches, and make them large.
>
>  (6) Can be extended by using more request flags and using up the padding
>      space in the statx struct.
>
> Note that no lstat() equivalent is required as that can be implemented
> through statx() with atflag == 0.  There is also no fstat() equivalent as
> that can be implemented through statx() with filename == NULL and the
> relevant fd passed as dfd.
>
>
> =======
> TESTING
> =======
>
> A test program is added into samples/statx/ by the first patch.
>
> David
> ---
> David Howells (4):
>       statx: Add a system call to make enhanced file info available
>       statx: Ext4: Return enhanced file attributes
>       statx: NFS: Return enhanced file attributes
>       statx: AFS: Return enhanced file attributes
>
>
>  arch/x86/entry/syscalls/syscall_32.tbl |    1
>  arch/x86/entry/syscalls/syscall_64.tbl |    1
>  fs/afs/inode.c                         |   21 ++
>  fs/exportfs/expfs.c                    |    4
>  fs/ext4/ext4.h                         |    2
>  fs/ext4/file.c                         |    2
>  fs/ext4/inode.c                        |   38 ++++
>  fs/ext4/namei.c                        |    2
>  fs/ext4/symlink.c                      |    2
>  fs/nfs/inode.c                         |   41 ++++
>  fs/stat.c                              |  294 +++++++++++++++++++++++++++++---
>  include/linux/fs.h                     |    5 -
>  include/linux/stat.h                   |   19 +-
>  include/linux/syscalls.h               |    3
>  include/uapi/linux/fcntl.h             |    2
>  include/uapi/linux/stat.h              |  124 +++++++++++++
>  samples/Kconfig                        |    5 +
>  samples/Makefile                       |    3
>  samples/statx/Makefile                 |   10 +
>  samples/statx/test-statx.c             |  248 +++++++++++++++++++++++++++
>  20 files changed, 771 insertions(+), 56 deletions(-)
>  create mode 100644 samples/statx/Makefile
>  create mode 100644 samples/statx/test-statx.c
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

Re: [RFC][PATCH 0/4] Enhanced file stat system call

[David Howells]

Michael Kerrisk <mtk.manpages@gmail.com> wrote:

> Can you please CC linux-api@ on all future iterations of this patch!

Sorry, yes - I remembered right after posting it, of course.

David

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[David Howells]

Andreas Dilger <adilger-m1MBpc4rdrD3fQ9qLvQP4Q@public.gmane.org> wrote:

> >> If neither AT_STATX_*_SYNC flag is set, the behaviour is the default for
> >> stat() on that filesystem.
> > 
> > We also need to specify here what happens if both bits are set. Should
> > that be -EINVAL?
> 
> If that is the case, then it doesn't make sense to have two contradictory
> flags.

Yes it does.  There are actually *three* cases, not two.  Maybe, rather than a
pair of flags, I should stake out a 2-bit field with three possible values.

> Pick a default behaviour (i.e. return what is known on the client),

The default behaviour has to be what stat() does now for any particular
filesystem.  statx() is likely to get used to emulate stat() so that stat()
can be made to return safe timestamps.  If we make it so that statx() cannot
do this, it's very likely that we'll see yet another stat() variant syscall
being added.

> and if this is 100% accurate (e.g. local filesystem or filesystem with
> strong coherency)

In a netfs, it was 100% accurate at the point the server started transmitting
its reply.  This may no longer be true - even with something like AFS that has
change notifications.

> then it can optionally set the SYNC flag in the returned
> flags.

So you suggest putting the SYNC flag(s) in the request mask rather than
sharing the AT_* flag space?

> If the application needs 100% accurate size info, then it can set the SYNC
> flag in the request and the filesystem may need to do extra work to fetch
> accurate data from the server.

Note that one of the things that people asked for was a
DONT_GO_TO_THE_SERVER_AT_ALL flag.  I take it this is your suggested default?

David

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[Andreas Dilger]

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

On Nov 18, 2016, at 1:59 AM, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> Andreas Dilger <adilger-m1MBpc4rdrD3fQ9qLvQP4Q@public.gmane.org> wrote:
> 
>>>> If neither AT_STATX_*_SYNC flag is set, the behaviour is the default for
>>>> stat() on that filesystem.
>>> 
>>> We also need to specify here what happens if both bits are set. Should
>>> that be -EINVAL?
>> 
>> If that is the case, then it doesn't make sense to have two contradictory
>> flags.
> 
> Yes it does.  There are actually *three* cases, not two.  Maybe, rather
> than a pair of flags, I should stake out a 2-bit field with three possible
> values.

That would probably be better in this case.

>> Pick a default behaviour (i.e. return what is known on the client),
> 
> The default behaviour has to be what stat() does now for any particular
> filesystem.  statx() is likely to get used to emulate stat() so that stat()
> can be made to return safe timestamps.  If we make it so that statx() cannot
> do this, it's very likely that we'll see yet another stat() variant syscall
> being added.
> 
>> and if this is 100% accurate (e.g. local filesystem or filesystem with
>> strong coherency)
> 
> In a netfs, it was 100% accurate at the point the server started
> transmitting its reply.  This may no longer be true - even with something
> like AFS that has change notifications.

Sure, that is always true, even when running with a local filesystem.
My distinction here is "whether the client currently has a lock that
ensures its copy of the size is correct" vs. "the client has some size
that was correct at one point but may be totally incorrect now".

>> then it can optionally set the SYNC flag in the returned
>> flags.
> 
> So you suggest putting the SYNC flag(s) in the request mask rather than
> sharing the AT_* flag space?

Sorry, I was conflating flags.

>> If the application needs 100% accurate size info, then it can set the SYNC
>> flag in the request and the filesystem may need to do extra work to fetch
>> accurate data from the server.
> 
> Note that one of the things that people asked for was a
> DONT_GO_TO_THE_SERVER_AT_ALL flag.  I take it this is your suggested default?

Isn't that what NFS does today?

In any case, I'm not trying to rewrite NFS semantics here.  The main fix
would be to have the three-valued two-bit flag so that there aren't two
flags that mean opposite things.  That would also allow expansion to have
a fourth state in the future, if there was a need.

Cheers, Andreas






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

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[David Howells]

Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:

> > (13) FS_IOC_GETFLAGS value.  These could be translated to BSD's st_flags.
> >      Note that the Linux IOC flags are a mess and filesystems such as Ext4
> >      define flags that aren't in linux/fs.h, so translation in the kernel
> >      may be a necessity (or, possibly, we provide the filesystem type too).
> 
> And we now also have FS_IOC_FSGETXATTR that extends the flags
> and information userspace can get from filesystems.

Btw, can you point me at the manpage that defines the fsxattr struct and its
flags?

Also, ioctl_list(2) should probably include FS_IOC_FS[GS]ETXATTR.

David

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[Dave Chinner]

On Fri, Nov 18, 2016 at 10:29:04AM +0000, David Howells wrote:
> Dave Chinner <david@fromorbit.com> wrote:
> 
> > > (13) FS_IOC_GETFLAGS value.  These could be translated to BSD's st_flags.
> > >      Note that the Linux IOC flags are a mess and filesystems such as Ext4
> > >      define flags that aren't in linux/fs.h, so translation in the kernel
> > >      may be a necessity (or, possibly, we provide the filesystem type too).
> > 
> > And we now also have FS_IOC_FSGETXATTR that extends the flags
> > and information userspace can get from filesystems.
> 
> Btw, can you point me at the manpage that defines the fsxattr struct and its
> flags?

man xfsctl is the original source. However, ....

> Also, ioctl_list(2) should probably include FS_IOC_FS[GS]ETXATTR.

... I thought patches had been sent to Michael to document them at
the VFS ioctl level. maybe I confused it with something else.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[David Howells]

Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:

> > Btw, can you point me at the manpage that defines the fsxattr struct and its
> > flags?
> 
> man xfsctl is the original source. However, ....

	warthog>man xfsctl
	No manual entry for xfsctl

This should be in the kernel manpages repo since it's kernel UAPI.

David

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[Dave Chinner]

On Fri, Nov 18, 2016 at 09:48:21PM +0000, David Howells wrote:
> Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> 
> > > Btw, can you point me at the manpage that defines the fsxattr struct and its
> > > flags?
> > 
> > man xfsctl is the original source. However, ....
> 
> 	warthog>man xfsctl
> 	No manual entry for xfsctl
> 
> This should be in the kernel manpages repo since it's kernel UAPI.

"original source" is where the API came from - that's
XFS_IOC_FSGETXATTR - and xfsctl from xfsprogs is where that was
documented. That page documents a bunch of other XFS specific
ioctls, too, so it is appropriately located.

Like I said, I thought patches had been sent to Michael to lift the
FS_IOC_FSGETXATTR documentation from this page into the official
linux man pages package. If not, that can be chased up separately,
but the XFS ioctl man page certainly does not belong there.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[Michael Kerrisk (man-pages)]

Hi Dave,

[Best to CC me when I'm mentioned in the discussion. I only caught
this by chance.]

On 18 November 2016 at 23:17, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Nov 18, 2016 at 09:48:21PM +0000, David Howells wrote:
>> Dave Chinner <david@fromorbit.com> wrote:
>>
>> > > Btw, can you point me at the manpage that defines the fsxattr struct and its
>> > > flags?
>> >
>> > man xfsctl is the original source. However, ....
>>
>>       warthog>man xfsctl
>>       No manual entry for xfsctl
>>
>> This should be in the kernel manpages repo since it's kernel UAPI.
>
> "original source" is where the API came from - that's
> XFS_IOC_FSGETXATTR - and xfsctl from xfsprogs is where that was
> documented. That page documents a bunch of other XFS specific
> ioctls, too, so it is appropriately located.
>
> Like I said, I thought patches had been sent to Michael to lift the
> FS_IOC_FSGETXATTR documentation from this page into the official
> linux man pages package. If not, that can be chased up separately,
> but the XFS ioctl man page certainly does not belong there.

No patches for FS_IOC_FSGETXATTR documentation came to man-pages, as
far as I know. I'll be happy to take them.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/