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

Groups > linux.kernel > #1524445 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/4] Enhanced file stat system call David Howells <dhowells@redhat.com> - 2016-11-17 15:00 +0100
    Re: [RFC][PATCH 0/4] Enhanced file stat system call David Howells <dhowells@redhat.com> - 2016-11-17 18:10 +0100
    Re: [RFC][PATCH 0/4] Enhanced file stat system call David Howells <dhowells@redhat.com> - 2016-11-17 18:10 +0100
      Re: [RFC][PATCH 0/4] Enhanced file stat system call bfields@fieldses.org (J. Bruce Fields) - 2016-11-17 21:10 +0100
        Re: [RFC][PATCH 0/4] Enhanced file stat system call Andreas Dilger <adilger@dilger.ca> - 2016-11-18 03:50 +0100
          Re: [RFC][PATCH 0/4] Enhanced file stat system call NeilBrown <neilb@suse.com> - 2016-11-18 05:40 +0100
      Re: [RFC][PATCH 0/4] Enhanced file stat system call One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-11-18 14:50 +0100
        Re: [RFC][PATCH 0/4] Enhanced file stat system call David Howells <dhowells@redhat.com> - 2016-11-18 14:50 +0100
    Re: [RFC][PATCH 0/4] Enhanced file stat system call Michael Kerrisk <mtk.manpages@gmail.com> - 2016-11-17 18:20 +0100
    [PATCH 3/4] statx: NFS: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-11-17 18:20 +0100
    [PATCH 2/4] statx: Ext4: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-11-17 18:20 +0100
      Re: [PATCH 2/4] statx: Ext4: Return enhanced file attributes Andreas Dilger <adilger@dilger.ca> - 2016-11-18 04:40 +0100
    [PATCH 4/4] statx: AFS: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-11-17 18:20 +0100
      Re: [PATCH 4/4] statx: AFS: Return enhanced file attributes Andreas Dilger <adilger@dilger.ca> - 2016-11-18 09:30 +0100
        Re: [PATCH 4/4] statx: AFS: Return enhanced file attributes David Howells <dhowells@redhat.com> - 2016-11-18 09:50 +0100
    Re: [RFC][PATCH 0/4] Enhanced file stat system call One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-11-17 19:00 +0100
    Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-18 00:50 +0100
      Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available Andreas Dilger <adilger@dilger.ca> - 2016-11-18 04:30 +0100
        Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 11:00 +0100
        Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-18 23:10 +0100
          Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-19 00:00 +0100
            Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-19 23:50 +0100
              Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-22 11:40 +0100
                Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Jeff Layton <jlayton@redhat.com> - 2016-11-22 15:00 +0100
                Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-22 22:00 +0100
            Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-11-21 15:40 +0100
              Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-21 21:50 +0100
      Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 10:40 +0100
        Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Jeff Layton <jlayton@poochiereds.net> - 2016-11-18 18:20 +0100
          Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 19:10 +0100
            Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Jeff Layton <jlayton@redhat.com> - 2016-11-18 20:00 +0100
              Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 20:10 +0100
      Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 10:50 +0100
        Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-18 22:50 +0100
          Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 23:30 +0100
      Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 11:30 +0100
        Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-18 22:30 +0100
          Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 22:50 +0100
            Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Dave Chinner <david@fromorbit.com> - 2016-11-18 23:20 +0100
              Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2016-11-19 11:30 +0100
    Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 09:50 +0100
      Re: [PATCH 1/4] statx: Add a system call to make enhanced file info  available Jeff Layton <jlayton@redhat.com> - 2016-11-18 13:10 +0100
    Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells <dhowells@redhat.com> - 2016-11-18 10:00 +0100
      Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available Andreas Dilger <adilger@dilger.ca> - 2016-11-18 10:30 +0100

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

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

Dave Chinner <david@fromorbit.com> wrote:

> And when we start thinking in those timeframes, an
> increase in timestamp resoultion of at least another 10e-3 is
> likely....

Is it, though?  To be useful, surely you have to be able to jam quite a few
instructions into a 1ns block, including memory accesses.

Rather than providing:

	struct timestamp {
		__s64 seconds;
		__s64 femtoseconds;
	};

which would require 64-bit divisions to get nanosecond timestamps that we do
actually use, I would lean towards:

	struct timestamp {
		__s64 seconds;
		__s32 nanoseconds;
		__s32 femtoseconds;
	};

where the fields are, in effect, additive.  Which means I could represent this
as:

	__s64	stx_atime_s;	/* Last access time */
	__s64	stx_btime_s;	/* File creation time */
	__s64	stx_ctime_s;	/* Last attribute change time */
	__s64	stx_mtime_s;	/* Last data modification time */
	__s32	stx_atime_ns;	/* Last access time (ns part) */
	__s32	stx_btime_ns;	/* File creation time (ns part) */
	__s32	stx_ctime_ns;	/* Last attribute change time (ns part) */
	__s32	stx_mtime_ns;	/* Last data modification time (ns part) */

