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 Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-17 18: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-17 19:00 +0100
                                                    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Ric Wheeler <rwheeler@redhat.com> - 2016-03-17 19:00 +0100
                                                    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Chris Mason <clm@fb.com> - 2016-03-17 19:40 +0100
                                                      Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Andreas Dilger <adilger@dilger.ca> - 2016-03-17 21:50 +0100
                                                        Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Chris Mason <clm@fb.com> - 2016-03-17 22: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-18 04:30 +0100
                                                            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Jeff Moyer <jmoyer@redhat.com> - 2016-03-18 16:20 +0100
                                                              Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-03-18 21:10 +0100
                                                    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of blocks Gregory Farnum <greg@gregs42.com> - 2016-03-18 08: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-18 08:30 +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 NeilBrown <neilb@suse.com> - 2016-03-19 00: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 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]

#1360083

On Wed, Mar 16, 2016 at 10:18 PM, Gregory Farnum <greg@gregs42.com> wrote:
>
> 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.

Hmm.

So to me it really sounds like somebody should cook up a patch, but we
shouldn't put it in the upstream kernel until we get numbers and
actual "yes, we'd use this" from outside of google.

I say "outside of google", because inside of google not only do we not
get numbers, but google can maintain their own patch.

But maybe Ted could at least post the patch google uses, and somebody
in the Ceph community might want to at least try it out...

>                                                                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.

Hmm. I don't hate that patch, because the NOATIME thing really does
wonders on many loads. NOMTIME makes sense.

It's not like you can't do this with utimes() anyway.

That said, I do wonder if people wouldn't just prefer to expand on and
improve on the lazytime.

Is there some reason you guys didn't use that?

> As noted though, we've basically given up and are moving to a
> pure-userspace solution as quickly as we can.

That argues against worrying about this all in the kernel unless there
are other users.

                  Linus

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

#1360094

On Thu, Mar 17, 2016 at 10:50 AM, Ric Wheeler <rwheeler@redhat.com> wrote:
>>
>> That argues against worrying about this all in the kernel unless there
>> are other users.
>
> Just a note, when Greg says "user space solution", Ceph is looking at
> writing directly to raw block devices which is kind of a through back to
> early enterprise database trends.

Right, I understand. But that makes it kind of pointless to worry
about NO_HIDE_STALE, since it wouldn't get used anyway. The issues
with filesystem preallocation just don't exist.

Of course, if there are other possible users, we should keep this on
the table, but ...

                 Linus

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

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

On 03/17/2016 01:47 PM, Linus Torvalds wrote:
> On Wed, Mar 16, 2016 at 10:18 PM, Gregory Farnum <greg@gregs42.com> wrote:
>> 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.
> Hmm.
>
> So to me it really sounds like somebody should cook up a patch, but we
> shouldn't put it in the upstream kernel until we get numbers and
> actual "yes, we'd use this" from outside of google.
>
> I say "outside of google", because inside of google not only do we not
> get numbers, but google can maintain their own patch.
>
> But maybe Ted could at least post the patch google uses, and somebody
> in the Ceph community might want to at least try it out...
>
>>                                                                 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.
> Hmm. I don't hate that patch, because the NOATIME thing really does
> wonders on many loads. NOMTIME makes sense.
>
> It's not like you can't do this with utimes() anyway.
>
> That said, I do wonder if people wouldn't just prefer to expand on and
> improve on the lazytime.
>
> Is there some reason you guys didn't use that?
>
>> As noted though, we've basically given up and are moving to a
>> pure-userspace solution as quickly as we can.
> That argues against worrying about this all in the kernel unless there
> are other users.
>
>                    Linus

Just a note, when Greg says "user space solution", Ceph is looking at writing 
directly to raw block devices which is kind of a through back to early 
enterprise database trends.

Ric

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

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

On Thu, Mar 17, 2016 at 10:47:29AM -0700, Linus Torvalds wrote:
> On Wed, Mar 16, 2016 at 10:18 PM, Gregory Farnum <greg@gregs42.com> wrote:
> >
> > 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.
> 
> Hmm.
> 
> So to me it really sounds like somebody should cook up a patch, but we
> shouldn't put it in the upstream kernel until we get numbers and
> actual "yes, we'd use this" from outside of google.

We haven't had internal tiers yelling at us for fallocate performance,
so I'm unlikely to suggest it, just because its a potential
privacy leak we'd have to educate people about.  What I'd be more likely
to use is code inside the filesystem like this:

somefs_fallocate() {
	if (trim_can_really_zero(my_device)) {
		trim
		allocate a regular extent
		return
	} else {
		do normal fallocate
	}
}

Then the out of tree patch (for google or whoever) becomes a hack to
flip trim_can_really_zero on a given block device.  The rest of us can
use explicit interfaces from the hardware when deciding what we want
preallocation to mean.

It gets messy for crcs in btrfs, so we'd need the old fashioned
preallocation anyway.  But the database workloads where this matters
aren't our target right now, so its more an ext4/xfs thing anyway.

-chris

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

#1360188

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

On Mar 17, 2016, at 12:35 PM, Chris Mason <clm@fb.com> wrote:
> 
> On Thu, Mar 17, 2016 at 10:47:29AM -0700, Linus Torvalds wrote:
>> On Wed, Mar 16, 2016 at 10:18 PM, Gregory Farnum <greg@gregs42.com> wrote:
>>> 
>>> 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.
>> 
>> Hmm.
>> 
>> So to me it really sounds like somebody should cook up a patch, but we
>> shouldn't put it in the upstream kernel until we get numbers and
>> actual "yes, we'd use this" from outside of google.
> 
> We haven't had internal tiers yelling at us for fallocate performance,
> so I'm unlikely to suggest it, just because its a potential
> privacy leak we'd have to educate people about.  What I'd be more likely
> to use is code inside the filesystem like this:
> 
> somefs_fallocate() {
> 	if (trim_can_really_zero(my_device)) {
> 		trim
> 		allocate a regular extent
> 		return
> 	} else {
> 		do normal fallocate
> 	}
> }

