| Message ID | 1590106777-5826-3-git-send-email-bijan.mottahedeh@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io_uring: call statx directly | expand |
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.
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.
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
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...
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:
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(-)