and then add:

	__s32	stx_atime_fs;	/* Last access time (fs part) */
	__s32	stx_btime_fs;	/* File creation time (fs part) */
	__s32	stx_ctime_fs;	/* Last attribute change time (fs part) */
	__s32	stx_mtime_fs;	/* Last data modification time (fs part) */

later.

If we *really* do want to allow for atto- or femto- second resolution
timestamps (and you've still not entirely convinced me that it's going to be
necessary - the speed of signal propagation still has an ungetroundable
limit), then we could stick the space in now - but I think it's likely to
remain dead space.

Maybe we should switch to Windows-style timestamp resolution:

	struct timestamp {
		__s64 hundred_ns;	/* Time in 100ns increments */
		__s32 femtoseconds;	/* Additional fs component */
	};

> > Using the existing FS_*_FL flags as initial values is not worse than
> > starting with any other arbitrary values for the flags.
>
> Except it starts with a sparse set of flags for no good reason.

Actually, a very good reason.  You can map those flags, on ext4 at least, with
a load, an AND and an OR.  Three instructions[*].  If the bits don't
correspond, it gets more expensive (4-5 instructions per bit + 1).

[*] Leastways, it *should* be three instructions, but gcc fails to optimise it
    correctly.  I have a bz logged for this.

David

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

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

On Fri, Nov 18, 2016 at 10:54:02PM +0000, David Howells wrote:
> Dave Chinner <david@fromorbit.com> wrote:
> 
> > And when we start thinking in those timeframes, an
> > increase in timestamp resoultion of at least another 10e-3 is
> > likely....
> 
> Is it, though?  To be useful, surely you have to be able to jam quite a few
> instructions into a 1ns block, including memory accesses.
> 
> Rather than providing:
> 
> 	struct timestamp {
> 		__s64 seconds;
> 		__s64 femtoseconds;
> 	};
> 
> which would require 64-bit divisions to get nanosecond timestamps that we do
> actually use, I would lean towards:
> 
> 	struct timestamp {
> 		__s64 seconds;
> 		__s32 nanoseconds;
> 		__s32 femtoseconds;
> 	};

No. Just provide a 64 bit high resoultion field, and define it to
contain nanoseconds. When we need higher resolution to be exported
to userspace, we use a /feature flag/ to indicate that is contains
something like attoseconds or the like.

This also gives userspace a method of choosing the resolution it
wants.....

> 	__s32	stx_btime_ns;	/* File creation time (ns part) */
> 	__s32	stx_ctime_ns;	/* Last attribute change time (ns part) */
> 	__s32	stx_mtime_ns;	/* Last data modification time (ns part) */
> 
> and then add:
> 
> 	__s32	stx_atime_fs;	/* Last access time (fs part) */
> 	__s32	stx_btime_fs;	/* File creation time (fs part) */
> 	__s32	stx_ctime_fs;	/* Last attribute change time (fs part) */
> 	__s32	stx_mtime_fs;	/* Last data modification time (fs part) */
> 
> later.

Which burns a significant part of the spare space in the structure.

> If we *really* do want to allow for atto- or femto- second resolution
> timestamps (and you've still not entirely convinced me that it's going to be
> necessary - the speed of signal propagation still has an ungetroundable
> limit), then we could stick the space in now - but I think it's likely to
> remain dead space.

We don't really care if the strucuture 250 bytes or 300 bytes, it's
not going to have any significant impact on memory usage or
performance. We should just suck it up now and future
proof the timestamp interface, as history has proven repeatedly that
we suck as making our time/timestamp interfaces future proof....

> > > Using the existing FS_*_FL flags as initial values is not worse than
> > > starting with any other arbitrary values for the flags.
> >
> > Except it starts with a sparse set of flags for no good reason.
> 
> Actually, a very good reason.  You can map those flags, on ext4 at least, with
> a load, an AND and an OR.  Three instructions[*].  If the bits don't
> correspond, it gets more expensive (4-5 instructions per bit + 1).

Two words: premature optimisation.

Every other filesystem has to map their own internal flags to the
user interface flags, so why should we make the user interface
harder to understand and maintain just to allow a special snowflake
optimisation for /only one filesystem/?

There's a worse problem than that, though - we can't add certain
flags to the userspace API because the /conflict with ext4 on-disk
flags/ that aren't exported to userspace and so to work around that
we have this shitty hack to define what parts of the flag space
contain flags that the userspace API can interact with:

#define FS_FL_USER_VISIBLE              0x0003DFFF /* User visible flags */
#define FS_FL_USER_MODIFIABLE           0x000380FF /* User modifiable flags */

