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

[David Howells <dhowells@redhat.com>]

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