All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@poochiereds.net>
To: Christoph Hellwig <hch@infradead.org>,
	David Howells <dhowells@redhat.com>
Cc: linux-fsdevel@vger.kernel.org, linux-afs@vger.kernel.org,
	linux-nfs@vger.kernel.org, samba-technical@lists.samba.org,
	linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available
Date: Mon, 09 May 2016 08:02:58 -0400	[thread overview]
Message-ID: <1462795378.4481.31.camel@poochiereds.net> (raw)
In-Reply-To: <20160508083543.GA14316@infradead.org>

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>

WARNING: multiple messages have this Message-ID
From: Jeff Layton <jlayton@poochiereds.net>
To: Christoph Hellwig <hch@infradead.org>,
	David Howells <dhowells@redhat.com>
Cc: linux-fsdevel@vger.kernel.org, linux-afs@vger.kernel.org,
	linux-nfs@vger.kernel.org, samba-technical@lists.samba.org,
	linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available
Date: Mon, 09 May 2016 08:02:58 -0400	[thread overview]
Message-ID: <1462795378.4481.31.camel@poochiereds.net> (raw)
In-Reply-To: <20160508083543.GA14316@infradead.org>

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>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2016-05-09 12:03 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-29 12:57 [RFC][PATCH 0/6] Enhanced file stat system call David Howells
2016-04-29 12:57 ` [PATCH 1/6] statx: Add a system call to make enhanced file info available David Howells
2016-05-02 22:46   ` Andreas Dilger
2016-05-02 22:46     ` Andreas Dilger
2016-05-03 15:53   ` David Howells
2016-05-04 22:56   ` Dave Chinner
2016-05-05  0:09     ` NeilBrown
2016-05-05  0:09       ` NeilBrown
2016-05-05 19:48       ` Jeff Layton
2016-05-06 18:07         ` J. Bruce Fields
2016-05-06 18:07           ` J. Bruce Fields
2016-05-05 20:04       ` David Howells
2016-05-05 20:04         ` David Howells
2016-05-06  1:39         ` Dave Chinner
2016-05-06  1:39           ` Dave Chinner
2016-05-06  1:39           ` Dave Chinner
2016-05-06 18:29     ` J. Bruce Fields
2016-05-09  1:45       ` Dave Chinner
2016-05-09  2:46         ` J. Bruce Fields
2016-05-04 23:56   ` NeilBrown
2016-05-08  8:35   ` Christoph Hellwig
2016-05-08  8:35     ` Christoph Hellwig
2016-05-09 12:02     ` Jeff Layton [this message]
2016-05-09 12:02       ` Jeff Layton
2016-05-10  7:00       ` Christoph Hellwig
2016-05-10  7:00         ` Christoph Hellwig
2016-05-10 13:21         ` Jeff Layton
2016-05-10 13:21           ` Jeff Layton
2016-05-09 12:57   ` David Howells
2016-05-09 12:57     ` David Howells
2016-05-09 13:23     ` Trond Myklebust
2016-05-09 13:23       ` Trond Myklebust
2016-05-09 13:23       ` Trond Myklebust
2016-05-10  7:04     ` Christoph Hellwig
2016-05-10  8:25     ` David Howells
2016-05-12  9:11       ` Christoph Hellwig
2016-05-13 15:28         ` Arnd Bergmann
2016-05-13 15:28           ` Arnd Bergmann
2016-05-23  8:22           ` Christoph Hellwig
2016-05-23  9:33           ` David Howells
2016-05-18 10:55         ` David Howells
2016-05-09 13:00   ` David Howells
2016-05-09 13:00     ` David Howells
2016-05-09 13:38   ` David Howells
2016-05-10  7:08     ` Christoph Hellwig
2016-05-10  8:43     ` David Howells
2016-05-12  9:12       ` Christoph Hellwig
2016-05-09 13:40   ` David Howells
2016-04-29 12:57 ` [PATCH 2/6] statx: AFS: Return enhanced file attributes David Howells
2016-04-29 12:57 ` [PATCH 3/6] statx: Ext4: " David Howells
2016-05-02 22:48   ` Andreas Dilger
2016-05-03 20:24   ` David Howells
2016-05-03 20:24     ` David Howells
2016-05-08  8:38   ` Christoph Hellwig
2016-05-08  8:38     ` Christoph Hellwig
2016-04-29 12:58 ` [PATCH 4/6] statx: NFS: " David Howells
2016-05-02 22:48   ` Andreas Dilger
2016-04-29 12:58 ` [PATCH 5/6] statx: Make windows attributes available for CIFS, NTFS and FAT to use David Howells
2016-05-02 22:52   ` Andreas Dilger
2016-10-03 21:03     ` Steve French
2016-10-03 21:03       ` Steve French
2016-05-03 20:23   ` David Howells
2016-05-08  8:39   ` Christoph Hellwig
2016-05-08  8:39     ` Christoph Hellwig
2016-04-29 12:58 ` [PATCH 6/6] statx: CIFS: Return enhanced attributes David Howells
2016-04-30 21:05 ` [RFC][PATCH 0/6] Enhanced file stat system call Jeff Layton
2016-04-30 21:05   ` Jeff Layton
2016-05-04 13:46 ` Arnd Bergmann
2016-05-04 13:46   ` Arnd Bergmann
2016-05-05 22:54   ` Steve French
2016-05-06  2:00     ` Steve French
2016-05-09 13:09       ` Arnd Bergmann
2016-05-09 13:09         ` Arnd Bergmann
2016-05-13 14:28         ` Richard Sharpe
2016-05-13 14:28           ` Richard Sharpe
2016-05-13 15:08           ` Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1462795378.4481.31.camel@poochiereds.net \
    --to=jlayton@poochiereds.net \
    --cc=dhowells@redhat.com \
    --cc=hch@infradead.org \
    --cc=linux-afs@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=samba-technical@lists.samba.org \
    --subject='Re: [PATCH 1/6] statx: Add a system call to make enhanced file info available' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.