Re: [PATCH v9 06/11] io_uring: introduce attributes for read/write and PI support

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

On 11/14/24 15:19, Christoph Hellwig wrote:
> On Thu, Nov 14, 2024 at 01:09:44PM +0000, Pavel Begunkov wrote:
>>> Eww.  I know it's frustration for your if maintainers give contradicting
>>> guidance, but this is really an awful interface.  Not only the pointless
>>
>> Because once you placed it at a fixed location nothing realistically
>> will be able to reuse it. Not everyone will need PI, but the assumption
>> that there will be more more additional types of attributes / parameters.
> 
> So?  If we have a strong enough requirement for something else we
> can triviall add another opcode.  Maybe we should just add different
> opcodes for read/write with metadata so that folks don't freak out
> about this?

IMHO, PI is not so special to have a special opcode for it unlike
some more generic read/write with meta / attributes, but that one
would have same questions.

FWIW, the series was steered from the separate opcode approach to avoid
duplicating things, for example there are 3 different OP_READ* opcodes
varying by the buffer type, and there is no reason meta reads wouldn't
want to support all of them as well. I have to admit that the effort is
a bit unfortunate on that side switching back a forth at least a couple
of times including attempts from 2+ years ago by some other guy.

>> With SQE128 it's also a problem that now all SQEs are 128 bytes regardless
>> of whether a particular request needs it or not, and the user will need
>> to zero them for each request.
> 
> The user is not going to create a SQE128 ring unless they need to,
> so this seem like a bit of an odd objection.

It doesn't bring this overhead to those who don't use meta/PI, that's
right, but it does add it if you want to mix it with nearly all other
request types, and that is desirable.

As I mentioned before, it's just one downside but not a deal breaker.
I'm more concerned that the next type of meta information won't be
able to fit into the SQE and then we'll need to solve the same problem
(indirection + optimising copy_from_user with other means) while having
PI as a special case. And that's more of a problem of the static
placing from previous version, e.g. it wouldn't be a problem if in the
long run it becomes sth like:

struct attr attr, *p;

if (flags & META_IN_USE_SQE128)
	p = sqe + 1;
else {
	copy_from_user(&attr);
	p = &attr;
}

but that shouldn't be PI specific.

-- 
Pavel Begunkov