implement async block discards and other ops via io_uring

block/ioctl.c +5 -0

+	sector_t sector = start >> SECTOR_SHIFT;
+	sector_t nr_sects = len >> SECTOR_SHIFT;
+	struct bio *prev = NULL, *bio;
+	int err;
+

block/ioctl.c +5 -0

+	if (nowait && nr_sects > bio_discard_limit(bdev, sector))
+		return -EAGAIN;
+
+	err = filemap_invalidate_pages(bdev->bd_mapping, start,
+					start + len - 1, nowait);

+sector_t bio_discard_limit(struct block_device *bdev, sector_t sector);

Quoted 10 lines

block/ioctl.c +10 -0

+	sector_t sector = start >> SECTOR_SHIFT;
+	sector_t nr_sects = len >> SECTOR_SHIFT;
+	struct bio *prev = NULL, *bio;
+	int err;
+
+	if (!bdev_max_discard_sectors(bdev))
+		return -EOPNOTSUPP;
+
+	if (!(file_to_blk_mode(cmd->file) & BLK_OPEN_WRITE))
+		return -EBADF;

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

Quoted 5 lines

block/ioctl.c +5 -0

+	if (bdev_read_only(bdev))
+		return -EPERM;
+	err = blk_validate_byte_range(bdev, start, len);
+	if (err)
+		return err;

Based on the above this function is misnamed, as it validates sector_t range and not a byte range.

Quoted 10 lines

block/ioctl.c +10 -0

+	if (nowait && nr_sects > bio_discard_limit(bdev, sector))
+		return -EAGAIN;
+
+	err = filemap_invalidate_pages(bdev->bd_mapping, start,
+					start + len - 1, nowait);
+	if (err)
+		return err;
+
+	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects, gfp))) {
+		if (nowait)

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

Quoted 5 lines

block/ioctl.c +5 -0

+			bio->bi_opf |= REQ_NOWAIT;
+		prev = bio_chain_and_submit(prev, bio);
+	}
+	if (!prev)
+		return -EAGAIN;

If a user changes the max_discard value between the check above and the loop here this is racy.

Quoted 1 line

+sector_t bio_discard_limit(struct block_device *bdev, sector_t sector);

And to be honest, I'd really prefer to not have bio_discard_limit exposed. Certainly not outside a header private to block/.

Quoted 10 lines

include/linux/bio.h +1 -0

+
  #endif /* __LINUX_BIO_H */

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

index 753971770733..7ea41ca97158 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			_IO(0x12,137)

Whitespace after the comma please.

Also why start at 137? A comment
would generally be pretty useful as well.

Also can we have a include/uapi/linux/blkdev.h for this instead of bloating fs.h that gets included just about everywhere?

Quoted 2 lines

Based on the above this function is misnamed, as it validates sector_t range and not a byte range.

Start and len here are in bytes. What do you mean?

Quoted 8 lines

Quoted 8 lines

block/ioctl.c +8 -0

+
+	err = filemap_invalidate_pages(bdev->bd_mapping, start,
+					start + len - 1, nowait);
+	if (err)
+		return err;
+
+	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects, gfp))) {
+		if (nowait)

Quoted 8 lines

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

Quoted 5 lines

block/ioctl.c +5 -0

+			bio->bi_opf |= REQ_NOWAIT;
+		prev = bio_chain_and_submit(prev, bio);
+	}
+	if (!prev)
+		return -EAGAIN;

If a user changes the max_discard value between the check above and the loop here this is racy.

If the driver randomly changes it, it's racy either way. What do you want to protect against?

Quoted 4 lines

Quoted 1 line

+sector_t bio_discard_limit(struct block_device *bdev, sector_t sector);

And to be honest, I'd really prefer to not have bio_discard_limit exposed. Certainly not outside a header private to block/.

Which is the other reason why first versions were putting down a bio seeing that there is more to be done for nowait, which you didn't like. I can return back to it or narrow the scopre.

Quoted 3 lines

Also why start at 137? A comment
would generally be pretty useful as well.

There is a comment, 2 lines above the new define.

/*
* A jump here: 130-136 are reserved for zoned block devices
* (see uapi/linux/blkzoned.h)
*/

Is that your concern?

Quoted 2 lines

Also can we have a include/uapi/linux/blkdev.h for this instead of bloating fs.h that gets included just about everywhere?

I don't think it belongs to this series.

Quoted 17 lines · Pavel Begunkov

Quoted 16 lines

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

Quoted 13 lines

block/ioctl.c +13 -0

+
+	err = filemap_invalidate_pages(bdev->bd_mapping, start,
+					start + len - 1, nowait);
+	if (err)
+		return err;
+
+	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects, gfp))) {
+		if (nowait)
+			bio->bi_opf |= REQ_NOWAIT;
+		prev = bio_chain_and_submit(prev, bio);
+	}
+	if (!prev)
+		return -EAGAIN;

