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