These mask out the ext2/3/4 flags that should not be visible to
userspace and/or not be modifiable by userspace. Having to maintain
crap like this is a direct result of exporting on-disk flag values
to userspace APIs.

*Let's not repeat the mistakes of the past* in new interfaces.

Cheers,

Dave.

> 
> [*] Leastways, it *should* be three instructions, but gcc fails to optimise it
>     correctly.  I have a bz logged for this.
> 
> David
> 

-- 
Dave Chinner
david@fromorbit.com

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

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

Dave Chinner <david@fromorbit.com> wrote:

> No. Just provide a 64 bit high resoultion field, and define it to
> contain nanoseconds. When we need higher resolution to be exported
> to userspace, we use a /feature flag/ to indicate that is contains
> something like attoseconds or the like.

That sounds suspiciously like a bad idea - if you're talking about a flag with
a currently undefined meaning that the kernel can inflict on userspace without
warning to change the meaning of the nanoseconds field to something we haven't
defined yet.

Userspace would have to ask for it.

David

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

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

On Tue, 2016-11-22 at 10:39 +0000, David Howells wrote:
> Dave Chinner <david@fromorbit.com> wrote:
> 
> > 
> > No. Just provide a 64 bit high resoultion field, and define it to
> > contain nanoseconds. When we need higher resolution to be exported
> > to userspace, we use a /feature flag/ to indicate that is contains
> > something like attoseconds or the like.
> 
> That sounds suspiciously like a bad idea - if you're talking about a flag with
> a currently undefined meaning that the kernel can inflict on userspace without
> warning to change the meaning of the nanoseconds field to something we haven't
> defined yet.
> 
> Userspace would have to ask for it.
> 

Agreed.

I think the best thing would be to simply plan to add new femtoseconds
fields in the struct if/when it becomes needed. We can easily hide the
ugliness of the fields not being adjacent to the rest of the timestamp
behind the femtosecond resolution glibc API that will also be needed.

If we're worried about space utilization, then let's pad the struct out
by 4 extra 32-bit fields in advance of that.

Again, a major design point of statx is that it is to be extendable. I
don't see any reason that we need to do this now.

-- 
Jeff Layton <jlayton@redhat.com>

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

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

On Tue, Nov 22, 2016 at 10:39:29AM +0000, David Howells wrote:
> Dave Chinner <david@fromorbit.com> wrote:
> 
> > No. Just provide a 64 bit high resoultion field, and define it to
> > contain nanoseconds. When we need higher resolution to be exported
> > to userspace, we use a /feature flag/ to indicate that is contains
> > something like attoseconds or the like.
> 
> That sounds suspiciously like a bad idea - if you're talking about a flag with
> a currently undefined meaning that the kernel can inflict on userspace without
> warning to change the meaning of the nanoseconds field to something we haven't
> defined yet.
> 
> Userspace would have to ask for it.

Yes, of course it would - this would enable userspace to move from
struct timespec to something with higher resolution without having
to use different structures or guess what the resolution being
returned by the kernel is for different filesystems,

We had a major mess with the time_t -> struct timespec upgrade of
the stat() kernel interface to support nanosecond timestamps in the
syscall because when stat() was first designed  all those years ago
single second resolution was all anyone needed. The original
designers of the stat API didn't have 30+ years of history telling
them that machines will get faster than anyone could imagine and
that timestamps will always get more accurate and increase in
resolution.

Perhaps people are fine with repeating past mistakes - all I can do
is point them out and suggest alternative approaches that will avoid
a repeat...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

> > increase in timestamp resoultion of at least another 10e-3 is
> > likely....  
> 
> Is it, though?  To be useful, surely you have to be able to jam quite a few
> instructions into a 1ns block, including memory accesses.
> 
> Rather than providing:
> 
> 	struct timestamp {
> 		__s64 seconds;
> 		__s64 femtoseconds;
> 	};
> 
> which would require 64-bit divisions to get nanosecond timestamps that we do
> actually use, I would lean towards:
> 
> 	struct timestamp {
> 		__s64 seconds;
> 		__s32 nanoseconds;
> 		__s32 femtoseconds;
> 	};

Which gets silly. The nanosecond world is defined by the speed of light.
Short of someone finding a way to change that digital computing as we
know it today is going to be living in the nanoseconds world. You hit the
point of 'can't measure the difference' before you hit the point of 'can
usefully order things using'

Alan

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

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

