[PATCH v6 0/3] fallocate for block devices to provide zero-out

Hi,

This is a redesign of the patch series that fixes various interface
problems with the existing "zero out this part of a block device"
code.  BLKZEROOUT2 is gone.

The first patch is still a fix to the existing BLKZEROOUT ioctl to
invalidate the page cache if the zeroing command to the underlying
device succeeds.

The second patch changes the internal block device functions to reject
attempts to discard or zeroout that are not aligned to the logical
block size.  Previously, we only checked that the start/len parameters
were 512-byte aligned, which caused kernel BUG_ONs for unaligned IOs
to 4k-LBA devices.

The third patch creates an fallocate handler for block devices, wires
up the FALLOC_FL_PUNCH_HOLE flag to zeroing-discard, and connects
FALLOC_FL_ZERO_RANGE to write-same so that we can have a consistent
fallocate interface between files and block devices.

Test cases[1] for the new block device fallocate will have been
submitted to the xfstests list as generic/70[5-7], though the
numbering will change to a lower number when the API and the tests are
accepted upstream.

Comments and questions are, as always, welcome.  Patches are against
4.5-rc6.

--D

[1] https://github.com/djwong/xfstests/commit/fdc0980ef01076dfb246fd1db2511227e9f67a3f

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

diff --git a/block/ioctl.c b/block/ioctl.c
index d8996bb..c6eb462 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c

@@ -226,7 +226,9 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
 		unsigned long arg)
 {
 	uint64_t range[2];
-	uint64_t start, len;
+	struct address_space *mapping;
+	uint64_t start, end, len;
+	int ret;
 
 	if (!(mode & FMODE_WRITE))
 		return -EBADF;

@@ -236,18 +238,33 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
 
 	start = range[0];
 	len = range[1];
+	end = start + len - 1;
 
 	if (start & 511)
 		return -EINVAL;
 	if (len & 511)
 		return -EINVAL;
-	start >>= 9;
-	len >>= 9;
-
-	if (start + len > (i_size_read(bdev->bd_inode) >> 9))
+	if (end >= (uint64_t)i_size_read(bdev->bd_inode))
+		return -EINVAL;
+	if (end < start)
 		return -EINVAL;
 
-	return blkdev_issue_zeroout(bdev, start, len, GFP_KERNEL, false);
+	/* Invalidate the page cache, including dirty pages */
+	mapping = bdev->bd_inode->i_mapping;
+	truncate_inode_pages_range(mapping, start, end);
+
+	ret = blkdev_issue_zeroout(bdev, start >> 9, len >> 9, GFP_KERNEL,
+				    false);
+	if (ret)
+		return ret;
+
+	/*
+	 * Invalidate again; if someone wandered in and dirtied a page,
+	 * the caller will be given -EBUSY.
+	 */
+	return invalidate_inode_pages2_range(mapping,
+					     start >> PAGE_CACHE_SHIFT,
+					     end >> PAGE_CACHE_SHIFT);
 }
 
 static int put_ushort(unsigned long arg, unsigned short val)

After much discussion, it seems that the fallocate feature flag
FALLOC_FL_ZERO_RANGE maps nicely to SCSI WRITE SAME; and the feature
FALLOC_FL_PUNCH_HOLE maps nicely to the devices that have been
whitelisted for zeroing SCSI UNMAP.  Both flags require that
FALLOC_FL_KEEP_SIZE are set, both return EINVAL if one tries
to write past the end of the device, and both require that the
offset and length be aligned at least to 512-byte offsets.q

Since the semantics of fallocate are fairly well established already,
wire up the two pieces.  The other fallocate variants (collapse range,
insert range, and allocate blocks) are not supported.

Signed-off-by: Darrick J. Wong <redacted>
---
 fs/block_dev.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/open.c      |    3 ++-
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 826b164..c9c9421 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c

@@ -30,6 +30,7 @@
 #include <linux/cleancache.h>
 #include <linux/dax.h>
 #include <asm/uaccess.h>
