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

[Pavel Begunkov <asml.silence@gmail.com>]

On 8/12/24 12:26, Christoph Hellwig wrote:
> On Fri, Aug 09, 2024 at 06:35:27PM +0100, Mark Harmstone wrote:

Mark, please CC io_uring for next versions, I've only found out
about the patch because Christoph added us.

>> Adds an io_uring interface for asynchronous encoded reads, using the
>> same interface as for the ioctl. To use this you would use an SQE opcode
>> of IORING_OP_URING_CMD, the cmd_op would be BTRFS_IOC_ENCODED_READ, and
>> addr would point to the userspace address of the
>> btrfs_ioctl_encoded_io_args struct. As with the ioctl, you need to have
>> CAP_SYS_ADMIN for this to work.
> 
> What is the point if this doesn't actually do anything but returning
> -EIOCBQUEUED?
> 
> Note that that the internals of the btrfs encoded read is built
> around kiocbs anyway, so you might as well turn things upside down,
> implement a real async io_uring cmd and just wait for it to complete
> to implement the existing synchronous ioctl.
> 
>>
>> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
>> ---
...
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 0493272a7668..8f5cc7d1429c 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
...
>> +static void btrfs_uring_encoded_read_compat_cb(struct io_uring_cmd *cmd,
>> +					       unsigned int issue_flags)
>> +{
>> +	int ret;
>> +
>> +	ret = btrfs_ioctl_encoded_read(cmd->file, (void __user *)cmd->sqe->addr,
>> +				       true);
>> +
>> +	io_uring_cmd_done(cmd, ret, 0, issue_flags);
>> +}
>> +
>> +static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd,
>> +				    unsigned int issue_flags)
>> +{
>> +	if (issue_flags & IO_URING_F_COMPAT)

Instead of two different callbacks we can add a helper

# include/linux/io_uring/cmd.h

static inline bool io_uring_cmd_is_compat(struct io_uring_cmd *cmd)
{
#ifdef COMPAT
	struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);

	return req->ctx->compat;
#else
	return false;
#endif
}

But then we pass the flag in btrfs_ioctl_encoded_read() and use to
interpret struct btrfs_ioctl_encoded_io_args, but next just call
import_iovec(), which derives compat differently. Since io_uring worker
threads are now forks of the original thread, ctx->compat vs
in_compat_syscall() are only marginally different, e.g. when we pass a
ring to another process with a different compat'ness, but I don't think
that something we care about much. Let's just make it consistent.

And the last point, I'm surprised there are two versions of
btrfs_ioctl_encoded_io_args. Maybe, it's a good moment to fix it if
we're creating a new interface.

E.g. by adding a new structure defined right with u64 and such, use it
in io_uring, and cast to it in the ioctl code when it's x64 (with
a good set of BUILD_BUG_ON sprinkled) and convert structures otherwise?

>> +		io_uring_cmd_complete_in_task(cmd, btrfs_uring_encoded_read_compat_cb);
>> +	else
>> +		io_uring_cmd_complete_in_task(cmd, btrfs_uring_encoded_read_cb);

As mentioned, the callback will be executed by the submitter task.
You mentioned offloading to a thread/iowq, that would look like:

btrfs_uring_encoded_read() {
	if (issue_flags & IO_URING_F_NONBLOCK)
		return -EAGAIN;
	// it's a worker thread, block is allowed
}

It's a bad pattern though for anything requiring good performance.
At minimum it should try to execute with a nowait flag set first

nowait = issue_flags & IO_URING_F_NONBLOCK;
btrfs_ioctl_encoded_read(..., nowait);

If needs to block it would return -EAGAIN, so that the core
io_uring would spin up a worker thread for it. Even better
if it does it asynchronously as Christoph mentioned.

>> +
>> +	return -EIOCBQUEUED;
>> +}
>> +
>> +int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
>> +{
>> +	switch (cmd->cmd_op) {
>> +	case BTRFS_IOC_ENCODED_READ:
>> +#if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
>> +	case BTRFS_IOC_ENCODED_READ_32:
>> +#endif
>> +		return btrfs_uring_encoded_read(cmd, issue_flags);
>> +	}
>> +
>> +	io_uring_cmd_done(cmd, -EINVAL, 0, issue_flags);
>> +	return -EIOCBQUEUED;
>> +}
>> +
>>   long btrfs_ioctl(struct file *file, unsigned int

-- 
Pavel Begunkov