On Mon, Nov 21, 2016 at 02:30:13PM +0000, One Thousand Gnomes wrote:
> > > increase in timestamp resoultion of at least another 10e-3 is
> > > likely....  
> > 
> > Is it, though?  To be useful, surely you have to be able to jam quite a few
> > instructions into a 1ns block, including memory accesses.
> > 
> > Rather than providing:
> > 
> > 	struct timestamp {
> > 		__s64 seconds;
> > 		__s64 femtoseconds;
> > 	};
> > 
> > which would require 64-bit divisions to get nanosecond timestamps that we do
> > actually use, I would lean towards:
> > 
> > 	struct timestamp {
> > 		__s64 seconds;
> > 		__s32 nanoseconds;
> > 		__s32 femtoseconds;
> > 	};
> 
> Which gets silly. The nanosecond world is defined by the speed of light.
> Short of someone finding a way to change that digital computing as we
> know it today is going to be living in the nanoseconds world. You hit the
> point of 'can't measure the difference' before you hit the point of 'can
> usefully order things using'

We already have clock rates that are fractions of a nanosecond per
cycle. We have pmem storage here right now that is accessed at the
speed of the CPU - actual nanosecond resolution timestamp capability
is a reality at the bleeding edge of storage technology right now.

It doesn't take much vision to extend the current hardare
capabilities with coherent hardware accelerators (e.g. as has been
added to the Power platform) writing directly into pmem storage and
providing higher resolution timestamps than the CPU can generate.

Call me silly if you want - I don't care - but let's not ignore the
emerging storage technology trends that are there for everyone to
see...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

Dave Chinner <david@fromorbit.com> wrote:

> >  (5) Data version number: Could be used by userspace NFS servers [Aneesh
> >      Kumar].
> > 
> >      Can also be used to modify fill_post_wcc() in NFSD which retrieves
> >      i_version directly, but has just called vfs_getattr().  It could get
> >      it from the kstat struct if it used vfs_xgetattr() instead.
> 
> This needs a much clearer name that stx_version as "version" is
> entirely ambiguous. e.g. Inodes have internal version numbers to
> disambiguate life cycles. and there are versioning filesystems
> which have multiple versions of the same file. 

We've already been through that.  I wanted to call it stx_data_version but
that got argued down to stx_version.  The problem is that what the version
number means is entirely filesystem dependent, and it might not just reflect
changes in the data.

> So if stx_version this is intended to export the internal filesystem
> inode change counter (i.e. inode->i_version) then lets call it that:
> stx_modification_count. It's clear and unambiguous as to what it
> represents, especially as this counter is more than just a "data
> modification" counter - inode metadata modifications will also
> cause it to change....

I disagree that it's unambiguous.  It works like mtime, right?

Which wouldn't be of use for certain filesystems.  An example of this would be
AFS, where it's incremented by 1 each time a write is committed, but is not
updated for metadata changes.  This is what matters for data caching.

