| Message ID | 7fc0a61ae29190a42e958eddfefd6d44cdf372ad.1725621577.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | implement async block discards and other ops via io_uring | expand |
> + 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; > + 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. > + 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, §or, &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. > +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/. > + > #endif /* __LINUX_BIO_H */ > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h > index 753971770733..7ea41ca97158 100644 > --- a/include/uapi/linux/fs.h > +++ b/include/uapi/linux/fs.h > @@ -208,6 +208,8 @@ struct fsxattr { > * (see uapi/linux/blkzoned.h) > */ > > +#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?
On 9/10/24 09:01, Christoph Hellwig wrote: >> + 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; >> + 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. Start and len here are in bytes. What do you mean? >> + 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, §or, &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. If the driver randomly changes it, it's racy either way. What do you want to protect against? >> +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. >> + >> #endif /* __LINUX_BIO_H */ >> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h >> index 753971770733..7ea41ca97158 100644 >> --- a/include/uapi/linux/fs.h >> +++ b/include/uapi/linux/fs.h >> @@ -208,6 +208,8 @@ struct fsxattr { >> * (see uapi/linux/blkzoned.h) >> */ >> >> +#define BLOCK_URING_CMD_DISCARD _IO(0x12,137) > > Whitespace after the comma please. That appears to be the "code style" of all BLK ioctls. > 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? > 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. Regardless, how do you see it? The new file can have just those several new definitions, in fs.h we'd have another comment why there is another empty range, but I don't think it's worth it at all. Another option is to move there everything block related, and make fs.h include blkdev.h, which can always be done on top.
On Tue, Sep 10, 2024 at 11:58:23AM +0100, Pavel Begunkov wrote: > > 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? You are right, sorry. > > > + > > > + err = filemap_invalidate_pages(bdev->bd_mapping, start, > > > + start + len - 1, nowait); > > > + if (err) > > > + return err; > > > + > > > + while ((bio = blk_alloc_discard_bio(bdev, §or, &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. > > 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. > > > +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. > > > 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? 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. > > 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.
On 9/10/24 15:17, Christoph Hellwig wrote: > On Tue, Sep 10, 2024 at 11:58:23AM +0100, Pavel Begunkov wrote: >>>> + >>>> + err = filemap_invalidate_pages(bdev->bd_mapping, start, >>>> + start + len - 1, nowait); >>>> + if (err) >>>> + return err; >>>> + >>>> + while ((bio = blk_alloc_discard_bio(bdev, §or, &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. >> >> 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 If it's shrinking then bios initialised and submitted with that initial larger limit should fail, e.g. by the disk or driver, which would be caught by bio_cmd_bio_end_io(). If nobody fails bios, then nothing ever will help here, you can always first queue up bios and change the limit afterwards while they're still in flight. > the nowait case should simply break out of the loop after the first bio. while ((bio = blk_alloc_discard_bio(bdev, §or, &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. >>>> +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. > >> >>> 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? > > 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. As far as I see nvme cmds are just dropped onto the 0x80- range. Not continuing ioctls, right, but nevertheless random and undocumented. And if we're arguing that IOC helps preventing people issuing ioctls to a wrong file type, we can easily extend it to "what if someone passes BLK* ioctl number to io_uring or vise versa? Not to mention that most of the IOC selling points have zero sense for io_uring like struct size and struct copy direction. >>> 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? 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. > 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. We can, by including blkdev.h into fs.h, but that's a different kind of a structure. > It is about adding the new command to the proper place.
On Tue, Sep 10, 2024 at 09:22:37PM +0100, Pavel Begunkov wrote: > > while ((bio = blk_alloc_discard_bio(bdev, §or, &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. You don't need the bio_chain_and_submit as bio is guaranteed to be NULL here. > > 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. blk ioctls have historically been in fs.h, and keeping it that way instead of moving some in the same range makes perfect sense. Adding new commands to this mess absolutely does not.
diff --git a/block/blk-lib.c b/block/blk-lib.c index 83eb7761c2bf..c94c67a75f7e 100644 --- a/block/blk-lib.c +++ b/block/blk-lib.c @@ -10,7 +10,8 @@ #include "blk.h" -static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector) +/* The maximum size of a discard that can be issued from a given sector. */ +sector_t bio_discard_limit(struct block_device *bdev, sector_t sector) { unsigned int discard_granularity = bdev_discard_granularity(bdev); sector_t granularity_aligned_sector; diff --git a/block/blk.h b/block/blk.h index 32f4e9f630a3..1a1a18d118f7 100644 --- a/block/blk.h +++ b/block/blk.h @@ -605,6 +605,7 @@ blk_mode_t file_to_blk_mode(struct file *file); int truncate_bdev_range(struct block_device *bdev, blk_mode_t mode, loff_t lstart, loff_t lend); long blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg); +int blkdev_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg); extern const struct address_space_operations def_blk_aops; diff --git a/block/fops.c b/block/fops.c index 9825c1713a49..8154b10b5abf 100644 --- a/block/fops.c +++ b/block/fops.c @@ -17,6 +17,7 @@ #include <linux/fs.h> #include <linux/iomap.h> #include <linux/module.h> +#include <linux/io_uring/cmd.h> #include "blk.h" static inline struct inode *bdev_file_inode(struct file *file) @@ -873,6 +874,7 @@ const struct file_operations def_blk_fops = { .splice_read = filemap_splice_read, .splice_write = iter_file_splice_write, .fallocate = blkdev_fallocate, + .uring_cmd = blkdev_uring_cmd, .fop_flags = FOP_BUFFER_RASYNC, }; diff --git a/block/ioctl.c b/block/ioctl.c index a820f692dd1c..19fba8332eee 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -11,6 +11,8 @@ #include <linux/blktrace_api.h> #include <linux/pr.h> #include <linux/uaccess.h> +#include <linux/pagemap.h> +#include <linux/io_uring/cmd.h> #include "blk.h" static int blkpg_do_ioctl(struct block_device *bdev, @@ -742,3 +744,103 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg) return ret; } #endif + +struct blk_iou_cmd { + int res; + bool nowait; +}; + +static void blk_cmd_complete(struct io_uring_cmd *cmd, unsigned int issue_flags) +{ + struct blk_iou_cmd *bic = io_uring_cmd_to_pdu(cmd, struct blk_iou_cmd); + + if (bic->res == -EAGAIN && bic->nowait) + io_uring_cmd_issue_blocking(cmd); + else + io_uring_cmd_done(cmd, bic->res, 0, issue_flags); +} + +static void bio_cmd_bio_end_io(struct bio *bio) +{ + struct io_uring_cmd *cmd = bio->bi_private; + struct blk_iou_cmd *bic = io_uring_cmd_to_pdu(cmd, struct blk_iou_cmd); + + if (unlikely(bio->bi_status) && !bic->res) + bic->res = blk_status_to_errno(bio->bi_status); + + io_uring_cmd_do_in_task_lazy(cmd, blk_cmd_complete); + bio_put(bio); +} + +static int blkdev_cmd_discard(struct io_uring_cmd *cmd, + struct block_device *bdev, + uint64_t start, uint64_t len, bool nowait) +{ + gfp_t gfp = nowait ? GFP_NOWAIT : GFP_KERNEL; + 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; + if (bdev_read_only(bdev)) + return -EPERM; + err = blk_validate_byte_range(bdev, start, len); + if (err) + return err; + + /* + * Don't allow multi-bio non-blocking submissions as subsequent bios + * may fail but we won't get a direct indication of that. Normally, + * the caller should retry from a blocking context. + */ + 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, §or, &nr_sects, gfp))) { + if (nowait) + bio->bi_opf |= REQ_NOWAIT; + prev = bio_chain_and_submit(prev, bio); + } + if (!prev) + return -EAGAIN; + + prev->bi_private = cmd; + prev->bi_end_io = bio_cmd_bio_end_io; + submit_bio(prev); + return -EIOCBQUEUED; +} + +int blkdev_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) +{ + struct block_device *bdev = I_BDEV(cmd->file->f_mapping->host); + struct blk_iou_cmd *bic = io_uring_cmd_to_pdu(cmd, struct blk_iou_cmd); + const struct io_uring_sqe *sqe = cmd->sqe; + u32 cmd_op = cmd->cmd_op; + uint64_t start, len; + + if (unlikely(sqe->ioprio || sqe->__pad1 || sqe->len || + sqe->rw_flags || sqe->file_index)) + return -EINVAL; + + bic->res = 0; + bic->nowait = issue_flags & IO_URING_F_NONBLOCK; + + start = READ_ONCE(sqe->addr); + len = READ_ONCE(sqe->addr3); + + switch (cmd_op) { + case BLOCK_URING_CMD_DISCARD: + return blkdev_cmd_discard(cmd, bdev, start, len, bic->nowait); + } + return -EINVAL; +} diff --git a/include/linux/bio.h b/include/linux/bio.h index faceadb040f9..78ead424484c 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -684,4 +684,6 @@ struct bio *bio_chain_and_submit(struct bio *prev, struct bio *new); struct bio *blk_alloc_discard_bio(struct block_device *bdev, sector_t *sector, sector_t *nr_sects, gfp_t gfp_mask); +sector_t bio_discard_limit(struct block_device *bdev, sector_t sector); + #endif /* __LINUX_BIO_H */ diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index 753971770733..7ea41ca97158 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -208,6 +208,8 @@ struct fsxattr { * (see uapi/linux/blkzoned.h) */ +#define BLOCK_URING_CMD_DISCARD _IO(0x12,137) + #define BMAP_IOCTL 1 /* obsolete - kept for compatibility */ #define FIBMAP _IO(0x00,1) /* bmap access */ #define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */
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. Suggested-by: Conrad Meyer <conradmeyer@meta.com> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- block/blk-lib.c | 3 +- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 102 ++++++++++++++++++++++++++++++++++++++++ include/linux/bio.h | 2 + include/uapi/linux/fs.h | 2 + 6 files changed, 111 insertions(+), 1 deletion(-)