[PATCH v7 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 for the new block device fallocate 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.
Look for the v2 testcase patch, which reflects v7 of this patchset.

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

v7: Strengthen parameter checking and fix various code issues pointed
out by Linus and Christoph.

--D

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-lib.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 9ebf653..9dca6bb 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 | 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 | 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 | nr_sects) & bs_mask)
+		return -EINVAL;
+
 	atomic_set(&bb.done, 1);
 	bb.error = 0;
 	bb.wait = &wait;

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>
Reviewed-by: Christoph Hellwig <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.  Punch still requires that
FALLOC_FL_KEEP_SIZE is set.  A length that goes past the end of the
device will be clamped to the device size if KEEP_SIZE is set; or will
return -EINVAL if not.  Both start and length must be aligned to the
device's logical block size.

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 |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/open.c      |    3 ++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 826b164..6137c6e 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,73 @@ 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, isize;
+	int error;
+
+	/* We only support zero range and punch hole. */
+	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
+		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 go off the end of the device */
+	isize = i_size_read(bdev->bd_inode);
+	if (start >= isize)
+		return -EINVAL;
+	if (end > isize) {
+		if (mode & FALLOC_FL_KEEP_SIZE) {
+			len = isize - start;
+			end = start + len - 1;
+		} else
+			return -EINVAL;
+	}
+
+	/* Don't allow IO that isn't aligned to logical block size */
+	bs_mask = bdev_logical_block_size(bdev) - 1;
+	if ((start | len) & bs_mask)
+		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 +1868,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 */

On Tue, Mar 15, 2016 at 12:42:44PM -0700, Darrick J. Wong wrote:

 #include <linux/cleancache.h>
 #include <linux/dax.h>
 #include <asm/uaccess.h>
+#include <linux/falloc.h>

Maybe keep this before asm/uaccess.h

+long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)

should be marked static.

+	/* We haven't a primitive for "ensure space exists" right now. */
+	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
+		return -EOPNOTSUPP;

I don't really understand the comment.  But I think you'd be much
better off with having blkdev_fallocate as just a tiny wrapper that has
a switch for the supported modes, e.g.

	switch (mode) {
	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE:
		return blkdev_punch_hole();
	case FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE::
		return blkdev_zero_range();
	default:
		return -EOPNOTSUPP;
	}

+EXPORT_SYMBOL_GPL(blkdev_fallocate);

and no need to export it either..

On Mon, Mar 21, 2016 at 08:38:27AM -0700, Christoph Hellwig wrote:

On Tue, Mar 15, 2016 at 12:42:44PM -0700, Darrick J. Wong wrote:

quoted

 #include <linux/cleancache.h>
 #include <linux/dax.h>
 #include <asm/uaccess.h>
+#include <linux/falloc.h>

Maybe keep this before asm/uaccess.h

quoted

+long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)

should be marked static.

Ok (to both).

quoted

+	/* We haven't a primitive for "ensure space exists" right now. */
+	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
+		return -EOPNOTSUPP;

I don't really understand the comment.  But I think you'd be much

I don't know of a block device primitive that corresponds to the "default"
mode of fallocate, as documented in the manpage (i.e. mode == 0).  I agree
that the whole thing could be simplified in the manner you point out below.

better off with having blkdev_fallocate as just a tiny wrapper that has
a switch for the supported modes, e.g.

	switch (mode) {
	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE:
		return blkdev_punch_hole();
	case FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE::
		return blkdev_zero_range();
	default:
		return -EOPNOTSUPP;
	}

quoted

+EXPORT_SYMBOL_GPL(blkdev_fallocate);

and no need to export it either..

Ok.

--D

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Mar 21, 2016 at 10:52:35AM -0700, Darrick J. Wong wrote:

quoted

I don't really understand the comment.  But I think you'd be much

I don't know of a block device primitive that corresponds to the "default"
mode of fallocate, as documented in the manpage (i.e. mode == 0).  I agree
that the whole thing could be simplified in the manner you point out below.

SCSI allows 'anchoring' blocks, which is pretty similar to a normal
fallocate, but we don't support anchoring blocks in Linux yet.

On Tue, Mar 15, 2016 at 3:42 PM, Darrick J. Wong
[off-list ref] wrote:

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.  Punch still requires that
FALLOC_FL_KEEP_SIZE is set.  A length that goes past the end of the
device will be clamped to the device size if KEEP_SIZE is set; or will
return -EINVAL if not.  Both start and length must be aligned to the
device's logical block size.

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.

I'd like to see fallocate (block allocation) extend down to DM thinp.
This more traditional use of fallocate would be useful for ensuring
ENOSPC won't occur -- especially important if the FS has committed
space in response to fallocate.  As of now fallocate doesn't inform DM
thinp at all.  Curious why you decided not to wire it up?

But I'm not sure what "it" (the "allocate blocks" variant) even is
given falloc.h doesn't show anything like "_ALLOCATE_BLOCKS"...

It would require a new block interface to pass the fallocate extent
down.  But it seems bizarre to implement "some of" fallocate but not
the most widely used case for fallocate.

Mike

On Mon, Mar 21 2016 at  3:11pm -0400,
Darrick J. Wong [off-list ref] wrote:

On Mon, Mar 21, 2016 at 02:52:00PM -0400, Mike Snitzer wrote:

quoted

On Tue, Mar 15, 2016 at 3:42 PM, Darrick J. Wong
[off-list ref] wrote:

quoted

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.  Punch still requires that
FALLOC_FL_KEEP_SIZE is set.  A length that goes past the end of the
device will be clamped to the device size if KEEP_SIZE is set; or will
return -EINVAL if not.  Both start and length must be aligned to the
device's logical block size.

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.