We were discussing almost this very same thing in the ext4 concall today.

Ted initially didn't think it was worthwhile to implement, but after looking
at the whitelist for SATA SSDs it seems that there are enough devices on the
market that support the ATA_HORKAGE_ZERO_AFTER_TRIM to make this approach
worthwhile to implement.

Also, if the ext4 extent size was limited it might even be possible to do
this efficiently enough with write_same on HDD devices.

> Then the out of tree patch (for google or whoever) becomes a hack to
> flip trim_can_really_zero on a given block device.  The rest of us can
> use explicit interfaces from the hardware when deciding what we want
> preallocation to mean.

This might be a bit trickier, since this would affect all zero/trim
operations, not just ones for uninitialized data extents.

> It gets messy for crcs in btrfs, so we'd need the old fashioned
> preallocation anyway.  But the database workloads where this matters
> aren't our target right now, so its more an ext4/xfs thing anyway.


Cheers, Andreas

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

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

On Thu, Mar 17, 2016 at 02:49:06PM -0600, Andreas Dilger wrote:
> On Mar 17, 2016, at 12:35 PM, Chris Mason <clm@fb.com> wrote:
> > 
> > On Thu, Mar 17, 2016 at 10:47:29AM -0700, Linus Torvalds wrote:
> >> On Wed, Mar 16, 2016 at 10:18 PM, Gregory Farnum <greg@gregs42.com> wrote:
> >>> 
> >>> 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.
> >> 
> >> Hmm.
> >> 
> >> So to me it really sounds like somebody should cook up a patch, but we
> >> shouldn't put it in the upstream kernel until we get numbers and
> >> actual "yes, we'd use this" from outside of google.
> > 
> > We haven't had internal tiers yelling at us for fallocate performance,
> > so I'm unlikely to suggest it, just because its a potential
> > privacy leak we'd have to educate people about.  What I'd be more likely
> > to use is code inside the filesystem like this:
> > 
> > somefs_fallocate() {
> > 	if (trim_can_really_zero(my_device)) {
> > 		trim
> > 		allocate a regular extent
> > 		return
> > 	} else {
> > 		do normal fallocate
> > 	}
> > }
> 
> We were discussing almost this very same thing in the ext4 concall today.
> 
> Ted initially didn't think it was worthwhile to implement, but after looking
> at the whitelist for SATA SSDs it seems that there are enough devices on the
> market that support the ATA_HORKAGE_ZERO_AFTER_TRIM to make this approach
> worthwhile to implement.

We'll end up with people complaining it makes fallocate slower because
of the trims, so it's not a perfect solution.  But I much prefer it to
fallocate-stale.

> 
> Also, if the ext4 extent size was limited it might even be possible to do
> this efficiently enough with write_same on HDD devices.
> 
> > Then the out of tree patch (for google or whoever) becomes a hack to
> > flip trim_can_really_zero on a given block device.  The rest of us can
> > use explicit interfaces from the hardware when deciding what we want
> > preallocation to mean.
> 
> This might be a bit trickier, since this would affect all zero/trim
> operations, not just ones for uninitialized data extents.

Thinking more, my guess is that google will just keep doing what they
are already doing ;)  But there could be a flag in sysfs dedicated to
trim-for-fallocate so admins can see what their devices are reporting.
readonly in mainline, if someone wants to patch it in their large data
center it wouldn't be hard.

-chris

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

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

On Thu, Mar 17, 2016 at 02:00:18PM -0700, Chris Mason wrote:
> 
> Thinking more, my guess is that google will just keep doing what they
> are already doing ;)  But there could be a flag in sysfs dedicated to
> trim-for-fallocate so admins can see what their devices are reporting.
> readonly in mainline, if someone wants to patch it in their large data
> center it wouldn't be hard.

That's true, because one of the major use cases is SATA drives where
trim isn't available.  Even for SAS drives where you have WRITE SAME,
you wouldn't want to use it for large fallocate regions.

So I see using reliable trim as a zeroing mechanism to be orthogonal
to the question of NO_HIIDE_STALE.

I do think that using TRIM in various causes where we are doing an
fallocate does make sense for non-rotational devices.  In general TRIM
should be fast enough that that I'd be surprised that people would be
complaining --- especially since most of the time, fallocate isn't on
the timing-critical path of most applications.

							- Ted

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

#1360728

"Theodore Ts'o" <tytso@mit.edu> writes:

> I do think that using TRIM in various causes where we are doing an
> fallocate does make sense for non-rotational devices.  In general TRIM
> should be fast enough that that I'd be surprised that people would be
> complaining --- especially since most of the time, fallocate isn't on
> the timing-critical path of most applications.

TRIM/UNMAP isn't just supported on solid state devices, though.  I do
recall some enterprise thinly provisioned storage that would take ages
to discard large regions.  I think that caused us to change the defaults
for mkfs, right?

Cheers,
Jeff

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

#1360903

>>>>> "Jeff" == Jeff Moyer <jmoyer@redhat.com> writes:

Jeff> TRIM/UNMAP isn't just supported on solid state devices, though.  I
Jeff> do recall some enterprise thinly provisioned storage that would
Jeff> take ages to discard large regions.  I think that caused us to
Jeff> change the defaults for mkfs, right?

I think those have largely been fixed. But, yes.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

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

Back to top | Article view | linux.kernel

csiph-web