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

Groups > linux.kernel > #1347770 > unrolled thread

[PATCH v5.1 0/2] create BLKZEROOUT ioctl that invalidates page cache

Back to article view | Back to linux.kernel

Contents

  [PATCH v5.1 0/2] create BLKZEROOUT ioctl that invalidates page cache "Darrick J. Wong" <darrick.wong@oracle.com> - 2016-03-02 05:20 +0100
    [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks "Darrick J. Wong" <darrick.wong@oracle.com> - 2016-03-02 05:20 +0100
      Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Christoph Hellwig <hch@infradead.org> - 2016-03-02 10: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-02 20: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-03 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-03 01:00 +0100
            Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-03 18: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-03 19:00 +0100
                Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Christoph Hellwig <hch@infradead.org> - 2016-03-03 19:10 +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-03 19: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-03 19:30 +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-03 19:10 +0100
              Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Christoph Hellwig <hch@infradead.org> - 2016-03-03 19: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-03 19: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-03 20:00 +0100
                  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Theodore Ts'o <tytso@mit.edu> - 2016-03-03 23:50 +0100
                    Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-04 00: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-04 01:30 +0100
                  Re: [PATCH 2/2] block: create ioctl to discard-or-zeroout a range of  blocks Dave Chinner <david@fromorbit.com> - 2016-03-04 00:00 +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-04 03: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-03 19:20 +0100
    Re: [PATCH v5.1 0/2] create BLKZEROOUT ioctl that invalidates page cache Arnd Bergmann <arnd@arndb.de> - 2016-03-02 10:20 +0100
      Re: [PATCH v5.1 0/2] create BLKZEROOUT ioctl that invalidates page  cache Christoph Hellwig <hch@infradead.org> - 2016-03-02 10:50 +0100
        Re: [PATCH v5.1 0/2] create BLKZEROOUT ioctl that invalidates page cache Arnd Bergmann <arnd@arndb.de> - 2016-03-02 12:00 +0100

Page 1 of 2  [1] 2  Next page →

#1347770 — [PATCH v5.1 0/2] create BLKZEROOUT ioctl that invalidates page cache

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

[toc] | [next] | [standalone]

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

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 <darrick.wong@oracle.com>
---
 block/ioctl.c           |   57 ++++++++++++++++++++++++++++++++++++-----------
 include/uapi/linux/fs.h |    9 +++++++
 2 files changed, 53 insertions(+), 13 deletions(-)


diff --git a/block/ioctl.c b/block/ioctl.c
index c6eb462..5567466 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -222,24 +222,20 @@ static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
 	return blkdev_issue_discard(bdev, start, len, GFP_KERNEL, flags);
 }
 
-static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
-		unsigned long arg)
+static int __blk_ioctl_zeroout(struct block_device *bdev,
+			       unsigned long long start,
+			       unsigned long long len,
+			       unsigned int flags)
 {
-	uint64_t range[2];
 	struct address_space *mapping;
-	uint64_t start, end, len;
+	unsigned long long end;
+	bool discard = false;
 	int ret;
 
-	if (!(mode & FMODE_WRITE))
-		return -EBADF;
-
-	if (copy_from_user(range, (void __user *)arg, sizeof(range)))
-		return -EFAULT;
-
-	start = range[0];
-	len = range[1];
 	end = start + len - 1;
 
+	if (flags & ~BLKZEROOUT2_DISCARD_OK)
+		return -EINVAL;
 	if (start & 511)
 		return -EINVAL;
 	if (len & 511)
@@ -253,8 +249,10 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
 	mapping = bdev->bd_inode->i_mapping;
 	truncate_inode_pages_range(mapping, start, end);
 
+	if (flags & BLKZEROOUT2_DISCARD_OK)
+		discard = true;
 	ret = blkdev_issue_zeroout(bdev, start >> 9, len >> 9, GFP_KERNEL,
-				    false);
+				   discard);
 	if (ret)
 		return ret;
 
@@ -267,6 +265,37 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
 					     end >> PAGE_CACHE_SHIFT);
 }
 
