Re: [RFC 5/5] block: implement io_uring discard cmd

[Jens Axboe <axboe@kernel.dk>]

On 8/15/24 8:16 PM, Pavel Begunkov wrote:
> On 8/16/24 03:08, Ming Lei wrote:
>> On Fri, Aug 16, 2024 at 02:59:49AM +0100, Pavel Begunkov wrote:
>>> On 8/16/24 02:45, Ming Lei wrote:
>>>> On Thu, Aug 15, 2024 at 07:24:16PM -0600, Jens Axboe wrote:
>>>>> On 8/15/24 5:44 PM, Ming Lei wrote:
>>>>>> On Thu, Aug 15, 2024 at 06:11:13PM +0100, Pavel Begunkov wrote:
>>>>>>> On 8/15/24 15:33, Jens Axboe wrote:
>>>>>>>> On 8/14/24 7:42 PM, Ming Lei wrote:
>>>>>>>>> On Wed, Aug 14, 2024 at 6:46?PM Pavel Begunkov <asml.silence@gmail.com> 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 <conradmeyer@meta.com>
>>>>>>>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>     block/blk.h             |  1 +
>>>>>>>>>>     block/fops.c            |  2 +
>>>>>>>>>>     block/ioctl.c           | 94 +++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>     include/uapi/linux/fs.h |  2 +
>>>>>>>>>>     4 files changed, 99 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/block/blk.h b/block/blk.h
>>>>>>>>>> index e180863f918b..5178c5ba6852 100644
>>>>>>>>>> --- a/block/blk.h
>>>>>>>>>> +++ b/block/blk.h
>>>>>>>>>> @@ -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;
>>>>>>>>>> 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,
>>>>>>>>>
>>>>>>>>> Just be curious, we have IORING_OP_FALLOCATE already 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.
>>>>>>
>>>>>> 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, :-)
>>>>>
>>>>> 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?
>>>
>>> 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.
> 
> 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.

-- 
Jens Axboe