I'd like to see fallocate (block allocation) extend down to DM thinp.
This more traditional use of fallocate would be useful for ensuring
ENOSPC won't occur -- especially important if the FS has committed
space in response to fallocate.  As of now fallocate doesn't inform DM
thinp at all.  Curious why you decided not to wire it up?

I don't know what to wire it up to. :)

Fair enough.  Yes something needs to be invented.
 

I didn't find any blkdev_* function that looked encouraging, though I
haven't dug too deeply into bfoster's "prototype a block reservation
allocation model" patchset yet.  At a high level I'd guess that would
be a reasonable piece to connect to?  It looks like the piece I want
is blk_provision_space().

Yes, something like that.

quoted

But I'm not sure what "it" (the "allocate blocks" variant) even is
given falloc.h doesn't show anything like "_ALLOCATE_BLOCKS"...

The default behavior of fallocate is to allocate blocks, which means
that one invokes it by not passing any mode flags (except possibly
KEEP_SIZE).

OK.

quoted

It would require a new block interface to pass the fallocate extent
down.  But it seems bizarre to implement "some of" fallocate but not
the most widely used case for fallocate.

Agreed.  I'd like to get the existing functionality wired up sooner than
later, and plumbing "allocate blocks" down to thinp can be done as a
followup.

(Or stall long enough that it becomes one patchset.)

Sure, sounds good.  Glad we're in agreement.

On Mon, Mar 21, 2016 at 03:22:29PM -0400, Mike Snitzer wrote:

On Mon, Mar 21 2016 at  3:11pm -0400,
Darrick J. Wong [off-list ref] wrote:

quoted

On Mon, Mar 21, 2016 at 02:52:00PM -0400, Mike Snitzer wrote:

quoted

On Tue, Mar 15, 2016 at 3:42 PM, Darrick J. Wong
[off-list ref] wrote:

quoted

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.  Punch still requires that
FALLOC_FL_KEEP_SIZE is set.  A length that goes past the end of the
device will be clamped to the device size if KEEP_SIZE is set; or will
return -EINVAL if not.  Both start and length must be aligned to the
device's logical block size.

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.

I'd like to see fallocate (block allocation) extend down to DM thinp.
This more traditional use of fallocate would be useful for ensuring
ENOSPC won't occur -- especially important if the FS has committed
space in response to fallocate.  As of now fallocate doesn't inform DM
thinp at all.  Curious why you decided not to wire it up?

I don't know what to wire it up to. :)

Fair enough.  Yes something needs to be invented.
 

quoted

I didn't find any blkdev_* function that looked encouraging, though I
haven't dug too deeply into bfoster's "prototype a block reservation
allocation model" patchset yet.  At a high level I'd guess that would
be a reasonable piece to connect to?  It looks like the piece I want
is blk_provision_space().

Yes, something like that.

Just a note that the caveat/hack with the provision call in there is
that it returns an allocated block count. That was necessary to help
maintain the local reservation accounting. I'd love to find a way to
handle that more cleanly or take advantage of generic fallocate, but I
don't have a clear idea on how to do that at the moment. (I do wonder
whether an internal-only set of falloc "reserve" flags would fly...).

Anyways, that's a separate topic. Feel free to steal any of that dm-thin
provision code if it is useful for generic fallocate(). :)

Brian

quoted

quoted

But I'm not sure what "it" (the "allocate blocks" variant) even is
given falloc.h doesn't show anything like "_ALLOCATE_BLOCKS"...

The default behavior of fallocate is to allocate blocks, which means
that one invokes it by not passing any mode flags (except possibly
KEEP_SIZE).

OK.

quoted

quoted

It would require a new block interface to pass the fallocate extent
down.  But it seems bizarre to implement "some of" fallocate but not
the most widely used case for fallocate.

Agreed.  I'd like to get the existing functionality wired up sooner than
later, and plumbing "allocate blocks" down to thinp can be done as a
followup.

(Or stall long enough that it becomes one patchset.)

Sure, sounds good.  Glad we're in agreement.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Mar 21, 2016 at 02:52:00PM -0400, Mike Snitzer wrote:

On Tue, Mar 15, 2016 at 3:42 PM, Darrick J. Wong
[off-list ref] wrote:

quoted

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.  Punch still requires that
FALLOC_FL_KEEP_SIZE is set.  A length that goes past the end of the
device will be clamped to the device size if KEEP_SIZE is set; or will
return -EINVAL if not.  Both start and length must be aligned to the
device's logical block size.

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.

I'd like to see fallocate (block allocation) extend down to DM thinp.
This more traditional use of fallocate would be useful for ensuring
ENOSPC won't occur -- especially important if the FS has committed
space in response to fallocate.  As of now fallocate doesn't inform DM
thinp at all.  Curious why you decided not to wire it up?

I don't know what to wire it up to. :)

I didn't find any blkdev_* function that looked encouraging, though I
haven't dug too deeply into bfoster's "prototype a block reservation
allocation model" patchset yet.  At a high level I'd guess that would
be a reasonable piece to connect to?  It looks like the piece I want
is blk_provision_space().

But I'm not sure what "it" (the "allocate blocks" variant) even is
given falloc.h doesn't show anything like "_ALLOCATE_BLOCKS"...

The default behavior of fallocate is to allocate blocks, which means
that one invokes it by not passing any mode flags (except possibly
KEEP_SIZE).

It would require a new block interface to pass the fallocate extent
down.  But it seems bizarre to implement "some of" fallocate but not
the most widely used case for fallocate.

Agreed.  I'd like to get the existing functionality wired up sooner than
later, and plumbing "allocate blocks" down to thinp can be done as a
followup.

(Or stall long enough that it becomes one patchset.)

--D

Mike