[2/2] io_uring: call statx directly
diff mbox series

Message ID 1590106777-5826-3-git-send-email-bijan.mottahedeh@oracle.com
State New
Headers show
Series
  • io_uring: call statx directly
Related show

Commit Message

Bijan Mottahedeh May 22, 2020, 12:19 a.m. UTC
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(-)

Comments

Al Viro May 22, 2020, 12:50 a.m. UTC | #1
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.
Al Viro May 22, 2020, 12:52 a.m. UTC | #2
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.
Bijan Mottahedeh May 22, 2020, 1:20 a.m. UTC | #3
On 5/21/2020 5:50 PM, 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.

Are you suggesting a separate io_statx structure or something similar?

Are you ok with the addition of do_statx() in the first patch?

Thanks.

--bijan
Al Viro May 22, 2020, 1:21 a.m. UTC | #4
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...

Patch
diff mbox series

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;
+	};
 	struct statx __user		*buffer;
 	struct open_how			how;
 	unsigned long			nofile;
@@ -3368,43 +3371,23 @@  static int io_fadvise(struct io_kiocb *req, bool force_nonblock)
 
 static int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
-	const char __user *fname;
-	unsigned lookup_flags;
-	int ret;
-
 	if (sqe->ioprio || sqe->buf_index)
 		return -EINVAL;
 	if (req->flags & REQ_F_FIXED_FILE)
 		return -EBADF;
-	if (req->flags & REQ_F_NEED_CLEANUP)
-		return 0;
 
 	req->open.dfd = READ_ONCE(sqe->fd);
 	req->open.mask = READ_ONCE(sqe->len);
-	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
+	req->open.fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	req->open.buffer = u64_to_user_ptr(READ_ONCE(sqe->addr2));
 	req->open.how.flags = READ_ONCE(sqe->statx_flags);
 
-	if (vfs_stat_set_lookup_flags(&lookup_flags, req->open.how.flags))
-		return -EINVAL;
-
-	req->open.filename = getname_flags(fname, lookup_flags, NULL);
-	if (IS_ERR(req->open.filename)) {
-		ret = PTR_ERR(req->open.filename);
-		req->open.filename = NULL;
-		return ret;
-	}
-
-	req->flags |= REQ_F_NEED_CLEANUP;
 	return 0;
 }
 
 static int io_statx(struct io_kiocb *req, bool force_nonblock)
 {
 	struct io_open *ctx = &req->open;
-	unsigned lookup_flags;
-	struct path path;
-	struct kstat stat;
 	int ret;
 
 	if (force_nonblock) {
@@ -3414,29 +3397,9 @@  static int io_statx(struct io_kiocb *req, bool force_nonblock)
 		return -EAGAIN;
 	}
 
-	if (vfs_stat_set_lookup_flags(&lookup_flags, ctx->how.flags))
-		return -EINVAL;
+	ret = do_statx(ctx->dfd, ctx->fname, ctx->how.flags, ctx->mask,
+		       ctx->buffer);
 
-retry:
-	/* filename_lookup() drops it, keep a reference */
-	ctx->filename->refcnt++;
-
-	ret = filename_lookup(ctx->dfd, ctx->filename, lookup_flags, &path,
-				NULL);
-	if (ret)
-		goto err;
-
-	ret = vfs_getattr(&path, &stat, ctx->mask, ctx->how.flags);
-	path_put(&path);
-	if (retry_estale(ret, lookup_flags)) {
-		lookup_flags |= LOOKUP_REVAL;
-		goto retry;
-	}
-	if (!ret)
-		ret = cp_statx(&stat, ctx->buffer);
-err:
-	putname(ctx->filename);
-	req->flags &= ~REQ_F_NEED_CLEANUP;
 	if (ret < 0)
 		req_set_fail_links(req);
 	io_cqring_add_event(req, ret);
@@ -5198,8 +5161,6 @@  static void io_cleanup_req(struct io_kiocb *req)
 		break;
 	case IORING_OP_OPENAT:
 	case IORING_OP_OPENAT2:
-	case IORING_OP_STATX:
-		putname(req->open.filename);
 		break;
 	case IORING_OP_SPLICE:
 	case IORING_OP_TEE: