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

Groups > linux.kernel > #1354530 > unrolled thread

Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Gregory Farnum <greg@gregs42.com> - 2016-03-09 23:30 +0100
    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-10 00:10 +0100
      Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Ric Wheeler <ricwheeler@gmail.com> - 2016-03-10 16:00 +0100
        Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-10 19:40 +0100
          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-10 22:50 +0100
          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Ric Wheeler <rwheeler@redhat.com> - 2016-03-11 05:50 +0100
            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range  of blocks One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-03-11 15:10 +0100
              Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-11 16:30 +0100
              Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-11 18:30 +0100
                Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Andy Lutomirski <luto@amacapital.net> - 2016-03-11 18:40 +0100
                  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-11 19:30 +0100
                    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-11 23:40 +0100
                      Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-12 01:40 +0100
                        Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-12 01:50 +0100
                          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-12 08:30 +0100
                            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Thomas Schoebel-Theuer <tst@schoebel-theuer.de> - 2016-03-12 11:20 +0100
                          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-14 00:40 +0100
                            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Ric Wheeler <rwheeler@redhat.com> - 2016-03-14 11:40 +0100
                              Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-14 15:50 +0100
                                Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-15 21:20 +0100
                                  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 21:50 +0100
                                    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-15 22:30 +0100
                                    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-15 23:40 +0100
                                      Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-16 00:00 +0100
                                        Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks "Darrick J. Wong" <darrick.wong@oracle.com> - 2016-03-16 03:00 +0100
                                          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Andreas Dilger <adilger@dilger.ca> - 2016-03-16 22:50 +0100
                                            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-17 01:20 +0100
                                              Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Eric Sandeen <esandeen@redhat.com> - 2016-03-17 01:40 +0100
                                                Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-17 02:00 +0100
                                                Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Gregory Farnum <greg@gregs42.com> - 2016-03-17 06:20 +0100
                                                  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-17 13:40 +0100
                                          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-17 02:10 +0100
                                            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks "Darrick J. Wong" <darrick.wong@oracle.com> - 2016-03-17 03:50 +0100
                                      Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-16 00:10 +0100
                                        Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-16 00:20 +0100
                                          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-16 01:10 +0100
                                        Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-16 01:00 +0100
                                          Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-16 01:10 +0100
                                            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Eric Sandeen <esandeen@redhat.com> - 2016-03-16 01:40 +0100
                                              Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Chris Mason <clm@fb.com> - 2016-03-16 02:00 +0100
                                                Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Chris Mason <clm@fb.com> - 2016-03-16 23:30 +0100
                                                  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Ric Wheeler <rwheeler@redhat.com> - 2016-03-17 14:50 +0100
                                  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Eric Sandeen <esandeen@redhat.com> - 2016-03-15 23:40 +0100
                      Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-12 01:40 +0100

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

#1358234

On Tue, Mar 15, 2016 at 1:14 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> Root can still change the group id of a file that has exposed stale
> data and hence make it visible outside of the group based
> containment wall.

Ok, Dave, now you're just being ridiculous.

The issue has never been - and *should* never be - that stale data
cannot get out.

The only issue is that we shouldn't make it ridiculously easy to make
silly mistakes.

There's no "group based containment wall" that is some kind of
absolute protection border.

Put another way: this is not about theoretical leaks - because those
are totally irrelevant (in theory, the original discard writer had
access to all that stale data anyway). This is about making it a
practical interface that doesn't have serious hidden gotchas.

So stop making silly theoretical arguments that make no sense.

We should make sure that we have _practical_ rules that are sensible,
but also not painful enough for the people who want to use this in
_practice_.

Reality trumps everything else.

If google is already using this kind of interface, then that is
_reality_. Take that into account.

             Linus

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

#1358252 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Tue, Mar 15, 2016 at 01:43:01PM -0700, Linus Torvalds wrote:
> Put another way: this is not about theoretical leaks - because those
> are totally irrelevant (in theory, the original discard writer had
> access to all that stale data anyway). This is about making it a
> practical interface that doesn't have serious hidden gotchas.

So there have been two interfaces proposed so far:

1) A mount option which specifies the group id under which a program
must be running in order to have the permission to use
FALLOC_FL_NO_HIDE_STALE flag in the fallocate system call.

2) The program must have read access to the underlying block device
inode under which the file system is mounted in order to have the
permission to use the FALLOC_FL_NO_HIDE_STALE flag in the fallocate
system call.

In both cases, the application has to be make C source code changes to
use this feature, and the system administrator has to set up the
application so it has the privileges to use it.  In the case of #1,
the sysadmin has to specify a mount option as well.

We're doing #1 in production in a very large number of mounted disks
today.  Linus has suggested #2, although there was some concern that
screw-up in the user namespaces configuration could result in
accidentally in a security exposure.  (To which my response is, as
opposed to the gazillions of other security nightmares which the user
namespace makes us vulnerable to?)

Still, my preference is for #1, since the mount option acts as an
additional control for those really paranoid types that seem convinced
that it can't be used safely, and it's what we're doing in production
already.   I'm open to #2 if other people are OK with it, though.

Cheers,

						- Ted

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

#1358303 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Tue, Mar 15, 2016 at 01:43:01PM -0700, Linus Torvalds wrote:
> On Tue, Mar 15, 2016 at 1:14 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > Root can still change the group id of a file that has exposed stale
> > data and hence make it visible outside of the group based
> > containment wall.
> 
> Ok, Dave, now you're just being ridiculous.
> 
> The issue has never been - and *should* never be - that stale data
> cannot get out.

Stale data escaping containment is a security issue. Enabling
generic kernel mechanisms to *enable containment escape* is
fundamentally wrong, and relying on userspace to Do The Right Thing
is even more of a gamble, IMO.

> The only issue is that we shouldn't make it ridiculously easy to make
> silly mistakes.

# sudo rsync -a ....

And now the stale data is on another machine without group-based
access containment.

> There's no "group based containment wall" that is some kind of
> absolute protection border.

Precisely my point - it's being pitched as a generic containment
mechanism, but it really isn't.

> Put another way: this is not about theoretical leaks - because those
> are totally irrelevant (in theory, the original discard writer had
> access to all that stale data anyway). This is about making it a
> practical interface that doesn't have serious hidden gotchas.
> 
> So stop making silly theoretical arguments that make no sense.

It's a practical concern because if we enable this functionality in
fallocate because it will get used by more than just special storage
apps. i.e. this can't be waved away with "only properly managed
applications will use it" arguments.

> We should make sure that we have _practical_ rules that are sensible,
> but also not painful enough for the people who want to use this in
> _practice_.

Of course. The fact is that most of the people discussing this issue
have very little domain specific expertise.

In _practice_, XFS has *always* been able to turn off unwritten
extents and expose stale data.  e.g. see this speed-racer blog from
2003 (first google hit on "xfs bonnie++ optimisation"):

http://everything2.com/title/Filesystem+performance+tweaking+with+XFS+on+Linux

	" The first XFS tweak I'll try relates to XFS' practice of
	adding a flag to all unwritten extents. This is a safety
	feature, and it can be disabled with an option during
	filesystem creation time (mkfs.xfs -d unwritten=0). [....]

	A few improvements, a few setbacks, they're all at the level
	of statistical noise. Disabling unwritten extent flagging
	doesn't seem to be terribly useful here."

I also don't make a habit of publicising the fact that since we
disabled the "-d unwritten=X" mkfs parameter (because of speed racer
blogs such as the above and configuration cargo-culting resulting in
unsuspecting users exposing stale data unintentionally) that the
functionality still exists in the kernel code and that it only takes
a single xfs_db command to turn off unwritten extents in XFS. i.e.
we can easily make fallocate on XFS expose stale data, filesystem
wide, without requiring mount options, kernel or application
modifications.

And, yes, I do know of proprietary and non-public storage
applications that have used this capability for years, even though
it is unsupported and performance benefits have only ever been
marginal.

> Reality trumps everything else.

Yes, it does. The reality is we've enabled people who know what they
are doing to expose stale data through preallocation interfaces on
XFS since 1998 and we haven't required kernel API hacks to do this.

> If google is already using this kind of interface, then that is
> _reality_. Take that into account.

Making Google's hack more widely available through the fallocate
API is entirely dependent on proving that:

	a) the performance problem still exists;
	b) the performance problem exists across multiple
	   filesytsems and is not isolated to just ext4 or one specific
	   set of workloads;
	c) the performance problem cannot be fixed;
	d) ext4 can't implement a simple feature check to turn off
	   unwritten extents similar to XFS; and
	e) if all else fails, that the API hack does not compromise the
	   security of general users unaware that applications might
	   be using this functionality.

a), b), c) and d) have not been demonstrated, discussed or iterated -
we've jumped straight to arguing about e). Before anything else,
we need to work through a)-d) because exposing stale data through a
general purpose API is a *last resort*.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

#1358318 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Wed, Mar 16, 2016 at 09:33:13AM +1100, Dave Chinner wrote:
> 
> Stale data escaping containment is a security issue. Enabling
> generic kernel mechanisms to *enable containment escape* is
> fundamentally wrong, and relying on userspace to Do The Right Thing
> is even more of a gamble, IMO.

We already have generic kernel mechanisms such as "the block device".   P

> It's a practical concern because if we enable this functionality in
> fallocate because it will get used by more than just special storage
> apps. i.e. this can't be waved away with "only properly managed
> applications will use it" arguments.

It requires a mount option.  How is this going to allow random
applications to use this feature, again?

> I also don't make a habit of publicising the fact that since we
> disabled the "-d unwritten=X" mkfs parameter (because of speed racer
> blogs such as the above and configuration cargo-culting resulting in
> unsuspecting users exposing stale data unintentionally) that the
> functionality still exists in the kernel code and that it only takes
> a single xfs_db command to turn off unwritten extents in XFS. i.e.
> we can easily make fallocate on XFS expose stale data, filesystem
> wide, without requiring mount options, kernel or application
> modifications.

So you have something even more dangerous in XFS and it's in the
kernel tree?  Has Red Hat threatened to have a distro-specific patch
to comment out this code to make sure irresponsible users can't use
it?  What I've been suggesting has even more controls that what you
have.  And I've been keeping it as an out-of-tree kernel patch mainly
because you've been arguing that it's such a horrible thing.

> Making Google's hack more widely available through the fallocate
> API is entirely dependent on proving that:

Ceph is about to completely bypass the file system because of your
intransigence, and reimplement a userspace file system.  They seem to
believe it's necessary.  I'll let them make the case, because they
seem to think it's necessary.  And if not, if Linus sides with you,
and doesn't want to take the patch, I'll just keep it as a
Google-specific out-of-tree patch.  I don't *need* to have this thing
upstream.

			    	      	    	   - Ted

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

#1358469 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Tue, Mar 15, 2016 at 06:52:24PM -0400, Theodore Ts'o wrote:
> On Wed, Mar 16, 2016 at 09:33:13AM +1100, Dave Chinner wrote:
> > 
> > Stale data escaping containment is a security issue. Enabling
> > generic kernel mechanisms to *enable containment escape* is
> > fundamentally wrong, and relying on userspace to Do The Right Thing
> > is even more of a gamble, IMO.
> 
> We already have generic kernel mechanisms such as "the block device".   P
> 
> > It's a practical concern because if we enable this functionality in
> > fallocate because it will get used by more than just special storage
> > apps. i.e. this can't be waved away with "only properly managed
> > applications will use it" arguments.
> 
> It requires a mount option.  How is this going to allow random
> applications to use this feature, again?
> 
> > I also don't make a habit of publicising the fact that since we
> > disabled the "-d unwritten=X" mkfs parameter (because of speed racer
> > blogs such as the above and configuration cargo-culting resulting in
> > unsuspecting users exposing stale data unintentionally) that the
> > functionality still exists in the kernel code and that it only takes
> > a single xfs_db command to turn off unwritten extents in XFS. i.e.
> > we can easily make fallocate on XFS expose stale data, filesystem
> > wide, without requiring mount options, kernel or application
> > modifications.
> 
> So you have something even more dangerous in XFS and it's in the
> kernel tree?  Has Red Hat threatened to have a distro-specific patch

xfs_db is the XFS debugger, so you can only enable that bit of functionality
with magical commands, which IMHO isn't much different than people messing
with their ext4 filesystems with debugfs.  You had better know what you're
doing and if you break the filesystem you can eat both pieces. :P

> to comment out this code to make sure irresponsible users can't use
> it?  What I've been suggesting has even more controls that what you
> have.  And I've been keeping it as an out-of-tree kernel patch mainly
> because you've been arguing that it's such a horrible thing.

One could lock it down even more -- hide it behind a Kconfig option that
depends on CONFIG_EXPERT=y and itself defaults to n, require a mount option,
only allow the file owner to call no-hide-stale and only if the file is 0600
(or the appropriate group equivalents like Ted's existing patch), and upon
adding stale extents, set an inode flag that locks uid/gid/mode/flags.
Obviously root can still get to the file, but at least there's hard evidence
that one is entering the twilight zone.

> > Making Google's hack more widely available through the fallocate
> > API is entirely dependent on proving that:
> 
> Ceph is about to completely bypass the file system because of your
> intransigence, and reimplement a userspace file system.  They seem to
> believe it's necessary.  I'll let them make the case, because they
> seem to think it's necessary.  And if not, if Linus sides with you,
> and doesn't want to take the patch, I'll just keep it as a
> Google-specific out-of-tree patch.  I don't *need* to have this thing
> upstream.

Frankly, I second Eric Sandeen's comments -- just how bad is ext4's
unwritten extent conversion for these folks?

I ran this crappy looping microbenchmark against a ramdisk:
	fallocate 400M
	write 400M
	fsync
	rewrite the 400M
	fsync
on kernel 4.5.

For writing 400M through the page cache in 4k chunks,
ext4: ~460MB/s -> ~580MB/s (~20%)
XFS: ~660 -> ~870 (~25%)
btrfs: ~130 -> ~200 (~35%)

For writing 400M in 80M chunks,
ext4: ~480MB/s -> ~720MB/s (~30%)
XFS: ~1GB/s -> ~1.5GB/s (~35%)
btrfs: ~590MB/s -> ~590MB/s (no change)

For directio writing 400MB in 4k chunks,
ext4: 25MB/s -> 26MB/s (~5%)
XFS: 25 -> 27 (~8%)
btrfs: 22 -> 18 (...)

For directio writing 1200MB in 80M chunks,
ext4: ~2.9GB/s -> ~3.3GB/s (~13%)
XFS: 3.2 -> 3.5 (~9%)
btrfs: 2.3 -> 2.2 (...)

Clearly, the performance hit of unwritten extent conversion is large
enough to tempt people to ask for no-hide-stale.  But I'd rather hear
that directly from a developer, Ceph or otherwise.

In the meantime, please have a look at the v7 blockdev fallocate patches,
which implement only the "you can read zeroes afterwards" commands.

--D

> 
> 			    	      	    	   - Ted
> --
> 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]

#1359380

[Multipart message — attachments visible in raw view] — view raw

On Mar 15, 2016, at 7:51 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> On Tue, Mar 15, 2016 at 06:52:24PM -0400, Theodore Ts'o wrote:
>> On Wed, Mar 16, 2016 at 09:33:13AM +1100, Dave Chinner wrote:
>>> 
>>> Stale data escaping containment is a security issue. Enabling
>>> generic kernel mechanisms to *enable containment escape* is
>>> fundamentally wrong, and relying on userspace to Do The Right Thing
>>> is even more of a gamble, IMO.
>> 
>> We already have generic kernel mechanisms such as "the block device".   P
>> 
>>> It's a practical concern because if we enable this functionality in
>>> fallocate because it will get used by more than just special storage
>>> apps. i.e. this can't be waved away with "only properly managed
>>> applications will use it" arguments.
>> 
>> It requires a mount option.  How is this going to allow random
>> applications to use this feature, again?
>> 
>>> I also don't make a habit of publicising the fact that since we
>>> disabled the "-d unwritten=X" mkfs parameter (because of speed racer
>>> blogs such as the above and configuration cargo-culting resulting in
>>> unsuspecting users exposing stale data unintentionally) that the
>>> functionality still exists in the kernel code and that it only takes
>>> a single xfs_db command to turn off unwritten extents in XFS. i.e.
>>> we can easily make fallocate on XFS expose stale data, filesystem
>>> wide, without requiring mount options, kernel or application
>>> modifications.
>> 
>> So you have something even more dangerous in XFS and it's in the
>> kernel tree?  Has Red Hat threatened to have a distro-specific patch
> 
> xfs_db is the XFS debugger, so you can only enable that bit of functionality
> with magical commands, which IMHO isn't much different than people messing
> with their ext4 filesystems with debugfs.  You had better know what you're
> doing and if you break the filesystem you can eat both pieces. :P
> 
>> to comment out this code to make sure irresponsible users can't use
>> it?  What I've been suggesting has even more controls that what you
>> have.  And I've been keeping it as an out-of-tree kernel patch mainly
>> because you've been arguing that it's such a horrible thing.
> 
> One could lock it down even more -- hide it behind a Kconfig option that
> depends on CONFIG_EXPERT=y and itself defaults to n, require a mount option,
> only allow the file owner to call no-hide-stale and only if the file is 0600
> (or the appropriate group equivalents like Ted's existing patch), and upon
> adding stale extents, set an inode flag that locks uid/gid/mode/flags.
> Obviously root can still get to the file, but at least there's hard evidence
> that one is entering the twilight zone.
> 
>>> Making Google's hack more widely available through the fallocate
>>> API is entirely dependent on proving that:
>> 
>> Ceph is about to completely bypass the file system because of your
>> intransigence, and reimplement a userspace file system.  They seem to
>> believe it's necessary.  I'll let them make the case, because they
>> seem to think it's necessary.  And if not, if Linus sides with you,
>> and doesn't want to take the patch, I'll just keep it as a
>> Google-specific out-of-tree patch.  I don't *need* to have this thing
>> upstream.
> 
> Frankly, I second Eric Sandeen's comments -- just how bad is ext4's
> unwritten extent conversion for these folks?
> 
> I ran this crappy looping microbenchmark against a ramdisk:
> 	fallocate 400M
> 	write 400M
> 	fsync
> 	rewrite the 400M
> 	fsync
> on kernel 4.5.
> 
> For writing 400M through the page cache in 4k chunks,
> ext4: ~460MB/s -> ~580MB/s (~20%)
> XFS: ~660 -> ~870 (~25%)
> btrfs: ~130 -> ~200 (~35%)
> 
> For writing 400M in 80M chunks,
> ext4: ~480MB/s -> ~720MB/s (~30%)
> XFS: ~1GB/s -> ~1.5GB/s (~35%)
> btrfs: ~590MB/s -> ~590MB/s (no change)
> 
> For directio writing 400MB in 4k chunks,
> ext4: 25MB/s -> 26MB/s (~5%)
> XFS: 25 -> 27 (~8%)
> btrfs: 22 -> 18 (...)
> 
> For directio writing 1200MB in 80M chunks,
> ext4: ~2.9GB/s -> ~3.3GB/s (~13%)
> XFS: 3.2 -> 3.5 (~9%)
> btrfs: 2.3 -> 2.2 (...)
> 
> Clearly, the performance hit of unwritten extent conversion is large
> enough to tempt people to ask for no-hide-stale.  But I'd rather hear
> that directly from a developer, Ceph or otherwise.

I suspect that this gets significantly worse if you are running with
random writes instead of sequential overwrites.  With sequential overwrites
there is only a single boundary between init and uninit extents, so at
most one extra extent in the tree.  The above performance deltas will also
be much larger when real disks are involved and seek latency is a factor.

If you are doing random writes in a large file, then there may be many
thousands or millions of new extents and tree splits because the large
uninit extents from fallocate() need to be converted to init extents in
a piecemeal fashion.


We might consider tricks to optimize this somewhat for ext4, such as
limiting the uninit extent size below the init extent size (e.g. 1/2)
so that if such extent splits happen we can get a bunch of extent splits
in one leaf without having to also reshuffle the extent tree.  We already
ensure that there is a minimum uninit->init extent conversion size
(64KB IIRC?) so that we don't get pathological 4KB init/uninit extent
interleaving.  As all of the extents become initialized and merge we
can reduce the tree depth, but at least could avoid the worst case
scenarios.  However, I still suspect there would be a big overhead
from doing conversion under such a workload.

Cheers, Andreas

> In the meantime, please have a look at the v7 blockdev fallocate patches,
> which implement only the "you can read zeroes afterwards" commands.
> 
> --D
> 
>> 
>> 			    	      	    	   - Ted
>> --
>> 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
> --
> 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


Cheers, Andreas

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

#1359483 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Wed, Mar 16, 2016 at 03:45:49PM -0600, Andreas Dilger wrote:
> > Clearly, the performance hit of unwritten extent conversion is large
> > enough to tempt people to ask for no-hide-stale.  But I'd rather hear
> > that directly from a developer, Ceph or otherwise.
> 
> I suspect that this gets significantly worse if you are running with
> random writes instead of sequential overwrites.  With sequential overwrites
> there is only a single boundary between init and uninit extents, so at
> most one extra extent in the tree.  The above performance deltas will also
> be much larger when real disks are involved and seek latency is a factor.

It will vary a lot depending on your use case.  If you are running
with data=ordered, and with journalled enabled, then even if it is a
single extent that is modified, the fact that a journal transaction
involved, with a forced data block flush to avoid revealing stale
data, that is certainly going to be measurable.

The other thing is if you are worried about tail latency, which is a
major concern at Google[1], and you are running your disks close to
flat out, the fact that you have to do an extra seek to update the
extent tree is a seek that you can't be using for useful work --- and
worse, could delay a low-latency read from completing within your SLO.

[1] https://research.google.com/pubs/pub44830.html

Part of what's challenging with giving numbers is that it's trivially
easy to give some worst case scneario where the numbers are really
terrible.  A random 4k random write benchmark into an fallocated file,
eeven with XFS, would have pretty bad numbers, But of course people
wouldn't say that it's very realistic.  But those are the easiest to
get.

The most realistic numbers are going to be a lot harder to get, and
wouldn't necessarily make a lot of sense without revealing a lot
proprietary information.  I will say that Google does have a fairly
large number of disks[2] and so even a small fractional percentage
gain multipled by gazillions of disks starts turning into a dollar
number with enough zeros that people really sit up and take notice.
I'll also note that map reduce can be quite nasty as far as random I/O
is concerned[3], and while map reduce jobs are often not high priority
jobs, they can interfere with low-latency reads from important
applications (e.g., web search, user-visible gmail operations, etc.)

[2] https://what-if.xkcd.com/63/
[3] https://pdfs.semanticscholar.org/6238/e5f0fd807f634f5999701c7aa6a09d88dfc8.pdf

So I'm not sure what numbers I can really give that would satisfy
people.  Doing a random write fio job is not hard, and will result in
fairly impressive numbers.  If that's enough, then either I can do
this, or Chris Mason can reproduce his experiment using XFS (which
would presumably eliminate the excuse that it's because ext4 sucks at
extent operations).  But if that's not going to convince people, then
I'd much rather not waste my time.

Besides, at Google it's easy enough for me to maintain the patch
out-of-tree.  It's the Ceph folks who would need to at the very least,
have such a patch ship in Red Hat Enterprise Linux.  So it's probably
better for them to justify it, if numbers are really necessary.

       	   	   	       	  	  - Ted

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

#1359491 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On 3/16/16 7:15 PM, Theodore Ts'o wrote:
> On Wed, Mar 16, 2016 at 03:45:49PM -0600, Andreas Dilger wrote:
>>> Clearly, the performance hit of unwritten extent conversion is large
>>> enough to tempt people to ask for no-hide-stale.  But I'd rather hear
>>> that directly from a developer, Ceph or otherwise.
>>
>> I suspect that this gets significantly worse if you are running with
>> random writes instead of sequential overwrites.  With sequential overwrites
>> there is only a single boundary between init and uninit extents, so at
>> most one extra extent in the tree.  The above performance deltas will also
>> be much larger when real disks are involved and seek latency is a factor.
> 
> It will vary a lot depending on your use case.  If you are running
> with data=ordered, and with journalled enabled, then even if it is a
> single extent that is modified, the fact that a journal transaction
> involved, with a forced data block flush to avoid revealing stale
> data, that is certainly going to be measurable.
> 
> The other thing is if you are worried about tail latency, which is a
> major concern at Google[1], and you are running your disks close to
> flat out, the fact that you have to do an extra seek to update the
> extent tree is a seek that you can't be using for useful work --- and
> worse, could delay a low-latency read from completing within your SLO.
> 
> [1] https://research.google.com/pubs/pub44830.html
> 
> Part of what's challenging with giving numbers is that it's trivially
> easy to give some worst case scneario where the numbers are really
> terrible.  A random 4k random write benchmark into an fallocated file,
> eeven with XFS, would have pretty bad numbers, But of course people
> wouldn't say that it's very realistic.  But those are the easiest to
> get.
> 
> The most realistic numbers are going to be a lot harder to get, and
> wouldn't necessarily make a lot of sense without revealing a lot
> proprietary information.  I will say that Google does have a fairly
> large number of disks[2] and so even a small fractional percentage
> gain multipled by gazillions of disks starts turning into a dollar
> number with enough zeros that people really sit up and take notice.
> I'll also note that map reduce can be quite nasty as far as random I/O
> is concerned[3], and while map reduce jobs are often not high priority
> jobs, they can interfere with low-latency reads from important
> applications (e.g., web search, user-visible gmail operations, etc.)
> 
> [2] https://what-if.xkcd.com/63/
> [3] https://pdfs.semanticscholar.org/6238/e5f0fd807f634f5999701c7aa6a09d88dfc8.pdf
> 
> So I'm not sure what numbers I can really give that would satisfy
> people.  Doing a random write fio job is not hard, and will result in
> fairly impressive numbers.  If that's enough, then either I can do
> this, or Chris Mason can reproduce his experiment using XFS (which
> would presumably eliminate the excuse that it's because ext4 sucks at
> extent operations).  But if that's not going to convince people, then
> I'd much rather not waste my time.
> 
> Besides, at Google it's easy enough for me to maintain the patch
> out-of-tree.  It's the Ceph folks who would need to at the very least,
> have such a patch ship in Red Hat Enterprise Linux.  So it's probably
> better for them to justify it, if numbers are really necessary.

I may have lost the thread at this point, with poor Darrick's original
patch submission devolving into a long thread about a NO_HIDE_STALE patch
used at Google, but I don't *think* Ceph ever asked for NO_HIDE_STALE.

At least I can't find any indication of that.

Am I missing something?  cc'ing Greg on this one in case I am.

-Eric

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

#1359497 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Wed, Mar 16, 2016 at 07:33:55PM -0500, Eric Sandeen wrote:
> I may have lost the thread at this point, with poor Darrick's original
> patch submission devolving into a long thread about a NO_HIDE_STALE patch
> used at Google, but I don't *think* Ceph ever asked for NO_HIDE_STALE.
> 
> At least I can't find any indication of that.
> 
> Am I missing something?  cc'ing Greg on this one in case I am.

This came up at dinner after the Linux FAST conference where the Ric
Wheeler, some Ceph folks, and I were at dinner, and they were
discussing creating a userspace file systems because they couldn't get
kernel file system developers to be responsive for their needs.  I
pointed out what I had implemented at Google for our cluster file
system, which includes NO_HIDE_STALE_FL and the BLKDISCARD ioctl to
work on ext4 files, both of which I had kept out of tree because they
were rejected last time and Ric Wheeler had threated to patch them out
of the RHEL kernel if they went in upstream.

When Ric indicated that he was now a bit more accepting of these
patches now that he was responsible for managing people who needed the
functionality, I thought it was time to revisit the topic ---
especially since Tao Bao has been using the same patches for their
hadoopfs servers, and at the very least I should offer the patches to
the Ceph folks to see if they could get them into RHEL, and perhaps
take another run at getting the patches upstream.

						- Ted

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

#1359581

On Wed, Mar 16, 2016 at 5:33 PM, Eric Sandeen <esandeen@redhat.com> wrote:
> I may have lost the thread at this point, with poor Darrick's original
> patch submission devolving into a long thread about a NO_HIDE_STALE patch
> used at Google, but I don't *think* Ceph ever asked for NO_HIDE_STALE.
>
> At least I can't find any indication of that.
>
> Am I missing something?  cc'ing Greg on this one in case I am.

Brief background:
Ceph currently has two big local storage subsystems: FileStore and
BlueStore. FileStore is the one that's been around for forever and is
currently stable/production-ready/bla bla bla. This one represents
RADOS objects as actual files and while it's *mostly* just converting
object operations into posix FS ones, it does rely on a few pieces of
the fs namespace and posix ops to do its work.
BlueStore is our new, pure userspace solution (Sage started this about
8 months ago, I think?). It started out using xfs basically as a block
allocator, but at this point it's just doing raw block access 100% in
userspace.

So we've not asked for NO_HIDE_STALE on the mailing lists, but I think
it was one of the problems Sage had using xfs in his BlueStore
implementation and was a big part of why it moved to pure userspace.
FileStore might use NO_HIDE_STALE in some places but it would be
pretty limited. When it came up at Linux FAST we were discussing how
it and similar things had been problems for us in the past and it
would've been nice if they were upstream. What *is* a big deal for
FileStore (and would be easy to take advantage of) is the thematically
similar O_NOMTIME flag, which is also about reducing metadata updates
and got blocked on similar stupid-user grounds (although not security
ones): http://thread.gmane.org/gmane.linux.kernel.api/10727.
As noted though, we've basically given up and are moving to a
pure-userspace solution as quickly as we can. So no, Ceph isn't likely
to be a big user of these interfaces as it's too late for us. Adding
them would be an investment for future distributed storage systems
more than current ones. Maybe that's not worth it, or maybe there are
better places to keep them in the kernel. (I think I saw a reference
to some hypothetical shared block allocator? That would be *awesome*.)

=========
Separately. In the particular case of the extents and data leaks, a
coworker of mine suggested you could tag any files which *ever* had
unwritten extents with something that prevents them being read by a
user who doesn't have raw block access (and, even better, let us apply
that flag on file create)...that's a weird new security rule for
people to know and requires space for tagging (no idea how bad that
is), but would work in any use cases we have and would not leak
anything the user doesn't already have access to.
-Greg

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

#1359807 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Wed, Mar 16, 2016 at 10:18:19PM -0700, Gregory Farnum wrote:
> would've been nice if they were upstream. What *is* a big deal for
> FileStore (and would be easy to take advantage of) is the thematically
> similar O_NOMTIME flag, which is also about reducing metadata updates
> and got blocked on similar stupid-user grounds (although not security
> ones): http://thread.gmane.org/gmane.linux.kernel.api/10727.

Try mounting with the lazytime mount option.  That should give you the
nearly all of the metaata update reduction for free.  It is slightly
better optimized for in ext4. but it should work for xfs and btrfs as
well.  The mtime will still get updated on disk about once a day (or
if memory pressure pushes the inode out of memory or on an unmount),
but the in-memory inode has the correct timestamps, so stat will
return the correct mtime value and so the result is POSIX compliant
for those people who still care such things.

(Actually, if you need to pass POSIX conformance testing you should
use strictatime,lazytime so that atime is updated when POSIX mandates
it to be updated.)

						- Ted

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

#1359504 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Tue, Mar 15, 2016 at 06:51:39PM -0700, Darrick J. Wong wrote:
> On Tue, Mar 15, 2016 at 06:52:24PM -0400, Theodore Ts'o wrote:
> > On Wed, Mar 16, 2016 at 09:33:13AM +1100, Dave Chinner wrote:
> > > 
> > > Stale data escaping containment is a security issue. Enabling
> > > generic kernel mechanisms to *enable containment escape* is
> > > fundamentally wrong, and relying on userspace to Do The Right Thing
> > > is even more of a gamble, IMO.
> > 
> > We already have generic kernel mechanisms such as "the block device".   P
> > 
> > > It's a practical concern because if we enable this functionality in
> > > fallocate because it will get used by more than just special storage
> > > apps. i.e. this can't be waved away with "only properly managed
> > > applications will use it" arguments.
> > 
> > It requires a mount option.  How is this going to allow random
> > applications to use this feature, again?
> > 
> > > I also don't make a habit of publicising the fact that since we
> > > disabled the "-d unwritten=X" mkfs parameter (because of speed racer
> > > blogs such as the above and configuration cargo-culting resulting in
> > > unsuspecting users exposing stale data unintentionally) that the
> > > functionality still exists in the kernel code and that it only takes
> > > a single xfs_db command to turn off unwritten extents in XFS. i.e.
> > > we can easily make fallocate on XFS expose stale data, filesystem
> > > wide, without requiring mount options, kernel or application
> > > modifications.
> > 
> > So you have something even more dangerous in XFS and it's in the
> > kernel tree?  Has Red Hat threatened to have a distro-specific patch
> 
> xfs_db is the XFS debugger, so you can only enable that bit of functionality
> with magical commands, which IMHO isn't much different than people messing
> with their ext4 filesystems with debugfs.  You had better know what you're
> doing and if you break the filesystem you can eat both pieces. :P
> 
> > to comment out this code to make sure irresponsible users can't use
> > it?  What I've been suggesting has even more controls that what you
> > have.  And I've been keeping it as an out-of-tree kernel patch mainly
> > because you've been arguing that it's such a horrible thing.
> 
> One could lock it down even more -- hide it behind a Kconfig option that
> depends on CONFIG_EXPERT=y and itself defaults to n, require a mount option,
> only allow the file owner to call no-hide-stale and only if the file is 0600
> (or the appropriate group equivalents like Ted's existing patch), and upon
> adding stale extents, set an inode flag that locks uid/gid/mode/flags.
> Obviously root can still get to the file, but at least there's hard evidence
> that one is entering the twilight zone.
> 
> > > Making Google's hack more widely available through the fallocate
> > > API is entirely dependent on proving that:
> > 
> > Ceph is about to completely bypass the file system because of your
> > intransigence, and reimplement a userspace file system.  They seem to
> > believe it's necessary.  I'll let them make the case, because they
> > seem to think it's necessary.  And if not, if Linus sides with you,
> > and doesn't want to take the patch, I'll just keep it as a
> > Google-specific out-of-tree patch.  I don't *need* to have this thing
> > upstream.
> 
> Frankly, I second Eric Sandeen's comments -- just how bad is ext4's
> unwritten extent conversion for these folks?
> 
> I ran this crappy looping microbenchmark against a ramdisk:
> 	fallocate 400M
> 	write 400M
> 	fsync
> 	rewrite the 400M
> 	fsync
> on kernel 4.5.
> 
> For writing 400M through the page cache in 4k chunks,
> ext4: ~460MB/s -> ~580MB/s (~20%)
> XFS: ~660 -> ~870 (~25%)
> btrfs: ~130 -> ~200 (~35%)
> 
> For writing 400M in 80M chunks,
> ext4: ~480MB/s -> ~720MB/s (~30%)
> XFS: ~1GB/s -> ~1.5GB/s (~35%)
> btrfs: ~590MB/s -> ~590MB/s (no change)
> 
> For directio writing 400MB in 4k chunks,
> ext4: 25MB/s -> 26MB/s (~5%)
> XFS: 25 -> 27 (~8%)
> btrfs: 22 -> 18 (...)
> 
> For directio writing 1200MB in 80M chunks,
> ext4: ~2.9GB/s -> ~3.3GB/s (~13%)
> XFS: 3.2 -> 3.5 (~9%)
> btrfs: 2.3 -> 2.2 (...)

That's not comparing apples to apples. overwrite does not require
allocation/extent manipulation at all so it is comparing performance
of completely different file extent operations. The operations we
should be comparing are "first write" operations, namely "write over
hole with allocation" vs "write over preallocation".  Overwrite
performance should be the same regardless of the method used for the
intial write/allocation.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

#1359537 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Thu, Mar 17, 2016 at 12:01:16PM +1100, Dave Chinner wrote:
> On Tue, Mar 15, 2016 at 06:51:39PM -0700, Darrick J. Wong wrote:
> > On Tue, Mar 15, 2016 at 06:52:24PM -0400, Theodore Ts'o wrote:
> > > On Wed, Mar 16, 2016 at 09:33:13AM +1100, Dave Chinner wrote:
> > > > 
> > > > Stale data escaping containment is a security issue. Enabling
> > > > generic kernel mechanisms to *enable containment escape* is
> > > > fundamentally wrong, and relying on userspace to Do The Right Thing
> > > > is even more of a gamble, IMO.
> > > 
> > > We already have generic kernel mechanisms such as "the block device".   P
> > > 
> > > > It's a practical concern because if we enable this functionality in
> > > > fallocate because it will get used by more than just special storage
> > > > apps. i.e. this can't be waved away with "only properly managed
> > > > applications will use it" arguments.
> > > 
> > > It requires a mount option.  How is this going to allow random
> > > applications to use this feature, again?
> > > 
> > > > I also don't make a habit of publicising the fact that since we
> > > > disabled the "-d unwritten=X" mkfs parameter (because of speed racer
> > > > blogs such as the above and configuration cargo-culting resulting in
> > > > unsuspecting users exposing stale data unintentionally) that the
> > > > functionality still exists in the kernel code and that it only takes
> > > > a single xfs_db command to turn off unwritten extents in XFS. i.e.
> > > > we can easily make fallocate on XFS expose stale data, filesystem
> > > > wide, without requiring mount options, kernel or application
> > > > modifications.
> > > 
> > > So you have something even more dangerous in XFS and it's in the
> > > kernel tree?  Has Red Hat threatened to have a distro-specific patch
> > 
> > xfs_db is the XFS debugger, so you can only enable that bit of functionality
> > with magical commands, which IMHO isn't much different than people messing
> > with their ext4 filesystems with debugfs.  You had better know what you're
> > doing and if you break the filesystem you can eat both pieces. :P
> > 
> > > to comment out this code to make sure irresponsible users can't use
> > > it?  What I've been suggesting has even more controls that what you
> > > have.  And I've been keeping it as an out-of-tree kernel patch mainly
> > > because you've been arguing that it's such a horrible thing.
> > 
> > One could lock it down even more -- hide it behind a Kconfig option that
> > depends on CONFIG_EXPERT=y and itself defaults to n, require a mount option,
> > only allow the file owner to call no-hide-stale and only if the file is 0600
> > (or the appropriate group equivalents like Ted's existing patch), and upon
> > adding stale extents, set an inode flag that locks uid/gid/mode/flags.
> > Obviously root can still get to the file, but at least there's hard evidence
> > that one is entering the twilight zone.
> > 
> > > > Making Google's hack more widely available through the fallocate
> > > > API is entirely dependent on proving that:
> > > 
> > > Ceph is about to completely bypass the file system because of your
> > > intransigence, and reimplement a userspace file system.  They seem to
> > > believe it's necessary.  I'll let them make the case, because they
> > > seem to think it's necessary.  And if not, if Linus sides with you,
> > > and doesn't want to take the patch, I'll just keep it as a
> > > Google-specific out-of-tree patch.  I don't *need* to have this thing
> > > upstream.
> > 
> > Frankly, I second Eric Sandeen's comments -- just how bad is ext4's
> > unwritten extent conversion for these folks?
> > 
> > I ran this crappy looping microbenchmark against a ramdisk:
> > 	fallocate 400M
> > 	write 400M
> > 	fsync
> > 	rewrite the 400M
> > 	fsync
> > on kernel 4.5.
> > 
> > For writing 400M through the page cache in 4k chunks,
> > ext4: ~460MB/s -> ~580MB/s (~20%)
> > XFS: ~660 -> ~870 (~25%)
> > btrfs: ~130 -> ~200 (~35%)
> > 
> > For writing 400M in 80M chunks,
> > ext4: ~480MB/s -> ~720MB/s (~30%)
> > XFS: ~1GB/s -> ~1.5GB/s (~35%)
> > btrfs: ~590MB/s -> ~590MB/s (no change)
> > 
> > For directio writing 400MB in 4k chunks,
> > ext4: 25MB/s -> 26MB/s (~5%)
> > XFS: 25 -> 27 (~8%)
> > btrfs: 22 -> 18 (...)
> > 
> > For directio writing 1200MB in 80M chunks,
> > ext4: ~2.9GB/s -> ~3.3GB/s (~13%)
> > XFS: 3.2 -> 3.5 (~9%)
> > btrfs: 2.3 -> 2.2 (...)
> 
> That's not comparing apples to apples. overwrite does not require
> allocation/extent manipulation at all so it is comparing performance
> of completely different file extent operations. The operations we
> should be comparing are "first write" operations, namely "write over
> hole with allocation" vs "write over preallocation".  Overwrite
> performance should be the same regardless of the method used for the
> intial write/allocation.

Eh, ok, let's compare writing an fallocated region vs writing an empty file:

(laptop this time, so the numbers aren't the same)

For writing 400M in 4k chunks,
ext4: ~720MB/s -> 620MB/s (~14%)
XFS: ~560MB/s -> 540MB/s (~4%)
btrfs: ~260 -> 580MB/s

For writing 400M in 80M chunks,
ext4: ~960MB/s -> ~730MB/s (~24%)
XFS: ~1000MB/s -> ~980MB/s (~2%)
btrfs: ~950MB/s -> ~930MB/s (~3%)

For directio writing 1200MB in 80M chunks,
ext4: ~2.9GB/s -> ~2.8GB/s (~4%)
XFS: 3.2 -> 3.2
btrfs: 2.3 -> 2.3

--D

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

#1358324

On Tue, Mar 15, 2016 at 3:33 PM, Dave Chinner <david@fromorbit.com> wrote:
>
>> There's no "group based containment wall" that is some kind of
>> absolute protection border.
>
> Precisely my point - it's being pitched as a generic containment
> mechanism, but it really isn't.

No it hasn't.

It has been pitched as

 "Companies are already doing this, and other groups are interested in it too".

What part of that is hard to understand?

At no point was it "generic containment" except in your mind.
Everybody else was very clear about the fact that stale data would be
visible. The only issue was to try to mitigate subtle mistakes.

And no, "Root reads a file that has possibly stale data in it" is not
a "subtle mistake". That's a rather obvious thing.

> Making Google's hack more widely available through the fallocate
> API is entirely dependent on proving that:
>
>         a) the performance problem still exists;
>         b) the performance problem exists across multiple
>            filesytsems and is not isolated to just ext4 or one specific
>            set of workloads;
>         c) the performance problem cannot be fixed;
>         d) ext4 can't implement a simple feature check to turn off
>            unwritten extents similar to XFS; and
>         e) if all else fails, that the API hack does not compromise the
>            security of general users unaware that applications might
>            be using this functionality.
>
> a), b), c) and d) have not been demonstrated, discussed or iterated -

The thing is, the onus of those shouldn't be on the people who already
have a solution that they are happy with.

The onus of that work should be on the people who are arguing against
it, and have alternate models they want to push.

So you can't ask people who already solved their issue to then try to
argue against their solution.

I do agree that it would be damn useful to have

 (a) numbers. No question about that.

 (b) we should have a better idea of exactly what the user needs are.
Because if we do expose this kind of functionality, it should be as
limited as possible - but that "as possible" very much depends on what
the usage models are.

And yes, "keep the patch entirely inside google" is obviously one good
way to limit the interface. But if there are really other groups that
want to explore this, then that sounds like a pretty horrible model
too.

                  Linus

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

#1358331

On Tue, Mar 15, 2016 at 4:06 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And yes, "keep the patch entirely inside google" is obviously one good
> way to limit the interface. But if there are really other groups that
> want to explore this, then that sounds like a pretty horrible model
> too.

Side note: I really don't see how your argument of "XFS has been able
to do something like this for over a decade, using an even uglier
trick that is hidden and not documented" is at all an argument for
your position.

You're saying "nobody else should be doing what I've been doing for a
long time", and backing that argument up with "but I don't document
it, and it's completely different because it's done at mkfs/debugfs
time rather than mount-time".

But now that people are talking about a filesystem-independent way of
doing the same thing, now it's suddenly poisonous.