If a user changes the max_discard value between the check above and the loop here this is racy.

Quoted 2 lines

If the driver randomly changes it, it's racy either way. What do you want to protect against?

The discard limit shrinking and now this successfully returning while not actually discarding the range. The fix is pretty simple in that

the nowait case should simply break out of the loop after the first bio.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

+sector_t bio_discard_limit(struct block_device *bdev, sector_t sector);

And to be honest, I'd really prefer to not have bio_discard_limit exposed. Certainly not outside a header private to block/.

Which is the other reason why first versions were putting down a bio seeing that there is more to be done for nowait, which you didn't like. I can return back to it or narrow the scopre.

The above should also take care of that.

Quoted 11 lines

Quoted 3 lines

Also why start at 137? A comment
would generally be pretty useful as well.

There is a comment, 2 lines above the new define.

/*
* A jump here: 130-136 are reserved for zoned block devices
* (see uapi/linux/blkzoned.h)
*/

Quoted 1 line

Is that your concern?

But those are ioctls, this is not an ioctl and uses a different number space. Take a look at e.g. nvme uring cmds which also don't try to use the same number space as the ioctl.

Quoted 3 lines

Quoted 2 lines

Also can we have a include/uapi/linux/blkdev.h for this instead of bloating fs.h that gets included just about everywhere?

I don't think it belongs to this series.

How would adding a proper header instead of bloating fs.h not be part of the series adding the first ever block layer uring_cmds?

Just in case I wasn't clear - this isn't asking you to move anything existing as we can't do that without breaking existing applications.

It is about adding the new command to the proper place.

while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects, gfp))) {
	if (nowait)
		bio->bi_opf |= REQ_NOWAIT;
	prev = bio_chain_and_submit(prev, bio);
	if (nowait)
		break;
}

Like this? I need to add nr_sects==0 post loop checking either way, but I don't see how this break would be better any better than bio_put before the submit from v2.

Quoted 2 lines

How would adding a proper header instead of bloating fs.h not be part of the series adding the first ever block layer uring_cmds?

Because, apparently, no one have ever gave a damn about it. I'll add it for you, but with header probing instead of a simple ifdef I'd call it a usability downgrade.

Add a command that writes the zero page to the drive. Apart from passing the zero page instead of actual data it uses the normal write path and doesn't do any further acceleration, nor it requires any special hardware support. The indended use is to have a fallback when BLOCK_URING_CMD_WRITE_ZEROES is not supported.

Quoted 5 lines · Pavel Begunkov

Add a command that writes the zero page to the drive. Apart from passing the zero page instead of actual data it uses the normal write path and doesn't do any further acceleration, nor it requires any special hardware support. The indended use is to have a fallback when BLOCK_URING_CMD_WRITE_ZEROES is not supported.

That's just a horrible API. The user should not have to care if the kernel is using different kinds of implementations.

Quoted 6 lines

Quoted 5 lines

Add a command that writes the zero page to the drive. Apart from passing the zero page instead of actual data it uses the normal write path and doesn't do any further acceleration, nor it requires any special hardware support. The indended use is to have a fallback when BLOCK_URING_CMD_WRITE_ZEROES is not supported.

Quoted 2 lines

That's just a horrible API. The user should not have to care if the kernel is using different kinds of implementations.

It's rather not a good api when instead of issuing a presumably low overhead fast command the user expects sending a good bunch of actual writes with different performance characteristics.

In my experience,
such fallbacks cause more pain when a more explicit approach is possible. And let me note that it's already exposed via fallocate, even though in a bit different way.

Quoted 9 lines · Pavel Begunkov

Quoted 8 lines

Quoted 5 lines

Add a command that writes the zero page to the drive. Apart from passing the zero page instead of actual data it uses the normal write path and doesn't do any further acceleration, nor it requires any special hardware support. The indended use is to have a fallback when BLOCK_URING_CMD_WRITE_ZEROES is not supported.

That's just a horrible API. The user should not have to care if the kernel is using different kinds of implementations.

Quoted 3 lines

It's rather not a good api when instead of issuing a presumably low overhead fast command the user expects sending a good bunch of actual writes with different performance characteristics.