+#include <linux/falloc.h>
 #include "internal.h"
 
 struct bdev_inode {

@@ -1786,6 +1787,71 @@ static int blkdev_mmap(struct file *file, struct vm_area_struct *vma)
 #define blkdev_mmap generic_file_mmap
 #endif
 
+#define	BLKDEV_FALLOC_FL_SUPPORTED					\
+		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
+		 FALLOC_FL_ZERO_RANGE)
+
+long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
+{
+	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
+	struct request_queue *q = bdev_get_queue(bdev);
+	struct address_space *mapping;
+	loff_t end = start + len - 1;
+	loff_t bs_mask;
+	int error;
+
+	/* We only support zero range and punch hole. */
+	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
+		return -EOPNOTSUPP;
+
+	/* We can't change the bdev size from here */
+	if (!(mode & FALLOC_FL_KEEP_SIZE))
+		return -EOPNOTSUPP;
+
+	/* We haven't a primitive for "ensure space exists" right now. */
+	if (mode == FALLOC_FL_KEEP_SIZE)
+		return -EOPNOTSUPP;
+
+	/* Only punch if the device can do zeroing discard. */
+	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
+	    (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))
+		return -EOPNOTSUPP;
+
+	/* Don't allow IO that isn't aligned to logical block size */
+	bs_mask = bdev_logical_block_size(bdev) - 1;
+	if ((start & bs_mask) || ((start + len) & bs_mask))
+		return -EINVAL;
+
+	/* Don't go off the end of the device */
+	if (end > i_size_read(bdev->bd_inode))
+		return -EINVAL;
+	if (end < start)
+		return -EINVAL;
+
+	/* Invalidate the page cache, including dirty pages. */
+	mapping = bdev->bd_inode->i_mapping;
+	truncate_inode_pages_range(mapping, start, end);
+
+	error = -EINVAL;
+	if (mode & FALLOC_FL_ZERO_RANGE)
+		error = blkdev_issue_zeroout(bdev, start >> 9, len >> 9,
+					    GFP_KERNEL, false);
+	else if (mode & FALLOC_FL_PUNCH_HOLE)
+		error = blkdev_issue_discard(bdev, start >> 9, len >> 9,
+					     GFP_KERNEL, 0);
+	if (error)
+		return error;
+
+	/*
+	 * Invalidate again; if someone wandered in and dirtied a page,
+	 * the caller will be given -EBUSY;
+	 */
+	return invalidate_inode_pages2_range(mapping,
+					     start >> PAGE_CACHE_SHIFT,
+					     end >> PAGE_CACHE_SHIFT);
+}
+EXPORT_SYMBOL_GPL(blkdev_fallocate);
+
 const struct file_operations def_blk_fops = {
 	.open		= blkdev_open,
 	.release	= blkdev_close,

@@ -1800,6 +1866,7 @@ const struct file_operations def_blk_fops = {
 #endif
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.fallocate	= blkdev_fallocate,
 };
 
 int ioctl_by_bdev(struct block_device *bdev, unsigned cmd, unsigned long arg)

diff --git a/fs/open.c b/fs/open.c
index 55bdc75..4f99adc 100644
--- a/fs/open.c
+++ b/fs/open.c

@@ -289,7 +289,8 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
-	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode) &&
+	    !S_ISBLK(inode->i_mode))
 		return -ENODEV;
 
 	/* Check for wrap through zero too */

Make sure that the offset and length arguments that we're using to
construct WRITE SAME and DISCARD requests are actually aligned to the
logical block size.  Failure to do this causes other errors in other
parts of the block layer or the SCSI layer because disks don't support
partial logical block writes.

Signed-off-by: Darrick J. Wong <redacted>
---
 block/blk-lib.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 9ebf653..3e5ca28 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c

@@ -49,6 +49,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	struct bio *bio;
 	int ret = 0;
 	struct blk_plug plug;
+	sector_t bs_mask;
 
 	if (!q)
 		return -ENXIO;

@@ -56,6 +57,10 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if (!blk_queue_discard(q))
 		return -EOPNOTSUPP;
 
+	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
+	if ((sector & bs_mask) || ((sector + nr_sects) & bs_mask))
+		return -EINVAL;
+
 	/* Zero-sector (unknown) and one-sector granularities are the same.  */
 	granularity = max(q->limits.discard_granularity >> 9, 1U);
 	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;

@@ -148,6 +153,7 @@ int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 	DECLARE_COMPLETION_ONSTACK(wait);
 	struct request_queue *q = bdev_get_queue(bdev);
 	unsigned int max_write_same_sectors;
+	sector_t bs_mask;
 	struct bio_batch bb;
 	struct bio *bio;
 	int ret = 0;

@@ -155,6 +161,10 @@ int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 	if (!q)
 		return -ENXIO;
 
+	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
+	if ((sector & bs_mask) || ((sector + nr_sects) & bs_mask))
+		return -EINVAL;
+
 	/* Ensure that max_write_same_sectors doesn't overflow bi_size */
 	max_write_same_sectors = UINT_MAX >> 9;
 

@@ -218,9 +228,14 @@ static int __blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	int ret;
 	struct bio *bio;
 	struct bio_batch bb;
+	sector_t bs_mask;
 	unsigned int sz;
 	DECLARE_COMPLETION_ONSTACK(wait);
 
+	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
+	if ((sector & bs_mask) || ((sector + nr_sects) & bs_mask))
+		return -EINVAL;
+
 	atomic_set(&bb.done, 1);
 	bb.error = 0;
 	bb.wait = &wait;

On Fri, Mar 4, 2016 at 4:56 PM, Darrick J. Wong [off-list ref] wrote:

