All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: David Howells <dhowells@redhat.com>
Cc: Andreas Dilger <adilger@dilger.ca>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available
Date: Sun, 20 Nov 2016 09:43:31 +1100	[thread overview]
Message-ID: <20161119224331.GE31101@dastard> (raw)
In-Reply-To: <11317.1479509642@warthog.procyon.org.uk>

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

  reply	other threads:[~2016-11-19 22:43 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-17 13:34 [RFC][PATCH 0/4] Enhanced file stat system call David Howells
2016-11-17 13:35 ` [PATCH 1/4] statx: Add a system call to make enhanced file info available David Howells
2016-11-17 18:39   ` Jeff Layton
2016-11-18  2:32     ` Andreas Dilger
2016-11-18  8:59     ` David Howells
2016-11-18  8:59       ` David Howells
2016-11-18  9:25       ` Andreas Dilger
2016-11-18  9:25         ` Andreas Dilger
2016-11-17 23:40   ` Dave Chinner
2016-11-18  3:28     ` Andreas Dilger
2016-11-18 22:07       ` Dave Chinner
2016-11-18 22:54       ` David Howells
2016-11-19 22:43         ` Dave Chinner [this message]
2016-11-21 14:30         ` One Thousand Gnomes
2016-11-21 20:43           ` Dave Chinner
2016-11-22 10:39         ` David Howells
2016-11-22 13:55           ` Jeff Layton
2016-11-22 20:58           ` Dave Chinner
2016-11-18  9:53     ` David Howells
2016-11-18  8:48   ` David Howells
2016-11-18 12:01     ` Jeff Layton
2016-11-18  9:36   ` David Howells
2016-11-18 17:17     ` Jeff Layton
2016-11-18 18:04     ` David Howells
2016-11-18 18:54       ` Jeff Layton
2016-11-18 19:08       ` David Howells
2016-11-18  9:43   ` David Howells
2016-11-18 21:41     ` Dave Chinner
2016-11-18 22:24     ` David Howells
2016-11-18 10:29   ` David Howells
2016-11-18 10:29     ` David Howells
2016-11-18 21:27     ` Dave Chinner
2016-11-18 21:48     ` David Howells
2016-11-18 21:48       ` David Howells
2016-11-18 22:17       ` Dave Chinner
2016-11-18 22:17         ` Dave Chinner
2016-11-19 10:21         ` Michael Kerrisk (man-pages)
2016-11-17 13:35 ` [PATCH 2/4] statx: Ext4: Return enhanced file attributes David Howells
2016-11-18  3:30   ` Andreas Dilger
2016-11-17 13:35 ` [PATCH 3/4] statx: NFS: " David Howells
2016-11-17 13:35 ` [PATCH 4/4] statx: AFS: " David Howells
2016-11-18  3:34   ` Andreas Dilger
2016-11-18  8:47   ` David Howells
2016-11-17 14:39 ` [RFC][PATCH 0/4] Enhanced file stat system call One Thousand Gnomes
2016-11-17 15:10 ` Michael Kerrisk
2016-11-17 16:33 ` David Howells
2016-11-17 16:45 ` David Howells
2016-11-17 20:00   ` J. Bruce Fields
2016-11-18  2:30     ` Andreas Dilger
2016-11-18  4:29       ` NeilBrown
2016-11-18 13:41   ` One Thousand Gnomes
2016-11-18 13:49   ` David Howells

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=20161119224331.GE31101@dastard \
    --to=david@fromorbit.com \
    --cc=adilger@dilger.ca \
    --cc=dhowells@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 1/4] 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.