implement async block discards/etc. via io_uring

In preparation to further changes extract a helper function out of blk_ioctl_discard() that validates if it's allowed to do a write-like operation for the given range.

Add yet another io_uring cmd implementing async secure erases. It has same page cache races as async discards and write zeroes and reuses the common paths in general.

kiocb_invalidate_pages() is useful for the write path, however not everything is backed by kiocb and we want to reuse the function for bio based discard implementation. Extract and and reuse a new helper called filemap_invalidate_pages(), which takes a argument indicating whether it should be non-blocking and might return -EAGAIN.

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/linux/pagemap.h |  2 ++
 mm/filemap.c            | 18 +++++++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

include/linux/pagemap.h +2 -0

index d9c7edb6422b..e39c3a7ce33c 100644
@@ -32,6 +32,8 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 		pgoff_t start, pgoff_t end);
 int kiocb_invalidate_pages(struct kiocb *iocb, size_t count);
 void kiocb_invalidate_post_direct_write(struct kiocb *iocb, size_t count);
+int filemap_invalidate_pages(struct address_space *mapping,
+			     loff_t pos, loff_t end, bool nowait);
 
 int write_inode_now(struct inode *, int sync);
 int filemap_fdatawrite(struct address_space *);

mm/filemap.c +3 -5

index d62150418b91..74baec119239 100644
@@ -2712,14 +2712,12 @@ int kiocb_write_and_wait(struct kiocb *iocb, size_t count)
 }
 EXPORT_SYMBOL_GPL(kiocb_write_and_wait);
 
-int kiocb_invalidate_pages(struct kiocb *iocb, size_t count)
+int filemap_invalidate_pages(struct address_space *mapping,
+			     loff_t pos, loff_t end, bool nowait)
 {
-	struct address_space *mapping = iocb->ki_filp->f_mapping;
-	loff_t pos = iocb->ki_pos;
-	loff_t end = pos + count - 1;
 	int ret;
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
+	if (nowait) {
 		/* we could block if there are any pages in the range */
 		if (filemap_range_has_page(mapping, pos, end))
 			return -EAGAIN;

@@ -2738,6 +2736,16 @@ int kiocb_invalidate_pages(struct kiocb *iocb, size_t count)
 	return invalidate_inode_pages2_range(mapping, pos >> PAGE_SHIFT,

io_uring allows to implement custom file specific operations via fops->uring_cmd callback. Use it to wire up asynchronous discard commands. Normally, first it tries to do a non-blocking issue, and if fails we'd retry from a blocking context by returning -EAGAIN to core io_uring.

Note, unlike ioctl(BLKDISCARD) with stronger guarantees against races, we only do a best effort attempt to invalidate page cache, and it can race with any writes and reads and leave page cache stale. It's the same kind of races we allow to direct writes.

+static void bio_cmd_end(struct bio *bio)

Add another io_uring cmd for block layer implementing asynchronous write zeroes. It reuses helpers we've added for async discards, and inherits the code structure as well as all considerations in regards to page cache races.

Quoted 3 lines · Pavel Begunkov

In preparation to further changes extract a helper function out of blk_ioctl_discard() that validates if it's allowed to do a write-like operation for the given range.

This isn't about a write, it is about a discard.

Quoted 3 lines · Pavel Begunkov

Add yet another io_uring cmd implementing async secure erases. It has same page cache races as async discards and write zeroes and reuses the common paths in general.

How did you test this? Asking because my interruption fix for secure

discard is still pending because I could not find a test environment for it. And also because for a "secure" [1] erase proper invalidation of the page cache actually matters, as otherwise you can still leak data to the device.

[2] LBA based "secure" erase can't actually be secure on any storage device that can write out of place. Which is all of flash, but also modern HDDs when encountering error. It's a really bad interface we should never have have started to support.

Quoted 6 lines · Pavel Begunkov

io_uring allows to implement custom file specific operations via fops->uring_cmd callback. Use it to wire up asynchronous discard commands. Normally, first it tries to do a non-blocking issue, and if fails we'd retry from a blocking context by returning -EAGAIN to core io_uring.

Quoted 4 lines

Note, unlike ioctl(BLKDISCARD) with stronger guarantees against races, we only do a best effort attempt to invalidate page cache, and it can race with any writes and reads and leave page cache stale. It's the same kind of races we allow to direct writes.

Can you please write up a man page for this that clear documents the expecvted semantics?

Quoted 1 line

+static void bio_cmd_end(struct bio *bio)

This is really weird function name. blk_cmd_end_io or blk_cmd_bio_end_io would be the usual choices.

block/ioctl.c long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg) · line 800

Quoted 2 lines

block/ioctl.c +2 -0

+	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects,
+					    GFP_KERNEL))) {

GFP_KERNEL can often will block. You'll probably want a GFP_NOWAIT allocation here for the nowait case.

Quoted 7 lines

block/ioctl.c +7 -0

+		if (nowait) {
+			/*
+			 * Don't allow multi-bio non-blocking submissions as
+			 * subsequent bios may fail but we won't get direct
+			 * feedback about that. Normally, the caller should
+			 * retry from a blocking context.
+			 */

block/ioctl.c long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg) · line 813

Quoted 6 lines

block/ioctl.c +6 -0

+			if (unlikely(nr_sects)) {
+				bio_put(bio);
+				return -EAGAIN;
+			}
+			bio->bi_opf |= REQ_NOWAIT;
+		}

