[PATCH 0/2] io_uring: call statx directly

[PATCH 0/2] io_uring: call statx directly

[Bijan Mottahedeh]

This patch set is a fix for the liburing statx test failure.

The test fails with a "Miscompare between io_uring and statx" error
because the statx system call path has additional processing in vfs_statx():

        stat->result_mask |= STATX_MNT_ID;
        if (path.mnt->mnt_root == path.dentry)
                stat->attributes |= STATX_ATTR_MOUNT_ROOT;
        stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;

which then results in different result_mask values.

Allowing the system call to be invoked directly simplifies the io_uring
interface and avoids potential future incompatibilities.  I'm not sure
if there was other reasoning fort not doing so initially.

One issue I cannot account for is the difference in "used" memory reported
by free(1) after running the statx a large (10000) number of times.

The difference is significant ~100k and doesn't really change after
dropping caches.

I enabled memory leak detection and couldn't see anything related to the test.

Bijan Mottahedeh (2):
  statx: allow the system call to be invoked from the kernel
  io_uring: call statx directly

 fs/internal.h |  2 ++
 fs/io_uring.c | 53 +++++++----------------------------------------------
 fs/stat.c     | 32 +++++++++++++++++++-------------
 3 files changed, 28 insertions(+), 59 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] statx: allow the system call to be invoked from the kernel

[Bijan Mottahedeh]

This is a prepatory patch to allow io_uring to invoke statx directly.

Signed-off-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
---
 fs/internal.h |  2 ++
 fs/stat.c     | 32 +++++++++++++++++++-------------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index aaa946d..37055a4 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -203,3 +203,5 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
  */
 unsigned vfs_stat_set_lookup_flags(unsigned *lookup_flags, int flags);
 int cp_statx(const struct kstat *stat, struct statx __user *buffer);
+int do_statx(int dfd, const char __user *filename, unsigned flags,
+	     unsigned int mask, struct statx __user *buffer);
diff --git a/fs/stat.c b/fs/stat.c
index 3213d1b..bc5d2e81 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -577,6 +577,24 @@ static long cp_new_stat64(struct kstat *stat, struct stat64 __user *statbuf)
 	return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
 }
 
+int do_statx(int dfd, const char __user *filename, unsigned flags,
+	     unsigned int mask, struct statx __user *buffer)
+{
+	struct kstat stat;
+	int error;
+
+	if (mask & STATX__RESERVED)
+		return -EINVAL;
+	if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
+		return -EINVAL;
+
+	error = vfs_statx(dfd, filename, flags, &stat, mask);
+	if (error)
+		return error;
+
+	return cp_statx(&stat, buffer);
+}
+
 /**
  * sys_statx - System call to get enhanced stats
  * @dfd: Base directory to pathwalk from *or* fd to stat.
@@ -593,19 +611,7 @@ static long cp_new_stat64(struct kstat *stat, struct stat64 __user *statbuf)
 		unsigned int, mask,
 		struct statx __user *, buffer)
 {
-	struct kstat stat;
-	int error;
-
-	if (mask & STATX__RESERVED)
-		return -EINVAL;
-	if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
-		return -EINVAL;
-
-	error = vfs_statx(dfd, filename, flags, &stat, mask);
-	if (error)
-		return error;
-
-	return cp_statx(&stat, buffer);
+	return do_statx(dfd, filename, flags, mask, buffer);
 }
 
 #ifdef CONFIG_COMPAT
-- 
1.8.3.1

[PATCH 2/2] io_uring: call statx directly

[Bijan Mottahedeh]

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;
+	};
 	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:
-- 
1.8.3.1

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

[Al Viro]

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.

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

[Al Viro]

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.

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

[Bijan Mottahedeh]

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

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

[Al Viro]

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...