Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument

[Jens Axboe <axboe@kernel.dk>]

On 10/22/21 8:19 AM, Jens Axboe wrote:
> On 10/21/21 3:03 PM, Jeff Moyer wrote:
>> Jeff Moyer <jmoyer@redhat.com> writes:
>>
>>> Jens Axboe <axboe@kernel.dk> 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