Re: [PATCH] btrfs: add io_uring interface for encoded writes

[Mark Harmstone <maharmstone@meta.com>]

On 12/11/24 21:01, Jens Axboe wrote:
> > 
> On 11/12/24 9:29 AM, Mark Harmstone wrote:
>> Add an io_uring interface for encoded writes, with the same parameters
>> as the BTRFS_IOC_ENCODED_WRITE ioctl.
>>
>> As with the encoded reads code, there's a test program for this at
>> https://github.com/maharmstone/io_uring-encoded, and I'll get this
>> worked into an fstest.
>>
>> How io_uring works is that it initially calls btrfs_uring_cmd with the
>> IO_URING_F_NONBLOCK flag set, and if we return -EAGAIN it tries again in
>> a kthread with the flag cleared.
>      ^^^^^^^^
> 
> Not a kernel thread, it's an io worker. The distinction may seem
> irrelevant, but it's really not - io workers inherit all the properties
> of the original task.
> 
>> Ideally we'd honour this and call try_lock etc., but there's still a lot
>> of work to be done to create non-blocking versions of all the functions
>> in our write path. Instead, just validate the input in
>> btrfs_uring_encoded_write() on the first pass and return -EAGAIN, with a
>> view to properly optimizing the happy path later on.
> 
> But you need to ensure stable state after the first issue, regardless of
> how you handle it. I don't have the other patches handy, but whatever
> you copy from userspace before you return -EAGAIN, you should not be
> copying again. By the time you get the 2nd invocation from io-wq, no
> copying should be taking place, you should be using the state you
> already ensured was stable for the non-blocking issue.
> 
> Maybe this is all handled by the caller of btrfs_uring_encoded_write()
> already? As far as looking at the code below, it just looks like it
> copies everything, then returns -EAGAIN, then copies it again later? Yes
> uring_cmd will make the sqe itself stable, but:
> 
> 	sqe_addr = u64_to_user_ptr(READ_ONCE(cmd->sqe->addr));
> 
> the userspace btrfs_ioctl_encoded_io_args that sqe->addr points too
> should remain stable as well. If not, consider userspace doing:
> 
> some_func()
> {
> 	struct btrfs_ioctl_encoded_io_args args;
> 
> 	fill_in_args(&args);
> 	sqe = io_uring_get_sqe(ring);
> 	sqe->addr = &args;
> 	io_uring_submit();		<- initial invocation here
> }
> 
> main_func()
> {
> 	some_func();
> 				- io-wq invocation perhaps here
> 	wait_on_cqes();
> }
> 
> where io-wq will be reading garbage as args went out of scope, unless
> some_func() used a stable/heap struct that isn't freed until completion.
> some_func() can obviously wait on the cqe, but at that point you'd be
> using it as a sync interface, and there's little point.
> 
> This is why io_kiocb->async_data exists. uring_cmd is already using that
> for the sqe, I think you'd want to add a 2nd "void *op_data" or
> something in there, and have the uring_cmd alloc cache get clear that to
> NULL and have uring_cmd alloc cache put kfree() it if it's non-NULL.
> 
> We'd also need to move the uring_cache struct into
> include/linux/io_uring_types.h so that btrfs can get to it (and probably
> rename it to something saner, uring_cmd_async_data for example).
> 
> static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issue_flags)
> {
> 	struct io_kiocb *req = cmd_to_io_kiocb(cmd);
> 	struct uring_cmd_async_data *data = req->async_data;
> 	struct btrfs_ioctl_encoded_io_args *args;
> 
> 	if (!data->op_data) {
> 		data->op_data = kmalloc(sizeof(*args), GFP_NOIO);
> 		if (!data->op_data)
> 			return -ENOMEM;
> 		if (copy_from_user(data->op_data, sqe_addr, sizeof(*args))
> 			return -EFAULT;
> 	}
> 	...
> }
> 
> and have it be stable, then moving your copying into a helper rather
> than inline in btrfs_uring_encoded_write() (it probably should be
> regardless). Ignored the compat above, it's just pseudo code.
> 
> Anyway, hope that helps. I'll be happy to do the uring_cmd bit for you,
> but it really should be pretty straight forward.
> 
> I'm also pondering if the encoded read side suffers from the same issue?
> 

Thanks Jens, that makes sense to me.

Mark