Dave, I call BS on your arguments. Or maybe I misunderstood it. But it
does smell very "do what I say, not what I do".

            Linus

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

#1358444 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Tue, Mar 15, 2016 at 04:14:32PM -0700, Linus Torvalds wrote:
> On Tue, Mar 15, 2016 at 4:06 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > And yes, "keep the patch entirely inside google" is obviously one good
> > way to limit the interface. But if there are really other groups that
> > want to explore this, then that sounds like a pretty horrible model
> > too.
> 
> Side note: I really don't see how your argument of "XFS has been able
> to do something like this for over a decade, using an even uglier
> trick that is hidden and not documented" is at all an argument for
> your position.
> 
> You're saying "nobody else should be doing what I've been doing for a
> long time", and backing that argument up with "but I don't document
> it, and it's completely different because it's done at mkfs/debugfs
> time rather than mount-time".

You can read it that way, but that is not the message I was trying
to get across.

The message I was trying to get across is that there are people out
there that have been using hacks like what google uses for as long
as there have been filesystems around that support unwritten
extents. And that they do this even though the benefits of such
hacks are marginal and frequently can't be backed up with numbers.

We don't support filesystems with unwritten extents disabled. I
don't like the fact that there are people who turn them off on XFS
filesystems, but we are stuck with it as it is part of the supported
on disk format that. The best we can do is to try to prevent
unsuspecting users from shooting themselves in the foot with such
features, which is what we did by removing the mkfs option back in
2007.

Like I said, most people don't know or understand the history....

> But now that people are talking about a filesystem-independent way of
> doing the same thing, now it's suddenly poisonous.

It's always been poisonous.

> Dave, I call BS on your arguments. Or maybe I misunderstood it. But it
> does smell very "do what I say, not what I do".

We're stuck with a lot of historical functionality in XFS that we
can't easily remove or disable. Learn from history, don't repeat it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

#1358416 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Tue, Mar 15, 2016 at 04:06:10PM -0700, Linus Torvalds wrote:
> On Tue, Mar 15, 2016 at 3:33 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> >> There's no "group based containment wall" that is some kind of
> >> absolute protection border.
> >
> > Precisely my point - it's being pitched as a generic containment
> > mechanism, but it really isn't.
> 
> No it hasn't.
> 
> It has been pitched as
> 
>  "Companies are already doing this, and other groups are interested in it too".
> 
> What part of that is hard to understand?
> 
> At no point was it "generic containment" except in your mind.
> Everybody else was very clear about the fact that stale data would be
> visible. The only issue was to try to mitigate subtle mistakes.
> 
> And no, "Root reads a file that has possibly stale data in it" is not
> a "subtle mistake". That's a rather obvious thing.
> 
> > Making Google's hack more widely available through the fallocate
> > API is entirely dependent on proving that:
> >
> >         a) the performance problem still exists;
> >         b) the performance problem exists across multiple
> >            filesytsems and is not isolated to just ext4 or one specific
> >            set of workloads;
> >         c) the performance problem cannot be fixed;
> >         d) ext4 can't implement a simple feature check to turn off
> >            unwritten extents similar to XFS; and
> >         e) if all else fails, that the API hack does not compromise the
> >            security of general users unaware that applications might
> >            be using this functionality.
> >
> > a), b), c) and d) have not been demonstrated, discussed or iterated -
> 
> The thing is, the onus of those shouldn't be on the people who already
> have a solution that they are happy with.
>
> The onus of that work should be on the people who are arguing against
> it, and have alternate models they want to push.