> > (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. It makes little
> sense to now add xstat() and not add everything this interface
> also exports...

I'm not sure I agree.  Stuff like extent sizes and extent hint flags seem like
very specialised things that don't belong in the stat interface.  The project
ID, on the other hand is arguably a good thing to include.  But we can always
add this later.

Note that are also two variants of the fsxattr struct defined in the kernel -
though one is a superset of the other.

> > 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 will also be negative if not zero.
> 
> So what happens in ten years time when we want to support
> femptosecond resolution in the timestamp interface? We've got to
> change everything to 64 bit? Shouldn't we just make everything
> timestamp related 64 bit?

You really think we're going to have accurate timestamps with a resolution of
a millionth of a nanosecond?  This means you're going to be doing a 64-bit
division every time you want a nanosecond timestamp.

Also, violet light has a period of ~1.2fs so your 1fs oscillator might emit UV
radiation.

> > The bits defined in the stx_attributes field convey information about a
> > file, how it is accessed, where it is and what it does.  The following
> > attributes map to FS_*_FL flags and are the same numerical value:
> 
> Please isolate the new interface flags completely from the FS_*_FL
> values. We should not repeat the mistake of tying values derived
> from filesystem specific on-disk values to a user interface. 

Why shouldn't I make a numerical correspondance with at least one set of such
flags?  I get to define a whole new numberspace and can pick the values I
want.  I see no particular reason to pick explicitly non-corresponding values
where possible.

Now, I can agree that the code should say, for example:

	if (ext4->flag & FS_COMPRESSED_FL)
		statx.attr |= STATX_ATTR_COMPRESSED;
	if (ext4->flag & FS_ENCRYPTED_FL)
		statx.attr |= STATX_ATTR_ENCRYPTED;
	if (ext4->flag & FS_IMMUTABLE_FL)
		statx.attr |= STATX_ATTR_IMMUTABLE;
	...

and that the *compiler* should collapse this to:

	statx.attr |= ext4->flag & mask;

but see:

	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78317

David

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

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

On Fri, 2016-11-18 at 09:36 +0000, David Howells wrote:
> Dave Chinner <david@fromorbit.com> wrote:
> 
> > >  (5) Data version number: Could be used by userspace NFS servers [Aneesh
> > >      Kumar].
> > > 
> > >      Can also be used to modify fill_post_wcc() in NFSD which retrieves
> > >      i_version directly, but has just called vfs_getattr().  It could get
> > >      it from the kstat struct if it used vfs_xgetattr() instead.
> > 
> > This needs a much clearer name that stx_version as "version" is
> > entirely ambiguous. e.g. Inodes have internal version numbers to
> > disambiguate life cycles. and there are versioning filesystems
> > which have multiple versions of the same file. 
> 
> We've already been through that.  I wanted to call it stx_data_version but
> that got argued down to stx_version.  The problem is that what the version
> number means is entirely filesystem dependent, and it might not just reflect
> changes in the data.
> 

It had better not just reflect data changes.

knfsd populates the NFSv4 change attribute from inode->i_version. It
_must_ have changed between subsequent queries if either the data or
metadata has changed (basically whenever you would update either the
ctime or the mtime).


> > So if stx_version this is intended to export the internal filesystem
> > inode change counter (i.e. inode->i_version) then lets call it that:
> > stx_modification_count. It's clear and unambiguous as to what it
> > represents, especially as this counter is more than just a "data
> > modification" counter - inode metadata modifications will also
> > cause it to change....
> 
> I disagree that it's unambiguous.  It works like mtime, right?
> 

More like ctime + mtime mashed together.

> Which wouldn't be of use for certain filesystems.  An example of this would be
> AFS, where it's incremented by 1 each time a write is committed, but is not
> updated for metadata changes.  This is what matters for data caching.
> 

No. Basically the rules are that if something in the inode data or
metadata changed, then it must be a "larger" value (also accounting for
wraparound). So you also need to change it (usually by incrementing it)
when doing namespace changes that involve it (renames, unlinks, etc.).

> > > (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. It makes little
> > sense to now add xstat() and not add everything this interface
> > also exports...
> 
> I'm not sure I agree.  Stuff like extent sizes and extent hint flags seem like
> very specialised things that don't belong in the stat interface.  The project
> ID, on the other hand is arguably a good thing to include.  But we can always
> add this later.
> 

Yes. The entire point of this interface is that it is extendable.

I'm all for simplicity here and just adding the bare minimum of fields
to get the new interface in. The main thing we must do here though is
to ID anything that would hobble us from being able to add new
attributes later.

Adding new fields in later piecemeal patches allows us to demonstrate
that that concept actually works.

> Note that are also two variants of the fsxattr struct defined in the kernel -
> though one is a superset of the other.
> 
> > > 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 will also be negative if not zero.
> > 
> > So what happens in ten years time when we want to support
> > femptosecond resolution in the timestamp interface? We've got to
> > change everything to 64 bit? Shouldn't we just make everything
> > timestamp related 64 bit?
> 
> You really think we're going to have accurate timestamps with a resolution of
> a millionth of a nanosecond?  This means you're going to be doing a 64-bit
> division every time you want a nanosecond timestamp.
> 
> Also, violet light has a period of ~1.2fs so your 1fs oscillator might emit UV
> radiation.
> 

Could contemporary machines get away with just shifting down by 32
bits?

> > > The bits defined in the stx_attributes field convey information about a
> > > file, how it is accessed, where it is and what it does.  The following
> > > attributes map to FS_*_FL flags and are the same numerical value:
> > 
> > Please isolate the new interface flags completely from the FS_*_FL
> > values. We should not repeat the mistake of tying values derived
> > from filesystem specific on-disk values to a user interface. 
> 
> Why shouldn't I make a numerical correspondance with at least one set of such
> flags?  I get to define a whole new numberspace and can pick the values I
> want.  I see no particular reason to pick explicitly non-corresponding values
> where possible.
> 
> Now, I can agree that the code should say, for example:
> 
> 	if (ext4->flag & FS_COMPRESSED_FL)
> 		statx.attr |= STATX_ATTR_COMPRESSED;
> 	if (ext4->flag & FS_ENCRYPTED_FL)
> 		statx.attr |= STATX_ATTR_ENCRYPTED;
> 	if (ext4->flag & FS_IMMUTABLE_FL)
> 		statx.attr |= STATX_ATTR_IMMUTABLE;
> 	...
> 
> and that the *compiler* should collapse this to:
> 
> 	statx.attr |= ext4->flag & mask;
> 
> but see:
> 
> 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78317
> 
> David
> --
> 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

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

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

Jeff Layton <jlayton@poochiereds.net> wrote:

> > We've already been through that.  I wanted to call it stx_data_version but
> > that got argued down to stx_version.  The problem is that what the version
> > number means is entirely filesystem dependent, and it might not just reflect
> > changes in the data.
> > 
> 
> It had better not just reflect data changes.
> 
> knfsd populates the NFSv4 change attribute from inode->i_version. It
> _must_ have changed between subsequent queries if either the data or
> metadata has changed (basically whenever you would update either the
> ctime or the mtime).

No, I think it *should* just reflect the data changes - otherwise you have
have to burn your cached data unnecessarily.

> > > So if stx_version this is intended to export the internal filesystem
> > > inode change counter (i.e. inode->i_version) then lets call it that:
> > > stx_modification_count. It's clear and unambiguous as to what it
> > > represents, especially as this counter is more than just a "data
> > > modification" counter - inode metadata modifications will also
> > > cause it to change....
> > 
> > I disagree that it's unambiguous.  It works like mtime, right?
> 
> More like ctime + mtime mashed together.

Isn't ctime updated every time mtime is?  In which case stx_change_count would
be a better name.

> > Which wouldn't be of use for certain filesystems.  An example of this
> > would be AFS, where it's incremented by 1 each time a write is committed,
> > but is not updated for metadata changes.  This is what matters for data
> > caching.
> > 
> 
> No. Basically the rules are that if something in the inode data or
> metadata changed, then it must be a "larger" value (also accounting for
> wraparound). So you also need to change it (usually by incrementing it)
> when doing namespace changes that involve it (renames, unlinks, etc.).

That's entirely filesystem dependent.

A better rule is that if you do a write and then compare the data version you
got back to the version you had before; if it's increased by exactly one,
there were no other writes between your last retrieval of the attributes and
your write that just got committed.  Admittedly, this assumes that the server
serialises writes to a particular file.

If the value just increases, you don't know that didn't happen by this
mechanism, so the version is of limited value.

> Adding new fields in later piecemeal patches allows us to demonstrate
> that that concept actually works.

You're probably right, but the downside is that we really need some way to
find out what's supported.  On the other hand, we probably need that anyway,
hence my suggestion of an fsinfo() syscall also.

> > You really think we're going to have accurate timestamps with a resolution
> > of a millionth of a nanosecond?  This means you're going to be doing a
> > 64-bit division every time you want a nanosecond timestamp.
> ...
> 
> Could contemporary machines get away with just shifting down by 32
> bits?

A better way would probably be to have:

	struct timestamp {
		__u64 seconds;
		__u32 nanoseconds;
		__u32 femtoseconds;
	};

where you effectively add all the fields together with appropriate
multipliers.

But I still wonder if we really are going to move to femtosecond timestamps,
given that that's going to involve clock frequencies well in excess of 1 THz
to be useful.  Even attoseconds is probably unnecessary, given that clock
frequencies don't seem to be moving much beyond a few GHz, though it's
reasonable that we could have a timestamp counter that has an attosecond
period - it's just that the processing time to deal with it seems likely to
render it unnecessary.

David

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

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

On Fri, 2016-11-18 at 18:04 +0000, David Howells wrote:
> Jeff Layton <jlayton@poochiereds.net> wrote:
> 
> > > We've already been through that.  I wanted to call it stx_data_version but
> > > that got argued down to stx_version.  The problem is that what the version
> > > number means is entirely filesystem dependent, and it might not just reflect
> > > changes in the data.
> > > 
> > 
> > It had better not just reflect data changes.
> > 
> > knfsd populates the NFSv4 change attribute from inode->i_version. It
> > _must_ have changed between subsequent queries if either the data or
> > metadata has changed (basically whenever you would update either the
> > ctime or the mtime).
> 
> No, I think it *should* just reflect the data changes - otherwise you have
> have to burn your cached data unnecessarily.
> 
> > > > So if stx_version this is intended to export the internal filesystem
> > > > inode change counter (i.e. inode->i_version) then lets call it that:
> > > > stx_modification_count. It's clear and unambiguous as to what it
> > > > represents, especially as this counter is more than just a "data
> > > > modification" counter - inode metadata modifications will also
> > > > cause it to change....
> > > 
> > > I disagree that it's unambiguous.  It works like mtime, right?
> > 
> > More like ctime + mtime mashed together.
> 
> Isn't ctime updated every time mtime is?  In which case stx_change_count would
> be a better name.
> 
> > > Which wouldn't be of use for certain filesystems.  An example of this
> > > would be AFS, where it's incremented by 1 each time a write is committed,
> > > but is not updated for metadata changes.  This is what matters for data
> > > caching.
> > > 
> > 
> > No. Basically the rules are that if something in the inode data or
> > metadata changed, then it must be a "larger" value (also accounting for
> > wraparound). So you also need to change it (usually by incrementing it)
> > when doing namespace changes that involve it (renames, unlinks, etc.).
> 
> That's entirely filesystem dependent.
> 

My mistake. I had thought that i_version was only used for NFSv4, and a
few internal callers (particularly, some readdir implementations). I
didn't realize that AFS also uses it.

> A better rule is that if you do a write and then compare the data version you
> got back to the version you had before; if it's increased by exactly one,
> there were no other writes between your last retrieval of the attributes and
> your write that just got committed.  Admittedly, this assumes that the server
> serialises writes to a particular file.
> 
> If the value just increases, you don't know that didn't happen by this
> mechanism, so the version is of limited value.
> 

For the case of NFSv4, you can't infer that anyway. The protocol pretty
much states that the client has to treat this value as semi-opaque. It
can't infer anything other than "something has changed" (though it can
look to see if a change attribute is "old" and discard it).

Does AFS allow you to infer something from the actual value?

Now that I realize that AFS has very different semantics, we might want
to step back for a bit on presenting i_version to userspace. I think we
need to come to some agreement on what i_version should actually mean
before we expose it to userland to use.

Maybe we should consider separate stx_change_attr and
stx_data_change_attr fields in here?

> > Adding new fields in later piecemeal patches allows us to demonstrate
> > that that concept actually works.
> 
> You're probably right, but the downside is that we really need some way to
> find out what's supported.  On the other hand, we probably need that anyway,
> hence my suggestion of an fsinfo() syscall also.
> 

Yeah, I think we will need an fsinfo call of some sort eventually.

Alternately, we could just add the fields of interest to statx so that
the callers can just query for it with the other fields (e.g.
STATX_TS_GRANULARITY). Just document those attributes as being per-
mount or whatever.

> > > You really think we're going to have accurate timestamps with a resolution
> > > of a millionth of a nanosecond?  This means you're going to be doing a
> > > 64-bit division every time you want a nanosecond timestamp.
> > 
> > ...
> > 
> > Could contemporary machines get away with just shifting down by 32
> > bits?
> 
> A better way would probably be to have:
> 
> 	struct timestamp {
> 		__u64 seconds;
> 		__u32 nanoseconds;
> 		__u32 femtoseconds;
> 	};
> 
> where you effectively add all the fields together with appropriate
> multipliers.
> 

Harder for those femtosecond scale machines to deal with, but maybe
you're right.

If the plan is to do that then we can just punt that out until that
need arises, and add stx_?time_fsec fields at that point. The ugliness
would all be hidden behind the glibc wrapper anyway (in principle).

> But I still wonder if we really are going to move to femtosecond timestamps,
> given that that's going to involve clock frequencies well in excess of 1 THz
> to be useful.  Even attoseconds is probably unnecessary, given that clock
> frequencies don't seem to be moving much beyond a few GHz, though it's
> reasonable that we could have a timestamp counter that has an attosecond
> period - it's just that the processing time to deal with it seems likely to
> render it unnecessary.
> 
> David

True, and we have to figure here that these are _file_ times, so you'd
have to have file updates happening on sub-nanosecond timescales for
this to make sense as well.

I don't think we really need to add this now, especially if we can add
the extra fields if/when the need ever arises.

-- 
Jeff Layton <jlayton@redhat.com>

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

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

Jeff Layton <jlayton@redhat.com> wrote:

> Does AFS allow you to infer something from the actual value?

Yes.  Given:

 (1) Each write on a file is atomic with respect to all other writes to that
     same file.

 (2) The data version is incremented by exactly one for each write committed,
     and that value is returned in the reply to the write RPC call.

If you think you have a file with version 99, you do a call and get back
version 100, you *know* that there was no conflicting write before your write.

If the version came back as 101, however, you *know* that there was a write
you didn't know about.  Therefore, you have to flush your cache.

David

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

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

Dave Chinner <david@fromorbit.com> wrote:

> > 	STATX_ATTR_KERNEL_API		File is kernel API (eg: procfs/sysfs)
> > 	STATX_ATTR_REMOTE		File is remote and needs network
> > 	STATX_ATTR_FABRICATED		File was made up by fs
> 
> Every file is fabricated by a filesystem :P
> 
> Perhaps you're wanting "virtual file" because it is has no physical
> presence?

Yeah - that might be a better name.  The idea of FABRICATED is to note objects
that have no actual existence in the backing store.  Directories that had to
be invented to act as mountpoints would be a good example of this.

> > Fields in struct statx come in a number of classes:
> > 
> >  (0) stx_dev_*, stx_blksize.
> > 
> >      These are local system information and are always available.
> 
> What does stx_blksize actually mean? It's completely ambiguous in
> stat() because we don't actually report the physical block size
> here - we report the "minimum unit of efficient IO" that we expect
> applications to use. Please define :P

Definition: "Same as struct stat::st_blksize".

> > The following test program can be used to test the statx system call:
> > 
> > 	samples/statx/test-statx.c
> > 
> > Just compile and run, passing it paths to the files you want to examine.
> > The file is built automatically if CONFIG_SAMPLES is enabled.
> 
> Can we get xfstests written to exercise and validate all this
> functionality, please? I'd suggest that adding xfs_io support for
> the statx syscall would be far more useful for xfstests than a
> standalone  test program, too. We already have equivalent stat()
> functionality in xfs_io and that's used quite a bit in xfstests....

Feel free to write some! ;-)

