Re: [PATCH 2/2] io_uring: call statx directly

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, May 22, 2020 at 01:52:34AM +0100, Al Viro wrote:
> On Fri, May 22, 2020 at 01:50:53AM +0100, Al Viro wrote:
> > On Thu, May 21, 2020 at 05:19:37PM -0700, Bijan Mottahedeh wrote:
> > > Calling statx directly both simplifies the interface and avoids potential
> > > incompatibilities between sync and async invokations.
> > > 
> > > Signed-off-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
> > > ---
> > >  fs/io_uring.c | 53 +++++++----------------------------------------------
> > >  1 file changed, 7 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > > index 12284ea..0540961 100644
> > > --- a/fs/io_uring.c
> > > +++ b/fs/io_uring.c
> > > @@ -427,7 +427,10 @@ struct io_open {
> > >  	union {
> > >  		unsigned		mask;
> > >  	};
> > > -	struct filename			*filename;
> > > +	union {
> > > +		struct filename		*filename;
> > > +		const char __user	*fname;
> > > +	};
> > 
> > NAK.  io_uring is already has ridiculous amount of multiplexing,
> > but this kind of shit is right out.
> > 
> > And frankly, the more I look at it, the more I want to rip
> > struct io_open out.  This kind of trashcan structures has
> > caused tons of headache pretty much every time we had those.
> > Don't do it.
> 
> s/io_open/io_kiocb/, sorry for typo.

To elaborate a bit: whenever we have that kind of objects, the question
for reviewer/author looking at the code several months down the road/
somebody trying to hunt down a bug is
	what guarantees that we have an instance of such variant
	structure always treated as the _same_ variant?
And the more convoluted it is, the worse.

_IF_ you have a set of methods (for all variants) and that gets set
once when you create your object and never changes after that, it
can be more or less survivable.  You have nothing of that sort.

What's more, when preconditions needed to work with different variants
are not the same (e.g. different locking is needed, etc.), _their_
consistency gets added to analysis.  The same goes for "which context
will it run in/what guarantees that we can do uaccess/etc."

It's really painful to analyse right now.  And it'll get only worse
as more kludges pile on top of each other...