I hate having to quote the guidelines we are supposed to work by,
but this seems pretty clear cut. Section 2 of
Documentation/SubmittingPatches:

"Describe your problem.  Whether your patch is a one-line bug fix or
5000 lines of a new feature, there must be an underlying problem
that motivated you to do this work.  Convince the reviewer that
there is a problem worth fixing and that it makes sense for them to
read past the first paragraph.

Describe user-visible impact.  Straight up crashes and lockups are
pretty convincing, but not all bugs are that blatant.  Even if the
problem was spotted during code review, describe the impact you think
it can have on users.  Keep in mind that the majority of Linux
installations run kernels from secondary stable trees or
vendor/product-specific trees that cherry-pick only specific patches
from upstream, so include anything that could help route your change
downstream: provoking circumstances, excerpts from dmesg, crash
descriptions, performance regressions, latency spikes, lockups, etc.

Quantify optimizations and trade-offs.  If you claim improvements in
performance, memory consumption, stack footprint, or binary size,
include numbers that back them up.  But also describe non-obvious
costs.  Optimizations usually aren't free but trade-offs between CPU,
memory, and readability; or, when it comes to heuristics, between
different workloads.  Describe the expected downsides of your
optimization so that the reviewer can weigh costs against benefits.

Once the problem is established, describe what you are actually doing
about it in technical detail.  It's important to describe the change
in plain English for the reviewer to verify that the code is behaving
as you intend it to."

It is pretty clear that the onus is on the patch submitter to
provide justification for inclusion, not for the reviewer/Maintainer
to have to prove that the solution is unworkable. "Google uses this"
is not sufficient justification.

> So you can't ask people who already solved their issue to then try to
> argue against their solution.

I'm not asking Ted to argue against his solution. I'm the person who
is trying to poke holes in the "solution" being presented...

> I do agree that it would be damn useful to have
> 
>  (a) numbers. No question about that.
> 
>  (b) we should have a better idea of exactly what the user needs are.
> Because if we do expose this kind of functionality, it should be as
> limited as possible - but that "as possible" very much depends on what
> the usage models are.

It's not "damn useful" - it's an absolute requirement that numbers
are provided to back up the assertion that there is no other way to
solve this problem. Once it is established that there is no other
way to solve the performance problem, then we can talk whether we
want to trade off data safety and complexity for performance....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

#1358448

On Tue, Mar 15, 2016 at 4:52 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> It is pretty clear that the onus is on the patch submitter to
> provide justification for inclusion, not for the reviewer/Maintainer
> to have to prove that the solution is unworkable.

I agree, but quite frankly, performance is a good justification.

So if Ted can give performance numbers, that's justification enough.
We've certainly taken changes with less.

And with your "we should _not_ do this" argument, the onus is clearly on you.

> "Google uses this" is not sufficient justification.

Not per se, no, but it's a very traditional and time-honored model for
"should we merge this".

It's traditionally been things like "Redhat merged it in their distro
kernel, because they have customers that want/need it". That is *the*
reason many big projects got merged, ranging from filesystems to
drivers etc. Take reiserfs, for example: it got merged because SuSE
was actively using it.

So "this feature is being used in real life" is a big hint that the
standard upstream kernel may be missing something important. People
arguing against things like that has been a big problem in the past.
It took people _years_ to get over the whole Android thing. We need to
merge stuff that people are using and depend on, because _not_ merging
them just causes more and more distance between peoples kernels, and
makes it even harder to merge in the future.

I do agree that we want to have hard numbers.

And I do think that we should strive for the whole "we want to merge"
phase to be a time when we also look at "can we improve the
interfaces". But that can - and often does - go too far. Again, we had
_years_ of pointless masturbation over the whole Android thing, just
because people were making up new interfaces.

               Linus

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

#1358452 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On 3/15/16 7:06 PM, Linus Torvalds wrote:
> On Tue, Mar 15, 2016 at 4:52 PM, Dave Chinner <david@fromorbit.com> wrote:
>> >
>> > It is pretty clear that the onus is on the patch submitter to
>> > provide justification for inclusion, not for the reviewer/Maintainer
>> > to have to prove that the solution is unworkable.
> I agree, but quite frankly, performance is a good justification.
> 
> So if Ted can give performance numbers, that's justification enough.
> We've certainly taken changes with less.

I've been away from ext4 for a while, so I'm really not on top of the
mechanics of the underlying problem at the moment.

But I would say that in addition to numbers showing that ext4 has trouble
with unwritten extent conversion, we should have an explanation of
why it can't be solved in a way that doesn't open up these concerns.

XFS certainly has different mechanisms, but is the demonstrated workload
problematic on XFS (or btrfs) as well?  If not, can ext4 adopt any of the
solutions that make the workload perform better on other filesystems?

Adding the risk and complexity of the stale data exposure should be
considered as a last resort, after the above questions have been answered.

-Eric

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

#1358458 — Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks

On Tue, Mar 15, 2016 at 07:30:14PM -0500, Eric Sandeen wrote:
> On 3/15/16 7:06 PM, Linus Torvalds wrote:
> > On Tue, Mar 15, 2016 at 4:52 PM, Dave Chinner <david@fromorbit.com> wrote:
> >> >
> >> > It is pretty clear that the onus is on the patch submitter to
> >> > provide justification for inclusion, not for the reviewer/Maintainer
> >> > to have to prove that the solution is unworkable.
> > I agree, but quite frankly, performance is a good justification.
> > 
> > So if Ted can give performance numbers, that's justification enough.
> > We've certainly taken changes with less.
> 
> I've been away from ext4 for a while, so I'm really not on top of the
> mechanics of the underlying problem at the moment.
> 
> But I would say that in addition to numbers showing that ext4 has trouble
> with unwritten extent conversion, we should have an explanation of
> why it can't be solved in a way that doesn't open up these concerns.
> 
> XFS certainly has different mechanisms, but is the demonstrated workload
> problematic on XFS (or btrfs) as well?  If not, can ext4 adopt any of the
> solutions that make the workload perform better on other filesystems?

When I've benchmarked this in the past, doing small random buffered writes
into an preallocated extent was dramatically (3x or more) slower on xfs
than doing them into a fully written extent.  That was two years ago,
but I can redo it.

On a fio card, this gets 16,000 iops on a preallocated extent and 40,000
iops if you run it a second time.  It's not random writes, but the fsync
probably means the preallocated conversion is more expensive.  That's
on a 4.0 kernel, but I'll rerun it on nvme on newer kernels.

fio --name=fsync --rw=write --fsync=1 --bs=4k --filename=/xfs/fio_4096  --size=4g --overwrite=0

I'm happy to run variations on things, just let me know what workloads
are interesting.

-chris

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

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

Back to top | Article view | linux.kernel

csiph-web