But I need a simple standalone test program to be able to test what I write,
and I've no inclination to wheel out huge testsuites for an interface that
people are still arguing about and wanting changed.

David

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

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

On Fri, Nov 18, 2016 at 09:43:38AM +0000, David Howells wrote:
> Dave Chinner <david@fromorbit.com> wrote:
> > > Fields in struct statx come in a number of classes:
> > > 
> > >  (0) stx_dev_*, stx_blksize.
> > > 
> > >      These are local system information and are always available.
> > 
> > What does stx_blksize actually mean? It's completely ambiguous in
> > stat() because we don't actually report the physical block size
> > here - we report the "minimum unit of efficient IO" that we expect
> > applications to use. Please define :P
> 
> Definition: "Same as struct stat::st_blksize".

So it is still defined as "mostly useless", then? :/

> > > The following test program can be used to test the statx system call:
> > > 
> > > 	samples/statx/test-statx.c
> > > 
> > > Just compile and run, passing it paths to the files you want to examine.
> > > The file is built automatically if CONFIG_SAMPLES is enabled.
> > 
> > Can we get xfstests written to exercise and validate all this
> > functionality, please? I'd suggest that adding xfs_io support for
> > the statx syscall would be far more useful for xfstests than a
> > standalone  test program, too. We already have equivalent stat()
> > functionality in xfs_io and that's used quite a bit in xfstests....
> 
> Feel free to write some! ;-)