+       bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
+       if ((sector & bs_mask) || ((sector + nr_sects) & bs_mask))
+               return -EINVAL;

This test may _work_, but it's kind of crazily overly complicated.

The sane test would be just "are the start and length aligned":

        if ((sector & bs_mask) || (nr_sects & bs_mask))
                return -EINVAL;

and the *smart* test is simpler still, and asks "are there invalid
bits in either the start or the length":

        if ((sector | nr_sects) & bs_mask)
                return -EINVAL:

I suspect either of these would be fine, and the compiler may even
notice that there's the smart way of doing it.

The compiler *might* even notice that the original version can be
simplified and generate sane code.

But I think that original version is not only overly complicated, it's
also actually less obvious than the simpler versions, if only because
the whole conditional is so big that you have to actively parse it.

That last shortest form is actually so simple that I think it's the
easiest to understand too - the conditional is simply so small that it
doesn't take a lot of effort to see what it does.

            Linus

On Fri, Mar 4, 2016 at 4:56 PM, Darrick J. Wong [off-list ref] wrote:

+       /* Only punch if the device can do zeroing discard. */
+       if ((mode & FALLOC_FL_PUNCH_HOLE) &&
+           (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))
+               return -EOPNOTSUPP;

I'm ok with this, but suspect that some users would prefer to just
turn this into ZERO_RANGE silently.

Comments from people who would be expected to use this?

            Linus

On Fri, Mar 4, 2016 at 4:56 PM, Darrick J. Wong [off-list ref] wrote:

+
+       /* We can't change the bdev size from here */
+       if (!(mode & FALLOC_FL_KEEP_SIZE))
+               return -EOPNOTSUPP;

Oh, and this I think is wrong.

The thing is, FALLOC_FL_KEEP_SIZE is only supposed to matter if the
region is outside the existing length.

So if y ou punch a hole in the middle of a file, you don't need
FALLOC_FL_KEEP_SIZE.

I would suggest removing this check entirely, since you already check
that people don't try to punch holes past the end of the device. So
FALLOC_FL_KEEP_SIZE is simply a non-issue, and shouldn't even be
checked.

              Linus

On Fri, Mar 4, 2016 at 4:55 PM, Darrick J. Wong [off-list ref] wrote:

This is a redesign of the patch series that fixes various interface
problems with the existing "zero out this part of a block device"
code.  BLKZEROOUT2 is gone.

I replied to two of the patches with small nits, but on the whole I
like it. The only nit that I think is serious is that you shouldn't be
requiring people to have to use FALLOC_FL_KEEP_SIZE.

If anything, you might just change that logic to say: If
FALLOC_FL_KEEP_SIZE is set, then we truncate the hole punching/zeroing
to the size of the device. So people could do something like

  fallocate(fd,
        FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE,
        0, ~0);

to trim the whole device (and if FALLOC_FL_KEEP_SIZE isn't set, then
the above would be an error simply because the requested hole was
bigger than the device)

            Linus

On Fri, Mar 04, 2016 at 07:06:38PM -0800, Linus Torvalds wrote:

quoted

+       if ((mode & FALLOC_FL_PUNCH_HOLE) &&
+           (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))
+               return -EOPNOTSUPP;

I'm ok with this, but suspect that some users would prefer to just
turn this into ZERO_RANGE silently.

Comments from people who would be expected to use this?

A hole punch should be a hole punch, and not silently allocate blocks
isntead of deallocating them.  It's not even a fallback, it's pretty
much the opposite for some workloads.

On Fri, Mar 04, 2016 at 07:13:25PM -0800, Linus Torvalds wrote:

quoted

+       /* We can't change the bdev size from here */
+       if (!(mode & FALLOC_FL_KEEP_SIZE))
+               return -EOPNOTSUPP;

Oh, and this I think is wrong.

The thing is, FALLOC_FL_KEEP_SIZE is only supposed to matter if the
region is outside the existing length.

For allocations...

So if y ou punch a hole in the middle of a file, you don't need
FALLOC_FL_KEEP_SIZE.

For FALLOC_FL_PUNCH_HOLE we always require FALLOC_FL_KEEP_SIZE so far,
and I'd rather not change things for block devices just because we can.

On Fri, Mar 04, 2016 at 04:56:03PM -0800, Darrick J. Wong wrote:

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>

Didn't I review this already?  If not:

Reviewed-by: Christoph Hellwig <redacted>

On Fri, Mar 04, 2016 at 04:56:10PM -0800, Darrick J. Wong wrote:

Make sure that the offset and length arguments that we're using to
construct WRITE SAME and DISCARD requests are actually aligned to the
logical block size.  Failure to do this causes other errors in other
parts of the block layer or the SCSI layer because disks don't support
partial logical block writes.

Looks fine,

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