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

Jeff Moyer [off-list ref] writes:

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.

-Jeff

On 10/21/21 3:03 PM, Jeff Moyer wrote:

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.

-- 
Jens Axboe

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

On 10/22/21 9:29 AM, Jens Axboe wrote:

On 10/22/21 8:19 AM, Jens Axboe wrote:

quoted

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.

For the record, deferring all of this until next week when I'm back.
I'll pick it back up then, obviously this isn't an urgent thing at all,
would just love to sort out the useless argument going down the line.
We'd need to pack all of them, not just the odd ones out.

-- 
Jens Axboe