implement asynchronous BLKDISCARD via io_uring
Subsystems (5)
cover implement asynchronous BLKDISCARD via io_uring
There is an interest in having asynchronous block operations like discard. The patch set implements that as io_uring commands, which is an io_uring request type allowing to implement custom file specific operations.
First 4 patches are simple preps, and the main part is in Patch 5. Not tested with a real drive yet, hence sending as an RFC.
I'm also going to add BLKDISCARDZEROES and BLKSECDISCARD, which should reuse structures and helpers from Patch 5.
liburing tests for reference:
https://github.com/isilence/liburing.git discard-cmd-test
Pavel Begunkov (5):
io_uring/cmd: expose iowq to cmds io_uring/cmd: give inline space in request to cmds
filemap: introduce filemap_invalidate_pages
block: introduce blk_validate_discard()
block: implement io_uring discard cmd
block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 139 ++++++++++++++++++++++++++++++----- include/linux/io_uring/cmd.h | 15 ++++ include/linux/pagemap.h | 2 + include/uapi/linux/fs.h | 2 + io_uring/io_uring.c | 11 +++ io_uring/io_uring.h | 1 + io_uring/uring_cmd.c | 7 ++ mm/filemap.c | 18 +++-- 10 files changed, 176 insertions(+), 22 deletions(-)
Signature · 1 line
2.45.2
RFC 1/5 io_uring/cmd: expose iowq to cmds
When an io_uring request needs blocking context we offload it to the io_uring's thread pool called io-wq. We can get there off ->uring_cmd by returning -EAGAIN, but there is no straightforward way of doing that from an asynchronous callback. Add a helper that would transfer a command to a blocking context.
Note, we do an extra hop via task_work before io_queue_iowq(), that's a limitation of io_uring infra we have that can likely be lifted later if that would ever become a problem.
--- include/linux/io_uring/cmd.h | 6 ++++++ io_uring/io_uring.c | 11 +++++++++++ io_uring/io_uring.h | 1 + io_uring/uring_cmd.c | 7 +++++++ 4 files changed, 25 insertions(+)
include/linux/io_uring/cmd.h +6 -0
@@ -48,6 +48,9 @@ void __io_uring_cmd_do_in_task(struct io_uring_cmd *ioucmd,void io_uring_cmd_mark_cancelable (struct io_uring_cmd *cmd,unsigned int issue_flags); +/* Execute the request from a blocking context */ +void io_uring_cmd_issue_blocking (struct io_uring_cmd *ioucmd); +#else static inline int io_uring_cmd_import_fixed (u64 ubuf,unsigned long len,int rw,struct iov_iter *iter,void *ioucmd)
@@ -67,6 +70,9 @@ static inline void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd, unsigned int issue_flags) { } +static inline void io_uring_cmd_issue_blocking (struct io_uring_cmd *ioucmd) +{ +}#endif /*
io_uring/io_uring.c +11 -0
@@ -533,6 +533,17 @@ static void io_queue_iowq(struct io_kiocb *req)io_queue_linked_timeout (link); } +static void io_req_queue_iowq_tw (struct io_kiocb *req,struct io_tw_state *ts) +{ +io_queue_iowq (req); +} + +void io_req_queue_iowq (struct io_kiocb *req) +{ + req->io_task_work.func = io_req_queue_iowq_tw; +io_req_task_work_add (req); +} +static __cold void io_queue_deferred (struct io_ring_ctx *ctx) {while (!list_empty (&ctx->defer_list)) {
io_uring/io_uring.h +1 -0
@@ -90,6 +90,7 @@ int io_uring_alloc_task_context(struct task_struct *task,int io_ring_add_registered_file (struct io_uring_task *tctx,struct file *file,int start,int end); +void io_req_queue_iowq (struct io_kiocb *req);int io_poll_issue (struct io_kiocb *req,struct io_tw_state *ts);int io_submit_sqes (struct io_ring_ctx *ctx,unsigned int nr);
io_uring/uring_cmd.c +7 -0
@@ -277,6 +277,13 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, }EXPORT_SYMBOL_GPL (io_uring_cmd_import_fixed); +void io_uring_cmd_issue_blocking (struct io_uring_cmd *ioucmd) +{ +struct io_kiocb *req =cmd_to_io_kiocb (ioucmd); + +io_req_queue_iowq (req); +} +static inline int io_uring_cmd_getsockopt (struct socket *sock,struct io_uring_cmd *cmd,unsigned int issue_flags)
Signature · 1 line
2.45.2
RFC 2/5 io_uring/cmd: give inline space in request to cmds
In reply to Pavel BegunkovSome io_uring commands can use some inline space in io_kiocb. We have 32 bytes in struct io_uring_cmd, expose it.
--- include/linux/io_uring/cmd.h | 9 +++++++++ 1 file changed, 9 insertions(+)
include/linux/io_uring/cmd.h +9 -0
@@ -23,6 +23,15 @@ static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe)return sqe->cmd; } +static inline void io_uring_cmd_private_sz_check (size_t cmd_sz) +{ + BUILD_BUG_ON(cmd_sz > sizeof_field(struct io_uring_cmd, pdu)); +} +#define io_uring_cmd_to_pdu (cmd, pdu_type) ( \ + io_uring_cmd_private_sz_check(sizeof(pdu_type)), \ + ((pdu_type *)&(cmd)->pdu) \ +) +#if defined(CONFIG_IO_URING )int io_uring_cmd_import_fixed (u64 ubuf,unsigned long len,int rw,struct iov_iter *iter,void *ioucmd);
Signature · 1 line
2.45.2
RFC 3/5 filemap: introduce filemap_invalidate_pages
In reply to Pavel Begunkovkiocb_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.
--- include/linux/pagemap.h | 2 ++ mm/filemap.c | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)
include/linux/pagemap.h +2 -0
@@ -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 +13 -5
@@ -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 , end >>PAGE_SHIFT ); } + +int kiocb_invalidate_pages (struct kiocb *iocb,size_t count) +{ +struct address_space *mapping = iocb->ki_filp->f_mapping; +loff_t pos = iocb->ki_pos; +loff_t end = pos + count -1 ; + +return filemap_invalidate_pages (mapping, pos, end, + iocb->ki_flags &IOCB_NOWAIT ); +}EXPORT_SYMBOL_GPL (kiocb_invalidate_pages);/**
Signature · 1 line
2.45.2
RFC 4/5 block: introduce blk_validate_discard()
In reply to Pavel BegunkovIn preparation to further changes extract a helper function out of blk_ioctl_discard() that validates discard arguments.
--- block/ioctl.c | 45 ++++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-)
block/ioctl.c +28 -17
@@ -92,39 +92,50 @@ static int compat_blkpg_ioctl(struct block_device *bdev, }#endif -static int blk_ioctl_discard (struct block_device *bdev,blk_mode_t mode, -unsigned long arg) +static int blk_validate_discard (struct block_device *bdev,blk_mode_t mode, +uint64_t start,uint64_t len) { -unsigned int bs_mask =bdev_logical_block_size (bdev) -1 ; -uint64_t range[2 ], start, len, end; -struct bio *prev =NULL , *bio; -sector_t sector, nr_sects; -struct blk_plug plug; -int err; +unsigned int bs_mask; +uint64_t end;if (!(mode &BLK_OPEN_WRITE ))return -EBADF ; -if (!bdev_max_discard_sectors (bdev))return -EOPNOTSUPP ;if (bdev_read_only (bdev))return -EPERM ; -if (copy_from_user (range, (void __user *)arg,sizeof (range))) -return -EFAULT ; - - start = range[0 ]; - len = range[1 ]; - -if (!len) -return -EINVAL ; + bs_mask =bdev_logical_block_size (bdev) -1 ;if ((start | len) & bs_mask)return -EINVAL ; +if (!len) +return -EINVAL ;if (check_add_overflow (start, len, &end) || end >bdev_nr_bytes (bdev))return -EINVAL ; +return 0 ; +} + +static int blk_ioctl_discard (struct block_device *bdev,blk_mode_t mode, +unsigned long arg) +{ +uint64_t range[2 ], start, len; +struct bio *prev =NULL , *bio; +sector_t sector, nr_sects; +struct blk_plug plug; +int err; + +if (copy_from_user (range, (void __user *)arg,sizeof (range))) +return -EFAULT ; + start = range[0 ]; + len = range[1 ]; + + err =blk_validate_discard (bdev, mode, start, len); +if (err) +return err; +filemap_invalidate_lock (bdev->bd_mapping); err =truncate_bdev_range (bdev, mode, start, start + len -1 );if (err)
Signature · 1 line
2.45.2
RFC 5/5 block: implement io_uring discard cmd
In reply to Pavel BegunkovAdd ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)
block/blk.h +1 -0
@@ -571,6 +571,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;
block/fops.c +2 -0
@@ -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, };
block/ioctl.c +94 -0
104 lines not on this page · raw message
include/uapi/linux/fs.h +2 -0
@@ -208,6 +208,8 @@ struct fsxattr {* (see uapi/linux/blkzoned.h) */ +#define BLOCK_URING_CMD_DISCARD 0 +#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 */
Signature · 1 line
2.45.2
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 42 lines · Pavel Begunkov
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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)
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 4 lines
block/fops.c +1
-0@@ -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,
Just be curious, we have IORING_OP_FALLOCATE already for sending discard to block device, why is .uring_cmd added for this purpose?
Thanks,
Jens Axboe
maintainerRe: [RFC 5/5] block: implement io_uring discard cmd
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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"
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?
I think wiring up a bdev uring_cmd makes sense, because:
1) The existing FALLOCATE op is using vfs_fallocate, which is inherently sync and hence always punted to io-wq.
2) There will most certainly be other async ops that would be interesting to add, at which point we'd need it anyway.
3) It arguably makes more sense to have a direct discard op than use fallocate for this, if working on a raw bdev.
And probably others...
Signature · 1 line
Jens Axboe
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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"
Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because vfs_fallocate is created synchronous IORING_OP_FALLOCATE is slow for anything but pretty large requests. Probably can be patched up, which would involve changing the fops->fallocate protot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.
Quoted 13 lines
I think wiring up a bdev
uring_cmdmakes sense, because:1) The existing FALLOCATE op is using
vfs_fallocate, which is inherently sync and hence always punted to io-wq.2) There will most certainly be other async ops that would be interesting to add, at which point we'd need it anyway.
3) It arguably makes more sense to have a direct discard op than use fallocate for this, if working on a raw bdev.
And probably others...
Signature · 1 line
Pavel Begunkov
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?
Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.
Yeah, we have ioctl(DISCARD), which acquires filemap_invalidate_lock, same with blkdev_fallocate().
But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
Thanks,
Ming
Jens Axboe
maintainerRe: [RFC 5/5] block: implement io_uring discard cmd
Quoted 63 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.
Quoted 6 lines
Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
Really not that unusual.
Signature · 1 line
Jens Axboe
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 67 lines · Jens Axboe
Quoted 66 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().
Quoted 7 lines
Quoted 3 lines
But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with IORING_OP_FALLOCATE?
thanks,
Ming
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 76 lines · Ming Lei
Quoted 67 lines · Jens Axboe
Quoted 66 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().Quoted 7 lines
Quoted 3 lines
But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
Quoted 6 lines
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?
Right, and agree that we can't trylock because we'd need to keep it locked until IO completes, at least the sync versions does that.
But I think *invalidate_pages() in the patch should be enough. That's what the write path does, so it shouldn't cause any problem to the kernel. As for user space, that'd be more relaxed than the ioctl, just as writes are, so nothing new to the user. I hope someone with better filemap understanding can confirm it (or not).
Signature · 1 line
Pavel Begunkov
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 84 lines · Pavel Begunkov
Quoted 76 lines · Ming Lei
Quoted 67 lines · Jens Axboe
Quoted 66 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().Quoted 7 lines
Quoted 3 lines
But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
Quoted 6 lines
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?
Quoted 8 lines
Right, and agree that we can't trylock because we'd need to keep it locked until IO completes, at least the sync versions does that.
But I think
*invalidate_pages()in the patch should be enough. That's what the write path does, so it shouldn't cause any problem to the kernel. As for user space, that'd be more relaxed than the ioctl, just as writes are, so nothing new to the user. I hope someone with better filemap understanding can confirm it (or not).
I may not be familiar with filemap enough, but looks *invalidate_pages() is only for removing pages from the page cache range, and the lock is added for preventing new page cache read from being started, so stale data read can be avoided when DISCARD is in-progress.
Thanks,
Ming
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 94 lines · Ming Lei
Quoted 84 lines · Pavel Begunkov
Quoted 76 lines · Ming Lei
Quoted 67 lines · Jens Axboe
Quoted 66 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().Quoted 7 lines
Quoted 3 lines
But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
Quoted 6 lines
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?Quoted 8 lines
Right, and agree that we can't trylock because we'd need to keep it locked until IO completes, at least the sync versions does that.
But I think
*invalidate_pages()in the patch should be enough. That's what the write path does, so it shouldn't cause any problem to the kernel. As for user space, that'd be more relaxed than the ioctl, just as writes are, so nothing new to the user. I hope someone with better filemap understanding can confirm it (or not).
Quoted 4 lines
I may not be familiar with filemap enough, but looks
*invalidate_pages()is only for removing pages from the page cache range, and the lock is added for preventing new page cache read from being started, so stale data read can be avoided when DISCARD is in-progress.
Sounds like it, but the point is it's the same data race for the user as if it would've had a write in progress.
Signature · 1 line
Pavel Begunkov
Jens Axboe
maintainerRe: [RFC 5/5] block: implement io_uring discard cmd
Quoted 95 lines · Pavel Begunkov
Quoted 94 lines · Ming Lei
Quoted 92 lines · Pavel Begunkov
Quoted 76 lines · Ming Lei
Quoted 67 lines · Jens Axboe
Quoted 66 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>---
block/blk.h | 1 +
block/fops.c | 2 +
block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++include/uapi/linux/fs.h| 2 +4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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"Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().Quoted 7 lines
Quoted 3 lines
But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
Quoted 6 lines
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?Right, and agree that we can't trylock because we'd need to keep it locked until IO completes, at least the sync versions does that.
But I think
*invalidate_pages()in the patch should be enough. That's what the write path does, so it shouldn't cause any problem to the kernel. As for user space, that'd be more relaxed than the ioctl, just as writes are, so nothing new to the user. I hope someone with better filemap understanding can confirm it (or not).
Quoted 7 lines
Quoted 4 lines
I may not be familiar with filemap enough, but looks
*invalidate_pages()is only for removing pages from the page cache range, and the lock is added for preventing new page cache read from being started, so stale data read can be avoided when DISCARD is in-progress.Sounds like it, but the point is it's the same data race for the user as if it would've had a write in progress.
Right, which is why it should not matter. I think it's pretty silly to take the sync implementation as gospel here, assuming that the original author knew what they were doing in full detail. It just needs proper documenting.
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerRe: [RFC 5/5] block: implement io_uring discard cmd
In reply to Ming LeiQuoted 72 lines · Ming Lei
Quoted 71 lines · Jens Axboe
Quoted 69 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
Quoted 7 lines
Quoted 3 lines
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Oh for sure, you could not do that anyway as you'd be holding a lock across the syscall boundary, which isn't allowed.
Quoted 2 lines
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?
I think the current truncate is overkill, we should be able to get by without. And no, I will not entertain an option that's "oh just punt it to io-wq".
Signature · 1 line
Jens Axboe
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 77 lines · Jens Axboe
Quoted 76 lines · Ming Lei
Quoted 74 lines · Jens Axboe
Quoted 63 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Quoted 6 lines
Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
Quoted 6 lines
Quoted 3 lines
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Oh for sure, you could not do that anyway as you'd be holding a lock across the syscall boundary, which isn't allowed.
Indeed.
Quoted 7 lines
Quoted 3 lines
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?I think the current truncate is overkill, we should be able to get by without. And no, I will not entertain an option that's "oh just punt it to io-wq".
BTW, the truncate is added by 351499a172c0 ("block: Invalidate cache on discard v2"), and block/009 serves as regression test for covering page cache coherency and discard.
Here the issue is actually related with the exclusive lock of filemap_invalidate_lock(). IMO, it is reasonable to prevent page read during discard for not polluting page cache. block/009 may fail too without the lock.
It is just that concurrent discards can't be allowed any more by down_write() of rw_semaphore, and block device is really capable of doing that. It can be thought as one regression of 7607c44c157d ("block: Hold invalidate_lock in BLKDISCARD ioctl").
Cc Jan Kara and Shin'ichiro Kawasaki.
Thanks,
Ming
Jens Axboe
maintainerRe: [RFC 5/5] block: implement io_uring discard cmd
Quoted 103 lines · Ming Lei
Quoted 77 lines · Jens Axboe
Quoted 76 lines · Ming Lei
Quoted 74 lines · Jens Axboe
Quoted 63 lines · Ming Lei
Quoted 53 lines · Pavel Begunkov
Quoted 42 lines · Jens Axboe
Quoted 41 lines · Ming Lei
Quoted 40 lines · On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <[email protected]> wrote:
Add ->uring_cmd callback for block device files and use it to implement asynchronous discard. Normally, it first tries to execute the command from non-blocking context, which we limit to a single bio because otherwise one of sub-bios may need to wait for other bios, and we don't want to deal with partial IO. If non-blocking attempt fails, we'll retry it in a blocking context.
Suggested-by Conrad Meyer <[email protected]>Signed-off-by Pavel Begunkov <[email protected]>--- block/blk.h | 1 + block/fops.c | 2 + block/ioctl.c | 94 +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 2 + 4 files changed, 99 insertions(+)block/blk.h +1
-0@@ -571,6 +571,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;block/fops.c +1
-0@@ -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" Quoted 9 lines
Quoted 9 lines
block/fops.c const struct file_operations def_blk_fops = { · line 877
Quoted 6 lines
block/fops.c +1
-0static 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,Just be curious, we have
IORING_OP_FALLOCATEalready for sending discard to block device, why is .uring_cmd added for this purpose?Quoted 8 lines
Which is a good question, I haven't thought about it, but I tend to agree with Jens. Because
vfs_fallocateis created synchronousIORING_OP_FALLOCATEis slow for anything but pretty large requests. Probably can be patched up, which would involve changing thefops->fallocateprotot, but I'm not sure async there makes sense outside of bdev (?), and cmd approach is simpler, can be made somewhat more efficient (1 less layer in the way), and it's not really something completely new since we have it in ioctl.Quoted 6 lines
Yeah, we have ioctl(DISCARD), which acquires
filemap_invalidate_lock, same withblkdev_fallocate().But this patch drops this exclusive lock, so it becomes async friendly, but may cause stale page cache. However, if the lock is required, it can't be efficient anymore and io-wq may be inevitable, :-)
If you want to grab the lock, you can still opportunistically grab it. For (by far) the common case, you'll get it, and you can still do it inline.
Quoted 6 lines
Quoted 3 lines
If the lock is grabbed in the whole cmd lifetime, it is basically one sync interface cause there is at most one async discard cmd in-flight for each device.
Oh for sure, you could not do that anyway as you'd be holding a lock across the syscall boundary, which isn't allowed.
Indeed.
Quoted 7 lines
Quoted 3 lines
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?I think the current truncate is overkill, we should be able to get by without. And no, I will not entertain an option that's "oh just punt it to io-wq".
BTW, the truncate is added by
351499a172c0("block: Invalidate cache on discard v2"), and block/009 serves as regression test for covering page cache coherency and discard.Here the issue is actually related with the exclusive lock of
filemap_invalidate_lock(). IMO, it is reasonable to prevent page read during discard for not polluting page cache. block/009 may fail too without the lock.
Quoted 6 lines
It is just that concurrent discards can't be allowed any more by
down_write()ofrw_semaphore, and block device is really capable of doing that. It can be thought as one regression of7607c44c157d("block: Holdinvalidate_lockin BLKDISCARD ioctl").Cc Jan Kara and Shin'ichiro Kawasaki.
Honestly I just think that's nonsense. It's like mixing direct and buffered writes. Can you get corruption? Yes you most certainly can. There should be no reason why we can't run discards without providing page cache coherency. The sync interface attempts to do that, but that doesn't mean that an async (or a different sync one, if that made sense) should.
If you do discards to the same range as you're doing buffered IO, you get to keep both potentially pieces. Fact is that most folks are doing dio for performant IO exactly because buffered writes tend to be horrible, and you could certainly use that with async discards and have the application manage it just fine.
So I really think any attempts to provide page cache synchronization for this is futile. And the existing sync one looks pretty abysmal, but it doesn't really matter as it's a sync interfce. If one were to do something about it for an async interface, then just pretend it's dio and increment i_dio_count.
Signature · 1 line
Jens Axboe
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 3 lines · Jens Axboe
Quoted 2 lines · Ming Lei
Quoted 1 line · Jens Axboe
On 8/15/24 7:45 PM, Ming Lei wrote:
...
Quoted 20 lines
Quoted 20 lines
Quoted 6 lines
Quoted 2 lines
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?I think the current truncate is overkill, we should be able to get by without. And no, I will not entertain an option that's "oh just punt it to io-wq".
BTW, the truncate is added by
351499a172c0("block: Invalidate cache on discard v2"), and block/009 serves as regression test for covering page cache coherency and discard.Here the issue is actually related with the exclusive lock of
filemap_invalidate_lock(). IMO, it is reasonable to prevent page read during discard for not polluting page cache. block/009 may fail too without the lock.It is just that concurrent discards can't be allowed any more by
down_write()ofrw_semaphore, and block device is really capable of doing that. It can be thought as one regression of7607c44c157d("block: Holdinvalidate_lockin BLKDISCARD ioctl").
Quoted 8 lines
Quoted 1 line
Cc Jan Kara and Shin'ichiro Kawasaki.
Honestly I just think that's nonsense. It's like mixing direct and buffered writes. Can you get corruption? Yes you most certainly can. There should be no reason why we can't run discards without providing page cache coherency. The sync interface attempts to do that, but that doesn't mean that an async (or a different sync one, if that made sense) should.
I don't see it as a problem either, it's a new interface, just need to be upfront on what guarantees it provides (one more reason why not fallocate), I'll elaborate on it in the commit message and so.
I think a reasonable thing to do is to have one rule for all write-like operations starting from plain writes, which is currently allowing races to happen and shift it to the user. Purely in theory we can get inventive with likes of range lock trees, but that's unwarranted for all sorts of reasons.
Quoted 9 lines
If you do discards to the same range as you're doing buffered IO, you get to keep both potentially pieces. Fact is that most folks are doing dio for performant IO exactly because buffered writes tend to be horrible, and you could certainly use that with async discards and have the application manage it just fine.
So I really think any attempts to provide page cache synchronization for this is futile. And the existing sync one looks pretty abysmal, but it doesn't really matter as it's a sync interfce. If one were to do
It should be a pain for sync as well, you can't even spin another process and parallelise this way.
Signature · 1 line
Pavel Begunkov
Re: [RFC 5/5] block: implement io_uring discard cmd
Quoted 34 lines · Pavel Begunkov
Quoted 3 lines · Jens Axboe
Quoted 2 lines · Ming Lei
Quoted 1 line · Jens Axboe
On 8/15/24 7:45 PM, Ming Lei wrote:
...
Quoted 20 lines
Quoted 20 lines
Quoted 6 lines
Quoted 2 lines
Meantime the handling has to move to io-wq for avoiding to block current context, the interface becomes same with
IORING_OP_FALLOCATE?I think the current truncate is overkill, we should be able to get by without. And no, I will not entertain an option that's "oh just punt it to io-wq".
BTW, the truncate is added by
351499a172c0("block: Invalidate cache on discard v2"), and block/009 serves as regression test for covering page cache coherency and discard.Here the issue is actually related with the exclusive lock of
filemap_invalidate_lock(). IMO, it is reasonable to prevent page read during discard for not polluting page cache. block/009 may fail too without the lock.It is just that concurrent discards can't be allowed any more by
down_write()ofrw_semaphore, and block device is really capable of doing that. It can be thought as one regression of7607c44c157d("block: Holdinvalidate_lockin BLKDISCARD ioctl").Quoted 8 lines
Quoted 1 line
Cc Jan Kara and Shin'ichiro Kawasaki.
Honestly I just think that's nonsense. It's like mixing direct and buffered writes. Can you get corruption? Yes you most certainly can. There should be no reason why we can't run discards without providing page cache coherency. The sync interface attempts to do that, but that doesn't mean that an async (or a different sync one, if that made sense) should.
Quoted 3 lines
I don't see it as a problem either, it's a new interface, just need to be upfront on what guarantees it provides (one more reason why not fallocate), I'll elaborate on it in the commit message and so.
Fair enough.
Quoted 13 lines
I think a reasonable thing to do is to have one rule for all write-like operations starting from plain writes, which is currently allowing races to happen and shift it to the user. Purely in theory we can get inventive with likes of range lock trees, but that's unwarranted for all sorts of reasons.
Quoted 6 lines
If you do discards to the same range as you're doing buffered IO, you get to keep both potentially pieces. Fact is that most folks are doing dio for performant IO exactly because buffered writes tend to be horrible, and you could certainly use that with async discards and have the application manage it just fine.
Quoted 6 lines
Quoted 3 lines
So I really think any attempts to provide page cache synchronization for this is futile. And the existing sync one looks pretty abysmal, but it doesn't really matter as it's a sync interfce. If one were to do
It should be a pain for sync as well, you can't even spin another process and parallelise this way.
Yes, this way has degraded some sync discard workloads perf a lot.
Thanks,
Ming
kernel test robot
build botRe: [RFC 5/5] block: implement io_uring discard cmd
In reply to Pavel Begunkovkernel test robot noticed the following build errors:
Hi Pavel,
[This is a private test report for your RFC patch.] kernel test robot noticed the following build errors:
[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on akpm-mm/mm-everything linus/master v6.11-rc3 next-20240814] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pavel-Begunkov/io_uring-cmd-expose-iowq-to-cmds/20240814-224344 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next patch link: https://lore.kernel.org/r/6ecd7ab3386f63f1656dc766c1b5b038ff5353c2.1723601134.git.asml.silence%40gmail.com patch subject: [RFC 5/5] block: implement io_uring discard cmd config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20240815/[email protected]/config) compiler: or1k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240815/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> or1k-linux-ld: block/fops.o:(.rodata+0x90): undefined reference to `blkdev_uring_cmd'
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
kernel test robot
build botRe: [RFC 5/5] block: implement io_uring discard cmd
In reply to Pavel Begunkovkernel test robot noticed the following build errors:
Hi Pavel,
[This is a private test report for your RFC patch.] kernel test robot noticed the following build errors:
[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on akpm-mm/mm-everything linus/master v6.11-rc3 next-20240814] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pavel-Begunkov/io_uring-cmd-expose-iowq-to-cmds/20240814-224344 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next patch link: https://lore.kernel.org/r/6ecd7ab3386f63f1656dc766c1b5b038ff5353c2.1723601134.git.asml.silence%40gmail.com patch subject: [RFC 5/5] block: implement io_uring discard cmd config: powerpc-allnoconfig (https://download.01.org/0day-ci/archive/20240815/[email protected]/config) compiler: powerpc-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240815/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> powerpc-linux-ld: block/fops.o:(.rodata+0x90): undefined reference to `blkdev_uring_cmd'
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Jens Axboe
maintainerRe: [RFC 5/5] block: implement io_uring discard cmd
In reply to Pavel BegunkovQuoted 98 lines · Pavel Begunkov
block/ioctl.c +83
-0@@ -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,@@ -744,4 +746,96 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)return ret; } + +struct blk_cmd { +blk_status_t status; +bool nowait; +}; + +static void blk_cmd_complete (struct io_uring_cmd *cmd,unsigned int issue_flags) +{ + struct blk_cmd *bc = io_uring_cmd_to_pdu(cmd, struct blk_cmd); +int res =blk_status_to_errno (bc->status); + +if (res == -EAGAIN && bc->nowait) +io_uring_cmd_issue_blocking (cmd); +else +io_uring_cmd_done (cmd, res,0 , issue_flags); +} + +static void bio_cmd_end (struct bio *bio) +{ +struct io_uring_cmd *cmd = bio->bi_private; + struct blk_cmd *bc = io_uring_cmd_to_pdu(cmd, struct blk_cmd); + +if (unlikely (bio->bi_status) && !bc->status) + bc->status = 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) +{ +sector_t sector = start >>SECTOR_SHIFT ; +sector_t nr_sects = len >>SECTOR_SHIFT ; +struct bio *prev =NULL , *bio; +int err; + + err =blk_validate_discard (bdev,file_to_blk_mode (cmd->file), + start, len); +if (err) +return err; + 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_KERNEL ))) { +if (nowait) { +if (unlikely (nr_sects)) { +bio_put (bio); +return -EAGAIN ; + } + bio->bi_opf |=REQ_NOWAIT ; + } + prev =bio_chain_and_submit (prev, bio); + } +if (!prev) +return -EFAULT ; + + prev->bi_private = cmd; + prev->bi_end_io = bio_cmd_end; +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_cmd *bc = io_uring_cmd_to_pdu(cmd, struct blk_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 ; + + bc->status =BLK_STS_OK ; + bc->nowait = issue_flags &IO_URING_F_NONBLOCK ;
block/ioctl.c long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg) · line 839
Quoted 10 lines
block/ioctl.c +10
-0+ + 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, bc->nowait); + } +return -EINVAL ; +}
This is inside the CONFIG_COMPAT section, which doesn't seem right.
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerQuoted 8 lines · Pavel Begunkov
There is an interest in having asynchronous block operations like discard. The patch set implements that as
io_uringcommands, which is anio_uringrequest type allowing to implement custom file specific operations.First 4 patches are simple preps, and the main part is in Patch 5. Not tested with a real drive yet, hence sending as an RFC.
Quoted 6 lines
I'm also going to add BLKDISCARDZEROES and BLKSECDISCARD, which should reuse structures and helpers from Patch 5.
liburing tests for reference:
https://github.com/isilence/liburing.git discard-cmd-test
FWIW, did a quick patch to wire it up for fio. Using this job file:
[trim]
filename=/dev/nvme31n1
ioengine=io_uring
iodepth=64
rw=randtrim
norandommap
bs=4k
the stock kernel gets:
trim: IOPS=21.6k, BW=84.4MiB/s (88.5MB/s)(847MiB/10036msec); 0 zone resets
using ~5% CPU, and with the process predictably stuck in D state all of the time, waiting on a sync trim.
With the patches:
trim: IOPS=75.8k, BW=296MiB/s (310MB/s)(2653MiB/8961msec); 0 zone resets
using ~11% CPU.
Didn't verify actual functionality, but did check trims are going up and down. Drive used is:
Dell NVMe PM1743 RI E3.S 7.68TB
Outside of async trim being useful for actual workloads rather than the sync trim we have now, it'll also help characterizing real world mixed workloads that have trims with reads/writes.
Signature · 1 line
Jens Axboe
Quoted 31 lines · Jens Axboe
Quoted 8 lines · Pavel Begunkov
There is an interest in having asynchronous block operations like discard. The patch set implements that as
io_uringcommands, which is anio_uringrequest type allowing to implement custom file specific operations.First 4 patches are simple preps, and the main part is in Patch 5. Not tested with a real drive yet, hence sending as an RFC.
Quoted 6 lines
I'm also going to add BLKDISCARDZEROES and BLKSECDISCARD, which should reuse structures and helpers from Patch 5.
liburing tests for reference:
https://github.com/isilence/liburing.git discard-cmd-test
FWIW, did a quick patch to wire it up for fio. Using this job file:
[trim]
filename=/dev/nvme31n1
ioengine=io_uring
iodepth=64
rw=randtrim
norandommap
bs=4kthe stock kernel gets:
trim: IOPS=21.6k, BW=84.4MiB/s (88.5MB/s)(847MiB/10036msec); 0 zone resets
Quoted 8 lines
using ~5% CPU, and with the process predictably stuck in D state all of the time, waiting on a sync trim.
With the patches:
trim: IOPS=75.8k, BW=296MiB/s (310MB/s)(2653MiB/8961msec); 0 zone resetsusing ~11% CPU.
Thanks for giving it a run. Can be further improved for particular use cases, e.g. by adding a vectored version as an additional feature, i.e. multiple discard ranges per request, and something like retry of short IO, but for that we'd want getting -EAGAIN directly from submit_bio unlike failing via the callback.
Quoted 8 lines
Didn't verify actual functionality, but did check trims are going up and down. Drive used is:
Dell NVMe PM1743 RI
E3.S7.68TBOutside of async trim being useful for actual workloads rather than the sync trim we have now, it'll also help characterizing real world mixed workloads that have trims with reads/writes.
Signature · 1 line
Pavel Begunkov
Hi Pavel!
Quoted 2 lines
I'm also going to add BLKDISCARDZEROES and BLKSECDISCARD, which should reuse structures and helpers from Patch 5.
Adding these is going to be very useful.
Just a nit: Please use either ZEROOUT (or WRITE_ZEROES) and SECURE_ERASE terminology for the additional operations when you add them.
DISCARDZEROES is obsolete and secure discard has been replaced by secure erase. We are stuck with the legacy ioctl names but we should avoid perpetuating unfortunate naming choices from the past.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 6 lines · Martin K. Petersen
Hi Pavel!
Quoted 2 lines
I'm also going to add BLKDISCARDZEROES and BLKSECDISCARD, which should reuse structures and helpers from Patch 5.
Quoted 8 lines
Adding these is going to be very useful.
Just a nit: Please use either ZEROOUT (or
WRITE_ZEROES) andSECURE_ERASEterminology for the additional operations when you add them.DISCARDZEROES is obsolete and secure discard has been replaced by secure erase. We are stuck with the legacy ioctl names but we should avoid perpetuating unfortunate naming choices from the past.
Noted, thanks, the ioctl names are indeed confusing.
Signature · 1 line
Pavel Begunkov