And this really looks weird. It first allocates a bio, potentially

blocking, and then gives up? I think you're much better off with something like:

	if (nowait) {
		if (nr_sects > bio_discard_limit(bdev, sector))
			return -EAGAIN;
		bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects,
						    GFP_NOWAIT);
		if (!bio)
			return -EAGAIN
		bio->bi_opf |= REQ_NOWAIT;
		goto submit;
	}

	/* submission loop here */

Quoted 8 lines

include/uapi/linux/fs.h +0 -0

index 753971770733..0016e38ed33c 100644
@@ -208,6 +208,8 @@ struct fsxattr {
   * (see uapi/linux/blkzoned.h)
   */
 

include/uapi/linux/fs.h struct fsxattr { · line 211

Quoted 1 line

include/uapi/linux/fs.h +1 -0

+#define BLOCK_URING_CMD_DISCARD			0

Is fs.h the reight place for this?

Curious: how to we deal with conflicting uring cmds on different device and how do we probe for them? The NVMe uring_cmds use the ioctl-style _IO* encoding which at least helps a bit with that and which seem like a good idea. Maybe someone needs to write up a few lose rules on uring commands?

Quoted 4 lines · Pavel Begunkov

Add another io_uring cmd for block layer implementing asynchronous write zeroes. It reuses helpers we've added for async discards, and inherits the code structure as well as all considerations in regards to page cache races.

Most comments from discard apply here as well.

block/ioctl.c static void bio_cmd_end(struct bio *bio) · line 781

Quoted 3 lines

block/ioctl.c +3 -0

+static int blkdev_queue_cmd(struct io_uring_cmd *cmd, struct block_device *bdev,
+			    uint64_t start, uint64_t len, sector_t limit,
+			    blk_opf_t opf)

This feels a little over generic as it doesn't just queue any random command.

block/ioctl.c static void bio_cmd_end(struct bio *bio) · line 841

Quoted 10 lines

block/ioctl.c +10 -0

+static int blkdev_cmd_write_zeroes(struct io_uring_cmd *cmd,
+				   struct block_device *bdev,
+				   uint64_t start, uint64_t len, bool nowait)
+{
+	blk_opf_t opf = REQ_OP_WRITE_ZEROES | REQ_NOUNMAP;
+
+	if (nowait)
+		opf |= REQ_NOWAIT;
+	return blkdev_queue_cmd(cmd, bdev, start, len,
+				bdev_write_zeroes_sectors(bdev), opf);

So no support for fallback to Write of zero page here? That's probably the case where the async offload is needed most.

It's used for other commands in the series, all of them are semantically "writes", or modifying data operations if that's better. How would you call it? Some blk_modify_validate_args, maybe?

Quoted 7 lines

Quoted 4 lines

Note, unlike ioctl(BLKDISCARD) with stronger guarantees against races, we only do a best effort attempt to invalidate page cache, and it can race with any writes and reads and leave page cache stale. It's the same kind of races we allow to direct writes.

Can you please write up a man page for this that clear documents the expecvted semantics?

Do we have it documented anywhere how O_DIRECT writes interact with page cache, so I can refer to it?

Quoted 2 lines

GFP_KERNEL can often will block. You'll probably want a GFP_NOWAIT allocation here for the nowait case.

I can change it for clarity, but I don't think it's much of a concern since the read/write path and pretty sure a bunch of other places never cared about it. It does the main thing, propagating it down e.g. for tag allocation.

I'd rather avoid calling bio_discard_limit() an extra time, it does too much stuff inside, when the expected case is a single bio and for multi-bio that overhead would really matter.

Maybe I should uniline blk_alloc_discard_bio() and dedup it with

Quoted 3 lines

Quoted 1 line

+#define BLOCK_URING_CMD_DISCARD 0

Is fs.h the reight place for this?

Arguable, but I can move it to io_uring, makes things simpler for me.

Quoted 6 lines

Curious: how to we deal with conflicting uring cmds on different device and how do we probe for them? The NVMe uring_cmds use the ioctl-style _IO* encoding which at least helps a bit with that and which seem like a good idea. Maybe someone needs to write up a few lose rules on uring commands?

My concern is that we're sacrificing compiler optimisations (well, jump tables are disabled IIRC) for something that doesn't even guarantee uniqueness. I'd like to see some degree of reflection, like user querying a file class in terms of what operations it supports, but that's beyond the scope of the series.

Quoted 10 lines · Pavel Begunkov

Quoted 7 lines

Quoted 4 lines

Note, unlike ioctl(BLKDISCARD) with stronger guarantees against races, we only do a best effort attempt to invalidate page cache, and it can race with any writes and reads and leave page cache stale. It's the same kind of races we allow to direct writes.

Can you please write up a man page for this that clear documents the expecvted semantics?

Do we have it documented anywhere how O_DIRECT writes interact with page cache, so I can refer to it?

I can't find a good writeup. Adding Dave as he tends to do long emails on topic like this so he might have one hiding somewhere.

Quoted 3 lines

Quoted 2 lines

GFP_KERNEL can often will block. You'll probably want a GFP_NOWAIT allocation here for the nowait case.

Quoted 4 lines

I can change it for clarity, but I don't think it's much of a concern since the read/write path and pretty sure a bunch of other places never cared about it. It does the main thing, propagating it down e.g. for tag allocation.

True, we're only doing the nowait allocation for larger data structures. Which is a bit odd indeed.

Quoted 3 lines

I'd rather avoid calling bio_discard_limit() an extra time, it does too much stuff inside, when the expected case is a single bio and for multi-bio that overhead would really matter.

Compared to a memory allocation it's not really doing all the much. In the long run we really should move splitting discard bios down the stack like we do for normal I/O anyway.

Quoted 1 line

Maybe I should uniline blk_alloc_discard_bio() and dedup it with

uniline? I read that as unіnline, but as it's not inline I don't understand what you mean either.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

+#define BLOCK_URING_CMD_DISCARD 0

Is fs.h the reight place for this?

Arguable, but I can move it to io_uring, makes things simpler for me.

I would have expected a uapi/linux/blkdev.h for it (and I'm kinda surprised we don't have that yet).

Quoted 11 lines

Quoted 5 lines

Curious: how to we deal with conflicting uring cmds on different device and how do we probe for them? The NVMe uring_cmds use the ioctl-style _IO* encoding which at least helps a bit with that and which seem like a good idea. Maybe someone needs to write up a few lose rules on uring commands?

My concern is that we're sacrificing compiler optimisations (well, jump tables are disabled IIRC) for something that doesn't even guarantee uniqueness. I'd like to see some degree of reflection, like user querying a file class in terms of what operations it supports, but that's beyond the scope of the series.

We can't guaranteed uniqueness, but between the class, the direction, and the argument size we get a pretty good one. There is a reason pretty much all ioctls added in the last 25 years are using this scheme.