No, not my job. It is the responsibility of the person added new
functionality to write the validity tests for everyone else to use.

> But I need a simple standalone test program to be able to test what I write,
> and I've no inclination to wheel out huge testsuites for an interface that
> people are still arguing about and wanting changed.

The test suite should be developed concurrently with the code. You
know, best software engineering practices and all that. Just a small
example: Darrick landed 100+ reflink related tests in xfstests
before we merged the XFS reflink functionality. And that's just for
XFS - this is something that /all filesystems/ need to work
correctly with, so it's even more important that we have tests to
verify functionality /before/ it gets merged.

Quite frankly, I think this has to be an unconditional requirement
for such generic, expandabled new syscall functionality - either we
get test coverage for it before merge, or we don't merge it. We've
demonstrated time and time again that shit doesn't work if it's not
tested and cannot be widely verified by independent filesystem
developers.

Again, I'll use the example of Darrick's reflink tests - that
exposed multiple bugs in the btrfs reflink implementation that
nobody knew existed because /they'd never been tested/. And we've
found several subtle differences in fs implementations that would
seriously confuse applications (e.g. how a range of "0 bytes" is
treated by the filesystem) as a result of adding tests to exercise
these exact semantics.

These are the sorts of issues we need test coverage to avoid, and we
need it before merge, not after.

