Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument
From: Jens Axboe <axboe@kernel.dk>
Date: 2021-10-22 15:29:50
Also in:
linux-fsdevel, linux-usb
Subsystem:
aio, filesystems (vfs and infrastructure), the rest, usb subsystem · Maintainers:
Benjamin LaHaise, Alexander Viro, Christian Brauner, Linus Torvalds, Greg Kroah-Hartman
Possibly related (same subject, not in this thread)
- 2021-10-21 · Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument · Jeff Moyer <hidden>
- 2021-10-21 · Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument · Jens Axboe <axboe@kernel.dk>
- 2021-10-21 · Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument · Jeff Moyer <hidden>
- 2021-10-21 · Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument · Jens Axboe <axboe@kernel.dk>
- 2021-10-21 · Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument · Jeff Moyer <hidden>
On 10/22/21 8:19 AM, Jens Axboe wrote:
On 10/21/21 3:03 PM, Jeff Moyer wrote:quoted
Jeff Moyer [off-list ref] writes:quoted
Jens Axboe [off-list ref] writes:quoted
On 10/21/21 12:05 PM, Jeff Moyer wrote:quoted
quoted
quoted
I'll follow up if there are issues.s390 (big endian, 64 bit) is failing libaio test 21: # harness/cases/21.p Expected -EAGAIN, got 4294967285 If I print out both res and res2 using %lx, you'll see what happened: Expected -EAGAIN, got fffffff5,ffffffff The sign extension is being split up.Funky, does it work if you apply this on top?diff --git a/fs/aio.c b/fs/aio.c index 3674abc43788..c56437908339 100644 --- a/fs/aio.c +++ b/fs/aio.c@@ -1442,8 +1442,8 @@ static void aio_complete_rw(struct kiocb *kiocb, u64 res) * 32-bits of value at most for either value, bundle these up and * pass them in one u64 value. */ - iocb->ki_res.res = lower_32_bits(res); - iocb->ki_res.res2 = upper_32_bits(res); + iocb->ki_res.res = (long) (res & 0xffffffff); + iocb->ki_res.res2 = (long) (res >> 32); iocb_put(iocb); }I think you'll also need to clamp any ki_complete() call sites to 32 bits (cast to int, or what have you). Otherwise that sign extension will spill over into res2. fwiw, I tested with this: iocb->ki_res.res = (long)(int)lower_32_bits(res); iocb->ki_res.res2 = (long)(int)upper_32_bits(res); Coupled with the call site changes, that made things work for me.This is all starting to feel like a minefield. If you don't have any concrete numbers to show that there is a speedup, I think we should shelf this change.It's really not a minefield at all, we just need a proper help to encode the value. I'm out until Tuesday, but I'll sort it out when I get back. Can also provide some numbers on this.
I think this incremental should fix it, also providing a helper to properly pack these. The more I look at the gadget stuff the more I also get the feeling that it really is wonky and nobody uses res2, which would be a nice cleanup to continue. But I think it should be separate.
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 8536f19d3c9a..9c5372229714 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c@@ -831,7 +831,7 @@ static void ffs_user_copy_worker(struct work_struct *work) kthread_unuse_mm(io_data->mm); } - io_data->kiocb->ki_complete(io_data->kiocb, ((u64) ret << 32) | ret); + io_data->kiocb->ki_complete(io_data->kiocb, aio_res_pack(ret, ret)); if (io_data->ffs->ffs_eventfd && !kiocb_has_eventfd) eventfd_signal(io_data->ffs->ffs_eventfd, 1);
diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index d3deb23eb2ab..15dff219b483 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c@@ -469,7 +469,7 @@ static void ep_user_copy_worker(struct work_struct *work) ret = -EFAULT; /* completing the iocb can drop the ctx and mm, don't touch mm after */ - iocb->ki_complete(iocb, ((u64) ret << 32) | ret); + iocb->ki_complete(iocb, aio_res_pack(ret, ret)); kfree(priv->buf); kfree(priv->to_free);
@@ -499,8 +499,10 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req) kfree(priv); iocb->private = NULL; /* aio_complete() reports bytes-transferred _and_ faults */ - aio_ret = req->actual ? req->actual : (long)req->status; - aio_ret |= (u64) req->status << 32; + if (req->actual) + aio_ret = aio_res_pack(req->actual, req->status); + else + aio_ret = aio_res_pack(req->status, req->status); iocb->ki_complete(iocb, aio_ret); } else { /* ep_copy_to_user() won't report both; we hide some faults */
diff --git a/fs/aio.c b/fs/aio.c
index 3674abc43788..cd43a26b2aa2 100644
--- a/fs/aio.c
+++ b/fs/aio.c@@ -1442,8 +1442,8 @@ static void aio_complete_rw(struct kiocb *kiocb, u64 res) * 32-bits of value at most for either value, bundle these up and * pass them in one u64 value. */ - iocb->ki_res.res = lower_32_bits(res); - iocb->ki_res.res2 = upper_32_bits(res); + iocb->ki_res.res = (long) lower_32_bits(res); + iocb->ki_res.res2 = (long) upper_32_bits(res); iocb_put(iocb); }
diff --git a/include/linux/aio.h b/include/linux/aio.h
index b83e68dd006f..50a6c7da27ec 100644
--- a/include/linux/aio.h
+++ b/include/linux/aio.h@@ -24,4 +24,18 @@ static inline void kiocb_set_cancel_fn(struct kiocb *req, extern unsigned long aio_nr; extern unsigned long aio_max_nr; +/* + * Take some care packing two 32-bit quantities into a 64-bit, so we don't + * get sign extensions ruining the result. aio uses long, but it's really + * just 32-bit values. + */ +static inline u64 aio_res_pack(long res, long res2) +{ + u64 ret; + + ret = (u64) res2 << 32; + ret |= (u32) res; + return ret; +} + #endif /* __LINUX__AIO_H */
--
Jens Axboe