On 10/22/21 8:19 AM, Jens Axboe wrote:
> On 10/21/21 3:03 PM, Jeff Moyer wrote:
>> Jeff Moyer <jmoyer@xxxxxxxxxx> writes:
>>
>>> Jens Axboe <axboe@xxxxxxxxx> writes:
>>>
>>>> On 10/21/21 12:05 PM, Jeff Moyer wrote:
>>>>>
>>>>>>> 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
