From: Darrick J. Wong <hidden> Date: 2016-03-02 04:10:16
Hi,
This is (yet another) repost of the patch series that fixes the
existing BLKZEROOUT ioctl to invalidate the page cache if the zeroing
command to the underlying device succeeds. This patch is against
4.5-rc6 and hasn't changed much in months.
The new BLKZEROOUT ioctl has the same semantics as the old one, but it
invalidates the page cache to prevent surprising results, just like
how dio writes invalidate page cache.
I've incorporated all the feedback I've received into these patches,
but haven't heard yea or nay or anything at all from the maintainer.
Will someone please pick this up for 4.6?
Comments and questions are, as always, welcome.
--D
From: Darrick J. Wong <hidden> Date: 2016-03-02 04:10:19
Invalidate the page cache (as a regular O_DIRECT write would do) to avoid
returning stale cache contents at a later time.
v5: Refactor the 4.4 refactoring of the ioctl code into separate functions.
Split the page invalidation and the new ioctl into separate patches.
Signed-off-by: Darrick J. Wong <redacted>
---
block/ioctl.c | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
From: Darrick J. Wong <hidden> Date: 2016-03-02 04:10:22
Create a new ioctl to expose the block layer's newfound ability to
issue either a zeroing discard, a WRITE SAME with a zero page, or a
regular write with the zero page. This BLKZEROOUT2 ioctl takes
{start, length, flags} as parameters. So far, the only flag available
is to enable the zeroing discard part -- without it, the call invokes
the old BLKZEROOUT behavior. start and length have the same meaning
as in BLKZEROOUT.
This new ioctl also invalidates the page cache correctly on account
of the previous patch in the series.
v3: Add extra padding for future expansion, and check the padding is zero.
v4: Check the start/len arguments for overflows prior to feeding the page
cache bogus numbers (that it'll ignore anyway).
v5: Refactor the 4.4 refactoring of the ioctl code into separate functions.
Separate patches for invalidation and new ioctl.
Signed-off-by: Darrick J. Wong <redacted>
---
block/ioctl.c | 57 ++++++++++++++++++++++++++++++++++++-----------
include/uapi/linux/fs.h | 9 +++++++
2 files changed, 53 insertions(+), 13 deletions(-)
On Tuesday 01 March 2016 20:09:32 Darrick J. Wong wrote:
This is (yet another) repost of the patch series that fixes the
existing BLKZEROOUT ioctl to invalidate the page cache if the zeroing
command to the underlying device succeeds. This patch is against
4.5-rc6 and hasn't changed much in months.
The new BLKZEROOUT ioctl has the same semantics as the old one, but it
invalidates the page cache to prevent surprising results, just like
how dio writes invalidate page cache.
I've incorporated all the feedback I've received into these patches,
but haven't heard yea or nay or anything at all from the maintainer.
Will someone please pick this up for 4.6?
Comments and questions are, as always, welcome.
I'm missing the background on this, just saw the patch fly by,
so sorry if this has been asked before:
Why do you want to invalidate the cache? Is this to save RAM
or is something else going to write here and you have to invalidate
it for correctness?
If you just want to save RAM, would it be possible to instead
point the page cache to the empty zero page to speed up subsequent
reads? Maybe that just causes more complexity than it helps.
Arnd
From: Christoph Hellwig <hch@infradead.org> Date: 2016-03-02 09:44:20
On Wed, Mar 02, 2016 at 10:15:02AM +0100, Arnd Bergmann wrote:
I'm missing the background on this, just saw the patch fly by,
so sorry if this has been asked before:
Why do you want to invalidate the cache? Is this to save RAM
or is something else going to write here and you have to invalidate
it for correctness?
BLKZEROOUT can be though as a direct I/O operation - it issues a
WRITE SAME (or similar) command straight to the device to zero
the specified range. If there was cached data for this range it would
now be stale.
On Wednesday 02 March 2016 01:44:16 Christoph Hellwig wrote:
On Wed, Mar 02, 2016 at 10:15:02AM +0100, Arnd Bergmann wrote:
quoted
I'm missing the background on this, just saw the patch fly by,
so sorry if this has been asked before:
Why do you want to invalidate the cache? Is this to save RAM
or is something else going to write here and you have to invalidate
it for correctness?
BLKZEROOUT can be though as a direct I/O operation - it issues a
WRITE SAME (or similar) command straight to the device to zero
the specified range. If there was cached data for this range it would
now be stale.
On Tue, Mar 1, 2016 at 8:09 PM, Darrick J. Wong [off-list ref] wrote:
Create a new ioctl to expose the block layer's newfound ability to
issue either a zeroing discard, a WRITE SAME with a zero page, or a
regular write with the zero page. This BLKZEROOUT2 ioctl takes
{start, length, flags} as parameters. So far, the only flag available
is to enable the zeroing discard part -- without it, the call invokes
the old BLKZEROOUT behavior. start and length have the same meaning
as in BLKZEROOUT.
NAK, just based on annoyance with the randomness of this interface:
- without describing what the point of the new flag and lots of extra
expansion room is, this should never be merged. We don't add padding
just randomly.
- Somewhat related to that: the flags are checked for zero, but the
random expansion room isn't. So not only are the random expansion
fields not explained, they will contain random garbage in the future.
- why is that "FMODE_WRITE" check not in the common code, but duplicated?
it all seems very ad-hoc. It makes a big deal about that idiotic
"discard" behavior, which is entirely pointless.
Who cares about that discard behavior anyway? Why is it set to "false"
in the current version of BLKZEROOUT in the first place? Why do we do
that WRITE_SAME without questioning it, but "discard" is somehow so
special that it has a flag, and it's turned off by default?
If this is some "security issue" where somebody believes that discard
is not secure, then those people are full of shit. Discard and
write-same have exactly the same semantics - they may just unmap the
target range with the guarantee that you'll get zeroes on read.
So quite frankly, right now it seems that
(a) the *only* excuse for this patch is that people want to use "discard"
(b) the reason we already don't use "discard" for the old BLKZEROOUT
is very questionable
(c) any future possible use of flags is not described and is questionable
You'll find people who think that "write-same with some non-zero
pattern" would be a real over-write and good for security. Those
people will then argue that a sane extension would be to make that
pattern part of the future expansion of BLKZEROOUT2. And those people
are full of shit. Write-same with a non-zero pattern may well be just
a discard with the pattern set in another table.
So the whole patch looks pointless.
Why isn't the patch just "change false to true in blk_ioctl_zeroout()
when it calls blkdev_issue_zeroout()".
No new interface, no new random padding, just a simple "it makes
absolutely no sense to not allow discard".
Linus
From: Darrick J. Wong <hidden> Date: 2016-03-02 22:56:01
On Wed, Mar 02, 2016 at 10:52:01AM -0800, Linus Torvalds wrote:
On Tue, Mar 1, 2016 at 8:09 PM, Darrick J. Wong [off-list ref] wrote:
quoted
Create a new ioctl to expose the block layer's newfound ability to
issue either a zeroing discard, a WRITE SAME with a zero page, or a
regular write with the zero page. This BLKZEROOUT2 ioctl takes
{start, length, flags} as parameters. So far, the only flag available
is to enable the zeroing discard part -- without it, the call invokes
the old BLKZEROOUT behavior. start and length have the same meaning
as in BLKZEROOUT.
NAK, just based on annoyance with the randomness of this interface:
- without describing what the point of the new flag and lots of extra
The new flag means "if the device supports discard and that discard zeroes data
then it's ok to use discard". The difference between discard/unmap and
write-same is in thin-provisioned storage arrays -- UNMAP can release backing
store, whereas WRITE SAME ensures that something's been written to media.
So by default, BLKZEROOUT2 means "ensure subsequent reads return zeroes and
make sure there's real space waiting for the next time I write to this".
Passing in the flag changes that to "ensure subsequent reads return zeroes and
I don't care what happens to the backing store".
I'll grant that this distinction could be clarified in the header file,
though anyone familiar with WS/UNMAP to want to use them from userspace
ought to know that already.
expansion room is, this should never be merged. We don't add padding
just randomly.
Oh yes we do. Adding required-zero padding to allow for future increases of
the expressiveness of an ioctl is very common.
$ egrep -rn '(reserved|padding).*;' include/uapi/ | wc -l
564
The size of the ioctl structure is embedded in the ioctl number definition,
so we might as well reserve a lot of space ahead of time. Better that than
having to declare new ioctl numbers every time someone needs to add something.
Both ext4 and XFS perform this sort of future proofing.
- Somewhat related to that: the flags are checked for zero, but the
random expansion room isn't. So not only are the random expansion
fields not explained, they will contain random garbage in the future.
Wrong. The padding fields /are/ checked, and the ioctl returns EINVAL
if they aren't zero:
static int blk_ioctl_zeroout2(struct block_device *bdev, fmode_t mode,
unsigned long arg)
{
...
if (p.padding || p.padding2)
return -EINVAL;
...
}
- why is that "FMODE_WRITE" check not in the common code, but duplicated?
The old BLKZEROOUT checked FMODE_WRITE before copying the ioctl data from
userspace, so the new BLKZEROOUT will behave the same way when possible to
minimize porting hassle for userland.
it all seems very ad-hoc. It makes a big deal about that idiotic
"discard" behavior, which is entirely pointless.
No. This is not about enabling use of "that idiotic discard behavior", for
that there's BLKDISCARD. This ioctl does NOT use the handwavy old TRIM
advisory request thing that could return "fuzzy wuzzy" without violating the
specs.
BLKZEROOUT is about telling a device "after this call I want subsequent reads
to return zeroes". The first patch fixes the problem that the pagecache isn't
invalidated when we tell the device that we want to be able to read zeroes.
Even if it behaves according to spec (and even if it doesn't) without that
patch, userland programs can end up reading stale data out of the kernel. That
first patch is purely a bug fix.
However, even once the cache coherence problem is fixed, there's still the
problem that the old BLKZEROOUT didn't maintain page cache coherence and
userspace has no way to figure out whether BLKZEROOUT on a given system
actually will. The reason for creating a new ioctl is to establish an ioctl
that guarantees a page cache flush or returns an error code. While we're
defining a new number, we might as well allow for userspace to control the
discard parameter to blkdev_issue_zeroout(), and reserve more space in the
structure than we think we need.
Who cares about that discard behavior anyway? Why is it set to "false"
in the current version of BLKZEROOUT in the first place? Why do we do
that WRITE_SAME without questioning it, but "discard" is somehow so
special that it has a flag, and it's turned off by default?
Some users want to use the SCSI command (WRITE SAME) that won't return
GOOD status until blocks full of zeroes have been committed to stable storage
that can be rewritten, and other users are fine with a zeroing TRIM/UNMAP which
can release the storage and DMA back pages of zeroes without committing
any stable storage to future rewrites.
If this is some "security issue" where somebody believes that discard
is not secure, then those people are full of shit. Discard and
write-same have exactly the same semantics - they may just unmap the
target range with the guarantee that you'll get zeroes on read.
No. The point is that mkfs can zero out filesystem metadata blocks with
confidence that a subsequent re-read will always return zeroes. We tried
making mke2fs use BLKZEROOUT and the cache coherence problem bit us in the
arse.
If you care about securely deleting data, throw it into the sun.
So quite frankly, right now it seems that
(a) the *only* excuse for this patch is that people want to use "discard"
(b) the reason we already don't use "discard" for the old BLKZEROOUT
is very questionable
See above.
(c) any future possible use of flags is not described and is questionable
How? If any of the other flag bits are set, it'll return -EINVAL. Or do you
mean that you think there will never be a need for any other flags? Or are
you complaining that you think there's too much padding and stuff in the
structure?
You'll find people who think that "write-same with some non-zero
pattern" would be a real over-write and good for security. Those
people will then argue that a sane extension would be to make that
pattern part of the future expansion of BLKZEROOUT2. And those people
are full of shit. Write-same with a non-zero pattern may well be just
a discard with the pattern set in another table.
So the whole patch looks pointless.
I disagree, obviously.
Why isn't the patch just "change false to true in blk_ioctl_zeroout()
when it calls blkdev_issue_zeroout()".
Some people might want real storage blocks full of zeroes backing the LBAs they
just wrote out, others might not care.
--D
No new interface, no new random padding, just a simple "it makes
absolutely no sense to not allow discard".
Linus
On Wed, Mar 2, 2016 at 2:56 PM, Darrick J. Wong [off-list ref] wrote:
Oh yes we do. Adding required-zero padding to allow for future increases of
the expressiveness of an ioctl is very common.
$ egrep -rn '(reserved|padding).*;' include/uapi/ | wc -l
564
Most of those should be for alignment reasons. In particular, you'll
find them to make sure subsequent u64's are properly aligned, or at
the end to make sure we have names for the padding at the end when the
last member is differently sized than the alignment of the structure.
Wrong. The padding fields /are/ checked, and the ioctl returns EINVAL
if they aren't zero:
Ok, I obviously overlooked that. Good.
quoted
- why is that "FMODE_WRITE" check not in the common code, but duplicated?
The old BLKZEROOUT checked FMODE_WRITE before copying the ioctl data from
userspace, so the new BLKZEROOUT will behave the same way when possible to
minimize porting hassle for userland.
That's a valid reason in theory, but I don't think anybody actually
cares. An error is an error. If you pass in invalid pointers _and_ a
read-only file descriptor, nobody will actually look at whether you
got EBADF or EFAULT.
No. This is not about enabling use of "that idiotic discard behavior", for
that there's BLKDISCARD. This ioctl does NOT use the handwavy old TRIM
advisory request thing that could return "fuzzy wuzzy" without violating the
specs.
So you agree that we could just make BLKZEROOUT always use trim?
BLKZEROOUT is about telling a device "after this call I want subsequent reads
to return zeroes". The first patch fixes the problem that the pagecache isn't
invalidated when we tell the device that we want to be able to read zeroes.
Even if it behaves according to spec (and even if it doesn't) without that
patch, userland programs can end up reading stale data out of the kernel. That
first patch is purely a bug fix.
Oh, I'm not at all arguing against the first patch.
It's the second one I think falls under the heading of "seems
over-engineered and unnecessary".
However, even once the cache coherence problem is fixed, there's still the
problem that the old BLKZEROOUT didn't maintain page cache coherence and
userspace has no way to figure out whether BLKZEROOUT on a given system
actually will. The reason for creating a new ioctl is to establish an ioctl
that guarantees a page cache flush or returns an error code. While we're
defining a new number, we might as well allow for userspace to control the
discard parameter to blkdev_issue_zeroout(), and reserve more space in the
structure than we think we need.
So quite frankly, this brings up three issues:
- why was this not explained in the commit message
- why would anybody *want* to control that parameter?
- what other parameters _could_ there be?
Some users want to use the SCSI command (WRITE SAME) that won't return
GOOD status until blocks full of zeroes have been committed to stable storage
that can be rewritten, and other users are fine with a zeroing TRIM/UNMAP which
can release the storage and DMA back pages of zeroes without committing
any stable storage to future rewrites.
Ehh. I'm not convinced that any disk that completes a TRIM command
without it beign "stable" would do the same for WRITE_SAME.
From everything I've ever seen, the difference between "stable on
disk" and "not stable" has almost never been about the particular
command used, and always been about the choice of the particular
target disk.
No. The point is that mkfs can zero out filesystem metadata blocks with
confidence that a subsequent re-read will always return zeroes. We tried
making mke2fs use BLKZEROOUT and the cache coherence problem bit us in the
arse.
Again, I don't disagree about the cache coherence at all.
How? If any of the other flag bits are set, it'll return -EINVAL. Or do you
mean that you think there will never be a need for any other flags? Or are
you complaining that you think there's too much padding and stuff in the
structure?
I absolutely *detest* code that tries to be overly forward-thinking by
randomly adding fields without even having an idea of what those
fields could possibly be used for.
I'm perfectly ok with padding that has a *reason*. Not a "we might use
it for something in the future".
Some people might want real storage blocks full of zeroes backing the LBAs they
just wrote out, others might not care.
.. but the flag doesn't even set that. Even if you avoid TRIM, there
is absolutely zero guarantees that WRITE_SAME would do "real storage
blocks full of zeroes backing the LBAs they just wrote out".
In fact, even if you do a real write of zeroed buffers, there's no
such guarantee. Doing some compression in the disk controller isn't
exactly unusual, even in enterprise hw (ie de-dup etc).
So I think this boils down to:
- I can see the "guaranteed cache flush". Get an error on old kernels
that might be without the flush.
At the same time, that sounds sad. The "add cache flush" sounds
like something that should be backported to stable, but new ioctl's?
Very questionable.
So now people who care would be forced to effectively do big zero
writes because they can't trust old kernels even if they are correct.
- it would seem that what you really want is a *generic* "invalidate
this page cache range" thing, because you have other users like the
whole "assemble SCSI command by hand" usage cases.
For example, maybe sync_file_range() should just be extended to
have a SYNC_FILE_RANGE_INVALIDATE flag? That sounds like a *much* more
generic thing that could be useful for other things than just the zero
write?
- I'd be much more ok with flags and extensions if they had
explanations and future cases.
IOW, this thing seems both too specific ("I need guarantees about
cache flushing, but only for this zerowrite thing") and too
"future-proofing" (I don't know what I might want in the future, but
zerowrite is such a generic operation that it migth want to have tons
of other arguments too").
It just rubs me the wrong way.
Linus
On Wed, Mar 02, 2016 at 03:49:53PM -0800, Linus Torvalds wrote:
quoted
No. This is not about enabling use of "that idiotic discard behavior", for
that there's BLKDISCARD. This ioctl does NOT use the handwavy old TRIM
advisory request thing that could return "fuzzy wuzzy" without violating the
specs.
So you agree that we could just make BLKZEROOUT always use trim?
There is a massive bug in the SATA specs about trim, which is that it
is considered advisory. So the storage device can throw it away
whenever it feels like it. (In practice, when it's too busy doing
other things).
The thing is, this fuzzy wuzzy definition of trim in the SATA specs is
more and more a SATA-specific bug. The eMMC 5.1 spec has a reliable
trim, and SCSI has WRITE SAME. So it's just SATA which has this crazy
definition of trim.
(In practice there is no real difference between trim and discard;
it's just a matter of terminology; in order to determine whether or
not trim/discard is reliable you really have to pay close attention to
the specs, on in the case of SATA, use a whitelist of drive models,
which is crazy.)
- Ted
On Thu, Mar 3, 2016 at 9:02 AM, Theodore Ts'o [off-list ref] wrote:
There is a massive bug in the SATA specs about trim, which is that it
is considered advisory. So the storage device can throw it away
whenever it feels like it. (In practice, when it's too busy doing
other things).
Ugh.
But that essentially says that we shouldn't expose this interface at
all (unless we trust our white-lists - I'm sure they are getting
better, but if nobody has ever really _relied_ on the zeroing behavior
of trim, then I guess there could be tons of bugs lurking).
Or maybe we should expose it, but not call it BLKZEROOUT, and make it
*much* more generic.
That migth actually put some of my complaints to rest: if this is more
a general "manage this range of blocks" model, then the flags make
more sense to me.
So what are people actually wanting to do?
If they don't care horribly about the zeroing, they might then say
"using trim is ok". But why wouldn't they use the BLKDISCARD ioctl
then?
So just looking at this more, that "trim is ok" flag still doesn't
make much sense to me.
I see two cases: either we guarantee zero-out behavior with discard
set to true (and we trust our whitelists), or we don't. Can anybody
see a third alternative?
And if we don't guarantee zero-out behavior from
blkdev_issue_zeroout() with "discard" set to true, then why would we
expose such a random interface to user space? No sane user space could
*possibly* use it: if they care about zeroing, it's the wrong thing to
do, and if they *don't* care about zeroing it's still the wrong thing
to do.
In other words, I still don't see how that flag can possibly make
sense in any possible scenario.
Put succinctly:
"Either we trust trim and and our whitelists (in which case _not_
using trim makes no sense), or we do (in which case exposing a random
untrustworthy user interface is pointless, since any user would be
fundamentally broken and should just have used BLKDISCARD)"
See where I'm coming from?
Now, the reason I think a more generic model that *isn't* hung up
about zeroing the buffer migth be ok is that maybe it would be a good
thing to have a more unified itnerface for doing all those things
people do want to do:
- flush caches
- discard (our current BLKDISCARD doesn't flush caches either, so
together with flushing caches this is something new)
- zero out
- synchronous/asynchronous
- other things?
So I do see a case for passing in multiple flags, but a lot of that
case ends up depending on the zeroing out *not* being the most central
feature.
I very much could see wanting "discard these blocks and flush caches".
And I could see just "flush caches", with or without zeroing. But I do
*not* see the point of "discard blocks and zero" for the reasons
outlined above.
Linus
From: Christoph Hellwig <hch@infradead.org> Date: 2016-03-03 18:00:07
On Thu, Mar 03, 2016 at 09:55:38AM -0800, Linus Torvalds wrote:
Ugh.
But that essentially says that we shouldn't expose this interface at
all (unless we trust our white-lists - I'm sure they are getting
better, but if nobody has ever really _relied_ on the zeroing behavior
of trim, then I guess there could be tons of bugs lurking).
Fortunately what Ted said is close to the truth, but a little off.
The mess about ATA is all real, and we work around it by never claiming
discard zeroes data for any ATA device except those specificly
whitelisted because the vendors gave guarantees for it (see the
ATA_HORKAGE_ZERO_AFTER_TRIM flag and the commit introducing it).
The reason why we don't use it unconditionally is simply because
there are users that want blocks zeroed without deallocating the
physical blocks. Now if you want to opt into the trim/unmap
behavior is one question, but we clearly need the split.
On Thu, Mar 03, 2016 at 09:55:38AM -0800, Linus Torvalds wrote:
But that essentially says that we shouldn't expose this interface at
all (unless we trust our white-lists - I'm sure they are getting
better, but if nobody has ever really _relied_ on the zeroing behavior
of trim, then I guess there could be tons of bugs lurking).
We don't, so this interface won't be useful for SATA disks, where
we'll need to write zeros until the SATA folks get off their duffs and
fix it with a new, reliable trim command.
But it will be useful for other storage systems, such as eMMC devices,
which *do* have a reliable trim command. So it may be that the first
place we'll see widepspread usage of this will be in the low-end and
high-end systems (where we can rely on eMMC's reliable trim and SCSI's
WRITE SAME command).
But that's why we want to have a new interface which is distinct from
BLKDISCARD. We want one interface for an advisory hint (we don't care
about the contents, so if it's convenient, feel free to forget about
the contents and replace it by zeros), and something where it truly is
a zeroout command. The intention is that BLKZEROOUT will be the
reliable zeroout command, while BLKDISCARD will be the unreliable
advisory hint.
- Ted