The normal use case (at least the ones I've been involved with) are simply zero these blocks or the entire device, and please do it as good as you can. Needing asynchronous error handling in userspace for that is extremely counter productive.

Quoted 4 lines

In my experience,
such fallbacks cause more pain when a more explicit approach is possible. And let me note that it's already exposed via fallocate, even though in a bit different way.

Do you mean the FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE case in blkdev_fallocate? As far as I can tell this is actually a really bad example, as even a hardware offloaded write zeroes can and often does write physical zeroes to the media, and does so from a firmware path that is often slower than the kernel loop.

But you have an actual use case where you want to send a write zeroes command but never a loop of writes, it would be good to document that and add a flag for it. And if we don't have that case it would still

be good to have a reserved flags field to add it later if needed.

Btw, do you have API documentation (e.g. in the form of a man page) for these new calls somewhere?

If we expect any error handling from the user space at all (we do), it'll and have to be asynchronous, it's async commands and io_uring. Asking the user to reissue a command in some form is normal.

That's a shame, I agree, which is why I call it "presumably" faster, but that actually gives more reasons why you might want this cmd separately from write zeroes, considering the user might know its hardware and the kernel doesn't try to choose which approach faster.

Users who know more about hw and e.g. prefer writes with 0 page as per above. Users with lots of devices who care about pcie / memory bandwidth, there is enough of those, they might want to do something different like adjusting algorithms and throttling. Better/easier testing, though of lesser importance.

Those I made up just now on the spot, but the reporter did specifically ask about some way to differentiate fallbacks.

Quoted 3 lines · Pavel Begunkov

If we expect any error handling from the user space at all (we do), it'll and have to be asynchronous, it's async commands and io_uring. Asking the user to reissue a command in some form is normal.

The point is that pretty much all other errors are fatal, while this is a not supported for which we have a guaranteed to work kernel

fallback. Kicking it off reuires a bit of work, but I'd rather have that in one place rather than applications that work on some hardware and not others.

Quoted 5 lines

That's a shame, I agree, which is why I call it "presumably" faster, but that actually gives more reasons why you might want this cmd separately from write zeroes, considering the user might know its hardware and the kernel doesn't try to choose which approach faster.

But the kernel is the right place to make that decision, even if we aren't very smart about it right now. Fanning that out to every single applications is a bad idea.

Quoted 8 lines

Users who know more about hw and e.g. prefer writes with 0 page as per above. Users with lots of devices who care about pcie / memory bandwidth, there is enough of those, they might want to do something different like adjusting algorithms and throttling. Better/easier testing, though of lesser importance.

Those I made up just now on the spot, but the reporter did specifically ask about some way to differentiate fallbacks.

Well, an optional nofallback flag would be in line with how we do that. Do you have the original report to share somewhere?

There is an interest in having asynchronous block operations like discard and write zeroes. The series implements that as io_uring commands, which is an io_uring request type allowing to implement custom file specific operations.

First 4 are preparation patches. Patch 5 introduces the main chunk of cmd infrastructure and discard commands. Patches 6-8 implement write zeroes variants.

Branch with tests and docs:
https://github.com/isilence/liburing.git discard-cmd

The man page specifically (need to shuffle it to some cmd section):
https://github.com/isilence/liburing/commit/a6fa2bc2400bf7fcb80496e322b5db4c8b3191f0

There is an interest in having asynchronous block operations like discard and write zeroes. The series implements that as io_uring commands, which is an io_uring request type allowing to implement custom file specific operations.

First 4 are preparation patches. Patch 5 introduces the main chunk of cmd infrastructure and discard commands. Patches 6-8 implement write zeroes variants.

Quoted 8 lines · Pavel Begunkov

There is an interest in having asynchronous block operations like discard and write zeroes. The series implements that as io_uring commands, which is an io_uring request type allowing to implement custom file specific operations.

First 4 are preparation patches. Patch 5 introduces the main chunk of cmd infrastructure and discard commands. Patches 6-8 implement write zeroes variants.

Sitting in for-6.12/io_uring-discard for now, as there's a hidden dependency with the end/len patch in for-6.12/block.

Ran a quick test - have 64 4k discards inflight. Here's the current performance, with 64 threads with sync discard:

qd64 sync discard: 21K IOPS, lat avg 3 msec (max 21 msec)

and using io_uring with async discard, otherwise same test case:

qd64 async discard: 76K IOPS, lat avg 845 usec (max 2.2 msec)

If we switch to doing 1M discards, then we get:

qd64 sync discard: 14K IOPS, lat avg 5 msec (max 25 msec)

and using io_uring with async discard, otherwise same test case:

qd64 async discard: 56K IOPS, lat avg 1153 usec (max 3.6 msec)

This is on a:

Samsung Electronics Co Ltd NVMe SSD Controller PM174X

nvme device. It doesn't have the fastest discard, but still nicely shows the improvement over a purely sync discard.

There is an interest in having asynchronous block operations like discard and write zeroes. The series implements that as io_uring commands, which is an io_uring request type allowing to implement custom file specific operations.

First 4 are preparation patches. Patch 5 introduces the main chunk of cmd infrastructure and discard commands. Patches 6-8 implement write zeroes variants.

[...]