Cheers,

Dave.

> 
> David
> 

-- 
Dave Chinner
david@fromorbit.com

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

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

Dave Chinner <david@fromorbit.com> wrote:

> > Definition: "Same as struct stat::st_blksize".
> 
> So it is still defined as "mostly useless", then? :/

If we're going to be able to emulate stat() with this, then st_blksize must be
determinable from whatever's in struct statx.  If you can provide something
better for stx_blksize and an algorithm for mapping that to st_blksize, then
please do so.

> The test suite should be developed concurrently with the code. You
> know, best software engineering practices and all that. Just a small
> example: Darrick landed 100+ reflink related tests in xfstests
> before we merged the XFS reflink functionality.

I can't give you tests to merge yet.  Given the amount of bikeshedding that's
taken place on this, I'm glad I *haven't* done the testsuite yet - it would
have much more than doubled the amount of work.  I *still* don't know what the
final form is going to be.  I've chucked out almost everything extra because
every bit has someone who argues with it - and this includes people who argue
against things that have to be there!

Further:

	warthog>ls xfstests-dev/doc
	CHANGES

The documentation is missing.  There's a bit in the top level README, but
there's a whole lot of information that *should* be there - and isn't.

David

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

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

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?

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

David

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

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

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

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

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

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.

David

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

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

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.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

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/

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

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

Back to top | Article view | linux.kernel

csiph-web