+static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
+		unsigned long arg)
+{
+	uint64_t range[2];
+
+	if (!(mode & FMODE_WRITE))
+		return -EBADF;
+
+	if (copy_from_user(range, (void __user *)arg, sizeof(range)))
+		return -EFAULT;
+
+	return __blk_ioctl_zeroout(bdev, range[0], range[1], 0);
+}
+
+static int blk_ioctl_zeroout2(struct block_device *bdev, fmode_t mode,
+		unsigned long arg)
+{
+	struct blkzeroout2 p;
+
+	if (!(mode & FMODE_WRITE))
+		return -EBADF;
+
+	if (copy_from_user(&p, (void __user *)arg, sizeof(p)))
+		return -EFAULT;
+
+	if (p.padding || p.padding2)
+		return -EINVAL;
+
+	return __blk_ioctl_zeroout(bdev, p.start, p.length, p.flags);
+}
+
 static int put_ushort(unsigned long arg, unsigned short val)
 {
 	return put_user(val, (unsigned short __user *)arg);
@@ -560,6 +589,8 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 				BLKDEV_DISCARD_SECURE);
 	case BLKZEROOUT:
 		return blk_ioctl_zeroout(bdev, mode, arg);
+	case BLKZEROOUT2:
+		return blk_ioctl_zeroout2(bdev, mode, arg);
 	case HDIO_GETGEO:
 		return blkdev_getgeo(bdev, argp);
 	case BLKRAGET:
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 149bec8..4c7a376 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -222,6 +222,15 @@ struct fsxattr {
 #define BLKSECDISCARD _IO(0x12,125)
 #define BLKROTATIONAL _IO(0x12,126)
 #define BLKZEROOUT _IO(0x12,127)
+struct blkzeroout2 {
+	__u64 start;
+	__u64 length;
+	__u32 flags;
+	__u32 padding;
+	__u64 padding2;
+};
+#define BLKZEROOUT2_DISCARD_OK	1
+#define BLKZEROOUT2 _IOR(0x12, 127, struct blkzeroout2)
 #define BLKDAXGET _IO(0x12,129)
 
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */

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

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

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

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

On Tue, Mar 1, 2016 at 8:09 PM, Darrick J. Wong <darrick.wong@oracle.com> 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

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

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

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 <darrick.wong@oracle.com> 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

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

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

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

On Wed, Mar 2, 2016 at 2:56 PM, Darrick J. Wong <darrick.wong@oracle.com> 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.

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

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

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

On Wed, Mar 02, 2016 at 03:49:53PM -0800, Linus Torvalds wrote:
> > 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

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

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

On Thu, Mar 3, 2016 at 9:02 AM, Theodore Ts'o <tytso@mit.edu> 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

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

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

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.

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

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

>>>>> "Linus" == Linus Torvalds <torvalds@linux-foundation.org> writes:

Linus> On Thu, Mar 3, 2016 at 9:02 AM, Theodore Ts'o <tytso@mit.edu> 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).

Linus> Ugh.

SCSI UNMAP provides similar semantics :(

Linus> But that essentially says that we shouldn't expose this interface
Linus> at all (unless we trust our white-lists - I'm sure they are
Linus> getting better, but if nobody has ever really _relied_ on the
Linus> zeroing behavior of trim, then I guess there could be tons of
Linus> bugs lurking).

We started out with the blacklist approach and it blew up. So now we're
down to a whitelist of drives that have been sanctioned by their
manufacturers to do the right thing.

Occasionally a new SSD model messes things up but we haven't updated it
since last summer. The drive vendors are much better at testing using
Linux than they used to be.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

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

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

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

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

>>>>> "Linus" == Linus Torvalds <torvalds@linux-foundation.org> writes:

Linus> .. but the flag doesn't even set that. Even if you avoid TRIM,
Linus> there is absolutely zero guarantees that WRITE_SAME would do
Linus> "real storage blocks full of zeroes backing the LBAs they just
Linus> wrote out".

That's not entirely true. Writing the blocks may cause them to be
allocated on the storage device (depending on which flags we feed it in
WRITE SAME).

The filesystems people were wanted the following semantics:

 - deallocate, don't care about contents for future reads (discard)
 - deallocate, guarantee zeroes on future reads (zeroout)
 - (re)allocate, guarantee zeroes on future reads (zeroout)

Maybe we just need a better naming scheme...

-- 
Martin K. Petersen	Oracle Linux Engineering

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

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

On Thu, Mar 03, 2016 at 01:01:11PM -0500, Martin K. Petersen wrote:
> That's not entirely true. Writing the blocks may cause them to be
> allocated on the storage device (depending on which flags we feed it in
> WRITE SAME).
> 
> The filesystems people were wanted the following semantics:
> 
>  - deallocate, don't care about contents for future reads (discard)
>  - deallocate, guarantee zeroes on future reads (zeroout)
>  - (re)allocate, guarantee zeroes on future reads (zeroout)
> 
> Maybe we just need a better naming scheme...

In filesystem terms we have two and three:

 - FALLOC_FL_PUNCH_HOLE assures zeroes are returned, but space is
   deallocated as much as possible
 - FALLOC_FL_ZERO_RANGE assures zeroes are returned, AND blocks are
   actually allocated

Returning stale blocks in a file system is a nasty security risk, so
we don't do that, and so shouldn't storage that offers any kind
of multi tenancy, and if it's just VMs using multiple partitions on it.

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

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

On Thu, Mar 03, 2016 at 10:09:24AM -0800, Christoph Hellwig wrote:
> On Thu, Mar 03, 2016 at 01:01:11PM -0500, Martin K. Petersen wrote:
> > That's not entirely true. Writing the blocks may cause them to be
> > allocated on the storage device (depending on which flags we feed it in
> > WRITE SAME).
> > 
> > The filesystems people were wanted the following semantics:
> > 
> >  - deallocate, don't care about contents for future reads (discard)
> >  - deallocate, guarantee zeroes on future reads (zeroout)
> >  - (re)allocate, guarantee zeroes on future reads (zeroout)
> > 
> > Maybe we just need a better naming scheme...
> 
> In filesystem terms we have two and three:
> 
>  - FALLOC_FL_PUNCH_HOLE assures zeroes are returned, but space is
>    deallocated as much as possible
>  - FALLOC_FL_ZERO_RANGE assures zeroes are returned, AND blocks are
>    actually allocated
> 
> Returning stale blocks in a file system is a nasty security risk, so
> we don't do that, and so shouldn't storage that offers any kind
> of multi tenancy, and if it's just VMs using multiple partitions on it.

Any particular reason why we can't just implement those two fallocate
flags for block devices?

--D

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

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

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph>  - FALLOC_FL_PUNCH_HOLE assures zeroes are returned, but
Christoph> space is deallocated as much as possible -
Christoph> FALLOC_FL_ZERO_RANGE assures zeroes are returned, AND blocks
Christoph> are actually allocated

That works for me. I think it would be great if we could have consistent
interfaces for fs and block. The more commonality the merrier.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

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

On Thu, Mar 03, 2016 at 01:54:54PM -0500, Martin K. Petersen wrote:
> >>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:
> 
> Christoph>  - FALLOC_FL_PUNCH_HOLE assures zeroes are returned, but
> Christoph> space is deallocated as much as possible -
> Christoph> FALLOC_FL_ZERO_RANGE assures zeroes are returned, AND blocks
> Christoph> are actually allocated
> 
> That works for me. I think it would be great if we could have consistent
> interfaces for fs and block. The more commonality the merrier.

So a question I have is do we want to add a "discard-as-a-hint" analog
for fallocate?  In the past we've said no because we don't trust
userspace.  (We seem to have as a core held belief that application
programmers are idiots and are not to be trusted with anything
dangerous, even if it would be highly useful in certain use cases.)

As a result I'm carrying an out-of-tree patch in our Google kernels so
that ext4 will honor BLKDISCARD on files.  I can't remember if I
floated it on linux-fsdevel, or I didn't bother because I knew it
would instantly shot down.  I believe it was the former, but I can't
be 100% sure.

It would be kind of nice, though, if we had some kind of agreement on
a consistent, unified interface for all three kinds of
"discard-as-a-hint", "zeroout", and "zeroout with deallocation" that
worked on block devices and files.  Whether it's via fallocate(2) or
BLK* ioctls, I'm agnostic.

							- Ted

P.S.  Speaking of things that are powerful and too dangerous for
application programmers, after the Linux FAST workshop, I was having
dinner with the Ceph developers and Ric Wheeler, and we were talking
about things they really needed.  Turns out they also could use an
FALLOC_FL_NO_HIDE_STALE functionality.  I told them I had an
out-of-tree patch that had that functionality, and even Ric Wheeler
started getting tempted....  :-)

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

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

On Thu, Mar 03, 2016 at 05:39:52PM -0500, Theodore Ts'o wrote:
> On Thu, Mar 03, 2016 at 01:54:54PM -0500, Martin K. Petersen wrote:
> > >>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:
> > 
> > Christoph>  - FALLOC_FL_PUNCH_HOLE assures zeroes are returned, but
> > Christoph> space is deallocated as much as possible -
> > Christoph> FALLOC_FL_ZERO_RANGE assures zeroes are returned, AND blocks
> > Christoph> are actually allocated
> > 
> > That works for me. I think it would be great if we could have consistent
> > interfaces for fs and block. The more commonality the merrier.
> 
> So a question I have is do we want to add a "discard-as-a-hint" analog
> for fallocate?

Well defined, reliable behaviour only, please. If the device can't
provide the required hardware offload, then it needs to use the
generic, slow implementation of the functionality or report
EOPNOTSUPP.

> P.S.  Speaking of things that are powerful and too dangerous for
> application programmers, after the Linux FAST workshop, I was having
> dinner with the Ceph developers and Ric Wheeler, and we were talking
> about things they really needed.  Turns out they also could use an
> FALLOC_FL_NO_HIDE_STALE functionality. 

For better or for worse, Ceph is moving away from using filesystems
for its back end object store, so the use of such a hack in Ceph
has a very limited life.

> I told them I had an
> out-of-tree patch that had that functionality, and even Ric Wheeler
> started getting tempted....  :-)

You can tempt all you want, but it does not change the basic fact
that it is dangerous and compromises system security. As such, it
does not belong in upstream kernels. Especially in this day and age
where ensuring the fundamental integrity of our systems is more
important than ever.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

On Fri, Mar 04, 2016 at 10:10:50AM +1100, Dave Chinner wrote:
> You can tempt all you want, but it does not change the basic fact
> that it is dangerous and compromises system security. As such, it
> does not belong in upstream kernels. Especially in this day and age
> where ensuring the fundamental integrity of our systems is more
> important than ever.

The fact of the matter is as there are more user-space servers for
cluster file systems which are trusted to do the right thing, and are
considered inside the TCB, the demand for this is going to strong.  We
do actually use a group id to provide access control to what is
admittedly a dangerous feature, so it's not available for all
userspace callers, and we didn't want the file server to run as root.

But I'll let the Ceph folks advocate internally for such a feature to
be shipped in RHEL if they feel strongly about it, and if later one we
want to come to an agreement about a better access control mechanism
than membership in a privileged group id passed in as a mount option,
I'm not wedded to that facility.  I didn't think it was worth cracking
open a new capability bit, but if we want to go down that route, or
some other route to provide the appropriate security controls, we
should definitely talk about it.

Cheers,

						- Ted

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

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

