Re: Questions regarding implementation of vmsplice in io_uring

[Jens Axboe <axboe@kernel.dk>]

On 1/3/21 3:22 PM, arni@dagur.eu wrote:
> From: Árni Dagur <arnidg@protonmail.ch>
> 
> Hello,
> 
> For my first stab at kernel development, I wanted to try implementing
> vmsplice for io_uring. I've attached the code I've written so far. I have two
> questions to ask, sorry if this is not the right place.
> 
> 1. Currently I use __import_iovec directly, instead of using
> io_import_iovec. That's because I've created a new "kiocb" struct
> called io_vmsplice, rather than using io_rw as io_import_iovec expects.
> The reason I created a new struct is so that it can hold an unsigned int
> for the flags argument -- which is not present in io_rw. Im guessing that I
> should find a way to use io_import_iovec instead?
> 
> One way I can think of is giving the io_vmsplice struct the same initial
> fields as io_rw, and letting io_import_iovec access the union as io_rw rather
> than io_vmsplice. Coming from a Rust background however, this solution
> sounds like a bug waiting to happen (if one of the structs is changed
> but the other is not).
> 
> 2. Whenever I run the test program at
> https://gist.githubusercontent.com/ArniDagur/07d87aefae93868ca1bf10766194599d/raw/dc14a63649d530e5e29f0d1288f41ed54bc6b810/main.c
> I get a BADF result value. The debugger tells me that this occurs
> because `file->f_op != &pipefifo_fops` in get_pipe_info() in fs/pipe.c
> (neither pointer is NULL).
> 
> I give the program the file descriptor "1". Shouldn't that always be a pipe?
> Is there something obvious that I'm missing?

The change looks reasonable, some changes needed around not blocking.
But you can't use the splice ops with a tty, you need to use an end of a
pipe. That's why you get -EBADF in your test program. I'm assuming you
didn't run the one you sent, because you're overwriting ->addr in that
one by setting splice_off_in after having assigned ->addr using the prep
function?

> @@ -967,6 +976,11 @@ static const struct io_op_def io_op_defs[] = {
>  		.unbound_nonreg_file	= 1,
>  		.work_flags		= IO_WQ_WORK_BLKCG,
>  	},
> +	[IORING_OP_VMSPLICE] = {
> +		.needs_file = 1,
> +		.hash_reg_file		= 1,
> +		.unbound_nonreg_file	= 1,
> 
> I couldn't find any information regarding what the work flags do, so
> I've left them empty for now.

As a minimum, you'd need IO_WQ_WORK_MM I think for the async part of it,
if we need to block.

Various style issues in here too, like lines that are too long and
function braces need to go on a new line (and no braces for single
lines). If you want to move further with this, also split it into two
patches. The first should do the abstraction needed for splice.[ch] and
the second should be the io_uring change.

-- 
Jens Axboe