On Thu, Mar 03, 2016 at 01:54:54PM -0500, Martin K. Petersen wrote:
> >>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:
> 
> Christoph>  - FALLOC_FL_PUNCH_HOLE assures zeroes are returned, but
> Christoph> space is deallocated as much as possible -
> Christoph> FALLOC_FL_ZERO_RANGE assures zeroes are returned, AND blocks
> Christoph> are actually allocated
> 
> That works for me. I think it would be great if we could have consistent
> interfaces for fs and block. The more commonality the merrier.

Absolutely in agreement here. it would be much nicer if filesystems
could just call bdev->ops->fallocate(PUNCH_HOLE, off, len) and
bdev->ops->fallocate(ZERO_RANGE, off, len) than all the weird
"technology specific" blkdev_issue_foo() functions we have grown
over time. Let the block device implement them as it sees fit - the
higher levels don't need to care about protocol/technology details.

---

FWIW, this reminds me of a "bigger picture" I think we should
be working towards. Does anyone remember this:

https://lwn.net/Articles/592091/

(Splitting filesytems in two)

i.e. if we add fallocate support to punch holes, zero ranges and
*allocate blocks* to a block device, we're mostly at the point where
we can offload all freespace management that the filesystem
currently does to the underlying block device.

There's really only a small extension we'd need - the block
allocation done by the block device needs to be able to return the
the sector and length of the newly allocated extent. Indeed, this is
something we talked about last year at LSFMM as a solution to the
SMR write ordering problem:

https://lwn.net/Articles/637035/

(near the end, paragraph talking about a "new kind of write command")

That "new kind of write command" would enable delayed allocation
algorithms to continue to work at the filesystem level on block
devices that freespace management completely is offloaded to...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

On 03/03/2016 11:56 PM, Dave Chinner wrote:
> That "new kind of write command" would enable delayed allocation
> algorithms to continue to work at the filesystem level on block
> devices that freespace management completely is offloaded to...
> Cheers, Dave. 

This would advocate a uniform /internal/ interface (family) across both
fs and block layers, similiar in spirit to my old Athomux research
prototype long ago (see www.athomux.net).

This allows for recursive nesting in complex (distributed) storage/fs
hierarchies.

It would be nice if that internal interface (family) would be (partly /
fully) asynchronous with callbacks. In ideal case, it should be
compatible with workqueues (no need for blocking threads anymore).

Uniformity is only needed at concept level. There might remain different
flavours of concrete interfaces at different subsystems, if the number
of subsystems remains as small as possible, and interfacing is close to
trivial.

I would like to support this also in future versions of MARS (see
github.com/schoebel/mars).

Cheers, Thomas

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web