[PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

It's not used for anything, and we're wasting time passing in zeroes
where we could just ignore it instead. Update all ki_complete users in
the kernel to drop that last argument.

The exception is the USB gadget code, which passes in non-zero. But
since nobody every looks at ret2, it's still pointless.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/block/fops.c b/block/fops.c
index d7eff331a07c..53408271771a 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -164,7 +164,7 @@ static void blkdev_bio_end_io(struct bio *bio)
 				ret = blk_status_to_errno(dio->bio.bi_status);
 			}
 
-			iocb->ki_complete(iocb, ret, 0);
+			iocb->ki_complete(iocb, ret);
 			if (flags & DIO_MULTI_BIO)
 				bio_put(&dio->bio);
 		} else {
@@ -318,7 +318,7 @@ static void blkdev_bio_end_io_async(struct bio *bio)
 		ret = blk_status_to_errno(bio->bi_status);
 	}
 
-	iocb->ki_complete(iocb, ret, 0);
+	iocb->ki_complete(iocb, ret);
 
 	if (dio->flags & DIO_SHOULD_DIRTY) {
 		bio_check_pages_dirty(bio);
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 8bd288d2b089..3dd5a773c320 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1076,7 +1076,7 @@ void af_alg_async_cb(struct crypto_async_request *_req, int err)
 	af_alg_free_resources(areq);
 	sock_put(sk);
 
-	iocb->ki_complete(iocb, err ? err : (int)resultlen, 0);
+	iocb->ki_complete(iocb, err ? err : (int)resultlen);
 }
 EXPORT_SYMBOL_GPL(af_alg_async_cb);
 
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 397bfafc4c25..92b87aa8be86 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -550,7 +550,7 @@ static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
 		blk_mq_complete_request(rq);
 }
 
-static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
+static void lo_rw_aio_complete(struct kiocb *iocb, long ret)
 {
 	struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb);
 
@@ -623,7 +623,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
 	lo_rw_aio_do_completion(cmd);
 
 	if (ret != -EIOCBQUEUED)
-		cmd->iocb.ki_complete(&cmd->iocb, ret, 0);
+		lo_rw_aio_complete(&cmd->iocb, ret);
 	return 0;
 }
 
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 7a249b752f3c..80a0f35ae1dc 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -123,7 +123,7 @@ static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
 	return call_iter(iocb, &iter);
 }
 
-static void nvmet_file_io_done(struct kiocb *iocb, long ret, long ret2)
+static void nvmet_file_io_done(struct kiocb *iocb, long ret)
 {
 	struct nvmet_req *req = container_of(iocb, struct nvmet_req, f.iocb);
 	u16 status = NVME_SC_SUCCESS;
@@ -220,7 +220,7 @@ static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 	}
 
 complete:
-	nvmet_file_io_done(&req->f.iocb, ret, 0);
+	nvmet_file_io_done(&req->f.iocb, ret);
 	return true;
 }
 
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index b471e726bb3d..968ace2ddf64 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -245,7 +245,7 @@ struct target_core_file_cmd {
 	struct bio_vec	bvecs[];
 };
 
-static void cmd_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
+static void cmd_rw_aio_complete(struct kiocb *iocb, long ret)
 {
 	struct target_core_file_cmd *cmd;
 
@@ -301,7 +301,7 @@ fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 		ret = call_read_iter(file, &aio_cmd->iocb, &iter);
 
 	if (ret != -EIOCBQUEUED)
-		cmd_rw_aio_complete(&aio_cmd->iocb, ret, 0);
+		cmd_rw_aio_complete(&aio_cmd->iocb, ret);
 
 	return 0;
 }
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 8260f38025b7..e20c19a0f106 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, ret, ret);
+	io_data->kiocb->ki_complete(io_data->kiocb, 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 539220d7f5b6..ad1739dbfab9 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, ret, ret);
+	iocb->ki_complete(iocb, ret);
 
 	kfree(priv->buf);
 	kfree(priv->to_free);
@@ -499,8 +499,7 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
 		/* aio_complete() reports bytes-transferred _and_ faults */
 
 		iocb->ki_complete(iocb,
-				req->actual ? req->actual : (long)req->status,
-				req->status);
+				req->actual ? req->actual : (long)req->status);
 	} else {
 		/* ep_copy_to_user() won't report both; we hide some faults */
 		if (unlikely(0 != req->status))
diff --git a/fs/aio.c b/fs/aio.c
index 51b08ab01dff..836dc7e48db7 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1417,7 +1417,7 @@ static void aio_remove_iocb(struct aio_kiocb *iocb)
 	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
 }
 
-static void aio_complete_rw(struct kiocb *kiocb, long res, long res2)
+static void aio_complete_rw(struct kiocb *kiocb, long res)
 {
 	struct aio_kiocb *iocb = container_of(kiocb, struct aio_kiocb, rw);
 
@@ -1437,7 +1437,7 @@ static void aio_complete_rw(struct kiocb *kiocb, long res, long res2)
 	}
 
 	iocb->ki_res.res = res;
-	iocb->ki_res.res2 = res2;
+	iocb->ki_res.res2 = 0;
 	iocb_put(iocb);
 }
 
@@ -1508,7 +1508,7 @@ static inline void aio_rw_done(struct kiocb *req, ssize_t ret)
 		ret = -EINTR;
 		fallthrough;
 	default:
-		req->ki_complete(req, ret, 0);
+		req->ki_complete(req, ret);
 	}
 }
 
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index fac2e8e7b533..effe37ef8629 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -37,11 +37,11 @@ static inline void cachefiles_put_kiocb(struct cachefiles_kiocb *ki)
 /*
  * Handle completion of a read from the cache.
  */
-static void cachefiles_read_complete(struct kiocb *iocb, long ret, long ret2)
+static void cachefiles_read_complete(struct kiocb *iocb, long ret)
 {
 	struct cachefiles_kiocb *ki = container_of(iocb, struct cachefiles_kiocb, iocb);
 
-	_enter("%ld,%ld", ret, ret2);
+	_enter("%ld", ret);
 
 	if (ki->term_func) {
 		if (ret >= 0)
@@ -139,7 +139,7 @@ static int cachefiles_read(struct netfs_cache_resources *cres,
 		fallthrough;
 	default:
 		ki->was_async = false;
-		cachefiles_read_complete(&ki->iocb, ret, 0);
+		cachefiles_read_complete(&ki->iocb, ret);
 		if (ret > 0)
 			ret = 0;
 		break;
@@ -159,12 +159,12 @@ static int cachefiles_read(struct netfs_cache_resources *cres,
 /*
  * Handle completion of a write to the cache.
  */
-static void cachefiles_write_complete(struct kiocb *iocb, long ret, long ret2)
+static void cachefiles_write_complete(struct kiocb *iocb, long ret)
 {
 	struct cachefiles_kiocb *ki = container_of(iocb, struct cachefiles_kiocb, iocb);
 	struct inode *inode = file_inode(ki->iocb.ki_filp);
 
-	_enter("%ld,%ld", ret, ret2);
+	_enter("%ld", ret);
 
 	/* Tell lockdep we inherited freeze protection from submission thread */
 	__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
@@ -244,7 +244,7 @@ static int cachefiles_write(struct netfs_cache_resources *cres,
 		fallthrough;
 	default:
 		ki->was_async = false;
-		cachefiles_write_complete(&ki->iocb, ret, 0);
+		cachefiles_write_complete(&ki->iocb, ret);
 		if (ret > 0)
 			ret = 0;
 		break;
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 5c1954ff3a82..41f4ca038191 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -1022,7 +1022,7 @@ static void ceph_aio_complete(struct inode *inode,
 	ceph_put_cap_refs(ci, (aio_req->write ? CEPH_CAP_FILE_WR :
 						CEPH_CAP_FILE_RD));
 
-	aio_req->iocb->ki_complete(aio_req->iocb, ret, 0);
+	aio_req->iocb->ki_complete(aio_req->iocb, ret);
 
 	ceph_free_cap_flush(aio_req->prealloc_cf);
 	kfree(aio_req);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 13f3182cf796..1b855fcb179e 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -3184,7 +3184,7 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
 	mutex_unlock(&ctx->aio_mutex);
 
 	if (ctx->iocb && ctx->iocb->ki_complete)
-		ctx->iocb->ki_complete(ctx->iocb, ctx->rc, 0);
+		ctx->iocb->ki_complete(ctx->iocb, ctx->rc);
 	else
 		complete(&ctx->done);
 }
@@ -3917,7 +3917,7 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
 	mutex_unlock(&ctx->aio_mutex);
 
 	if (ctx->iocb && ctx->iocb->ki_complete)
-		ctx->iocb->ki_complete(ctx->iocb, ctx->rc, 0);
+		ctx->iocb->ki_complete(ctx->iocb, ctx->rc);
 	else
 		complete(&ctx->done);
 }
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 453dcff0e7f5..654443558047 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -307,7 +307,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
 
 		if (ret > 0 && dio->op == REQ_OP_WRITE)
 			ret = generic_write_sync(dio->iocb, ret);
-		dio->iocb->ki_complete(dio->iocb, ret, 0);
+		dio->iocb->ki_complete(dio->iocb, ret);
 	}
 
 	kmem_cache_free(dio_cache, dio);
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 11404f8c21c7..e6039f22311b 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -687,7 +687,7 @@ static void fuse_aio_complete(struct fuse_io_priv *io, int err, ssize_t pos)
 			spin_unlock(&fi->lock);
 		}
 
-		io->iocb->ki_complete(io->iocb, res, 0);
+		io->iocb->ki_complete(io->iocb, res);
 	}
 
 	kref_put(&io->refcnt, fuse_io_release);
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5edde3b2f72d..5ad046145f29 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2672,7 +2672,7 @@ static void __io_complete_rw(struct io_kiocb *req, long res, long res2,
 	__io_req_complete(req, issue_flags, req->result, io_put_rw_kbuf(req));
 }
 
-static void io_complete_rw(struct kiocb *kiocb, long res, long res2)
+static void io_complete_rw(struct kiocb *kiocb, long res)
 {
 	struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw.kiocb);
 
@@ -2683,7 +2683,7 @@ static void io_complete_rw(struct kiocb *kiocb, long res, long res2)
 	io_req_task_work_add(req);
 }
 
-static void io_complete_rw_iopoll(struct kiocb *kiocb, long res, long res2)
+static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)
 {
 	struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw.kiocb);
 
@@ -2891,7 +2891,7 @@ static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
 		ret = -EINTR;
 		fallthrough;
 	default:
-		kiocb->ki_complete(kiocb, ret, 0);
+		kiocb->ki_complete(kiocb, ret);
 	}
 }
 
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 83ecfba53abe..811c898125a5 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -125,7 +125,7 @@ static void iomap_dio_complete_work(struct work_struct *work)
 	struct iomap_dio *dio = container_of(work, struct iomap_dio, aio.work);
 	struct kiocb *iocb = dio->iocb;
 
-	iocb->ki_complete(iocb, iomap_dio_complete(dio), 0);
+	iocb->ki_complete(iocb, iomap_dio_complete(dio));
 }
 
 /*
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 2e894fec036b..7a5f287c5391 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -275,7 +275,7 @@ static void nfs_direct_complete(struct nfs_direct_req *dreq)
 			res = (long) dreq->count;
 			WARN_ON_ONCE(dreq->count < 0);
 		}
-		dreq->iocb->ki_complete(dreq->iocb, res, 0);
+		dreq->iocb->ki_complete(dreq->iocb, res);
 	}
 
 	complete(&dreq->completion);
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index c88ac571593d..ac461a499882 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -272,14 +272,14 @@ static void ovl_aio_cleanup_handler(struct ovl_aio_req *aio_req)
 	kmem_cache_free(ovl_aio_request_cachep, aio_req);
 }
 
-static void ovl_aio_rw_complete(struct kiocb *iocb, long res, long res2)
+static void ovl_aio_rw_complete(struct kiocb *iocb, long res)
 {
 	struct ovl_aio_req *aio_req = container_of(iocb,
 						   struct ovl_aio_req, iocb);
 	struct kiocb *orig_iocb = aio_req->orig_iocb;
 
 	ovl_aio_cleanup_handler(aio_req);
-	orig_iocb->ki_complete(orig_iocb, res, res2);
+	orig_iocb->ki_complete(orig_iocb, res);
 }
 
 static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 31029a91f440..0dcb9020a7b3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -330,7 +330,7 @@ struct kiocb {
 	randomized_struct_fields_start
 
 	loff_t			ki_pos;
-	void (*ki_complete)(struct kiocb *iocb, long ret, long ret2);
+	void (*ki_complete)(struct kiocb *iocb, long ret);
 	void			*private;
 	int			ki_flags;
 	u16			ki_hint;

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Christoph Hellwig]

On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:
> It's not used for anything, and we're wasting time passing in zeroes
> where we could just ignore it instead. Update all ki_complete users in
> the kernel to drop that last argument.
> 
> The exception is the USB gadget code, which passes in non-zero. But
> since nobody every looks at ret2, it's still pointless.

Yes, the USB gadget passes non-zero, and aio passes that on to
userspace.  So this is an ABI change.  Does it actually matter?
I don't know, but you could CC the relevant maintainers and list
to try to figure that out.

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/20/21 11:30 AM, Christoph Hellwig wrote:
> On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:
>> It's not used for anything, and we're wasting time passing in zeroes
>> where we could just ignore it instead. Update all ki_complete users in
>> the kernel to drop that last argument.
>>
>> The exception is the USB gadget code, which passes in non-zero. But
>> since nobody every looks at ret2, it's still pointless.
> 
> Yes, the USB gadget passes non-zero, and aio passes that on to
> userspace.  So this is an ABI change.  Does it actually matter?
> I don't know, but you could CC the relevant maintainers and list
> to try to figure that out.

True, guess it does go out to userspace. Greg, is anyone using
it on the userspace side?

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Greg Kroah-Hartman]

On Wed, Oct 20, 2021 at 11:35:27AM -0600, Jens Axboe wrote:
> On 10/20/21 11:30 AM, Christoph Hellwig wrote:
> > On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:
> >> It's not used for anything, and we're wasting time passing in zeroes
> >> where we could just ignore it instead. Update all ki_complete users in
> >> the kernel to drop that last argument.
> >>
> >> The exception is the USB gadget code, which passes in non-zero. But
> >> since nobody every looks at ret2, it's still pointless.
> > 
> > Yes, the USB gadget passes non-zero, and aio passes that on to
> > userspace.  So this is an ABI change.  Does it actually matter?
> > I don't know, but you could CC the relevant maintainers and list
> > to try to figure that out.
> 
> True, guess it does go out to userspace. Greg, is anyone using
> it on the userspace side?

I really do not know (adding linux-usb@vger)  My interactions with the
gadget code have not been through the aio api, thankfully :)

Odds are it's fine, I think that something had to be passed in there so
that was chosen?  If the aio code didn't do anything with it, I can't
see where the gadget code gets it back at anywhere, but I might be
looking in the wrong place.

Anyone else here know?

thanks,

greg k-h

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jeff Moyer]

Hi, Jens,

Jens Axboe <axboe@kernel.dk> writes:

> It's not used for anything, and we're wasting time passing in zeroes
> where we could just ignore it instead. Update all ki_complete users in
> the kernel to drop that last argument.

What does "wasting time passing in zeroes" mean?

> The exception is the USB gadget code, which passes in non-zero. But
> since nobody every looks at ret2, it's still pointless.

As Christoph mentioned, it is passed along to userspace as part of the
io_event.

> @@ -499,8 +499,7 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
>  		/* aio_complete() reports bytes-transferred _and_ faults */

Note this comment ^^^

>  
>  		iocb->ki_complete(iocb,
> -				req->actual ? req->actual : (long)req->status,
> -				req->status);
> +				req->actual ? req->actual : (long)req->status);

We can't know whether some userspace implementation relies on this
behavior, so I don't think you can change it.

Cheers,
Jeff

p.s. Please CC linux-aio on future changes to the aio code.

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/20/21 12:16 PM, Jeff Moyer wrote:
> Hi, Jens,
> 
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> It's not used for anything, and we're wasting time passing in zeroes
>> where we could just ignore it instead. Update all ki_complete users in
>> the kernel to drop that last argument.
> 
> What does "wasting time passing in zeroes" mean?

That everybody but the funky usb gadget code passes in zero, hence it's
a waste of time to pass it in as an argument.

>> The exception is the USB gadget code, which passes in non-zero. But
>> since nobody every looks at ret2, it's still pointless.
> 
> As Christoph mentioned, it is passed along to userspace as part of the
> io_event.

Right

>> @@ -499,8 +499,7 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
>>  		/* aio_complete() reports bytes-transferred _and_ faults */
> 
> Note this comment ^^^
> 
>>  
>>  		iocb->ki_complete(iocb,
>> -				req->actual ? req->actual : (long)req->status,
>> -				req->status);
>> +				req->actual ? req->actual : (long)req->status);
> 
> We can't know whether some userspace implementation relies on this
> behavior, so I don't think you can change it.

Well, I think we should find out, particularly as it's the sole user of
that extra argument. No generic aio code would look at res2, exactly
because it is always zero for anything but some weird usb gadget code.

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On 10/20/21 12:16 PM, Jeff Moyer wrote:
>> Hi, Jens,
>> 
>> Jens Axboe <axboe@kernel.dk> writes:
>> 
>>> It's not used for anything, and we're wasting time passing in zeroes
>>> where we could just ignore it instead. Update all ki_complete users in
>>> the kernel to drop that last argument.
>> 
>> What does "wasting time passing in zeroes" mean?
>
> That everybody but the funky usb gadget code passes in zero, hence it's
> a waste of time to pass it in as an argument.

OK.  Just making sure you hadn't found some performance gain from this.
:)

>> We can't know whether some userspace implementation relies on this
>> behavior, so I don't think you can change it.
>
> Well, I think we should find out, particularly as it's the sole user of
> that extra argument.

How can we find out?  Anyone can write userspace usb gadget code.  Some
of those users may be proprietary.  Is that likely?  I don't know.  I'd
rather err on the side of not (potentially) breaking existing
applications, though.

> No generic aio code would look at res2, exactly because it is always
> zero for anything but some weird usb gadget code.

I think that no generic code looks at it because it isn't meant to be
interpreted by generic code.  :)

-Jeff

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/20/21 12:37 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 10/20/21 12:16 PM, Jeff Moyer wrote:
>>> Hi, Jens,
>>>
>>> Jens Axboe <axboe@kernel.dk> writes:
>>>
>>>> It's not used for anything, and we're wasting time passing in zeroes
>>>> where we could just ignore it instead. Update all ki_complete users in
>>>> the kernel to drop that last argument.
>>>
>>> What does "wasting time passing in zeroes" mean?
>>
>> That everybody but the funky usb gadget code passes in zero, hence it's
>> a waste of time to pass it in as an argument.
> 
> OK.  Just making sure you hadn't found some performance gain from this.
> :)

Oh there certainly is, not uncommon to see an extra register cleared and
used just for passing in this 0.

>>> We can't know whether some userspace implementation relies on this
>>> behavior, so I don't think you can change it.
>>
>> Well, I think we should find out, particularly as it's the sole user of
>> that extra argument.
> 
> How can we find out?  Anyone can write userspace usb gadget code.  Some
> of those users may be proprietary.  Is that likely?  I don't know.  I'd
> rather err on the side of not (potentially) breaking existing
> applications, though.

Yeah I don't want to risk that either. And since I can see this turning
into a discussion on what potentially would be using it, seems like the
path of less resistance...

Working on just changing it to a 64-bit type instead, then we can pass
in both at once with res2 being the upper 32 bits. That'll keep the same
API on the aio side.

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/20/21 12:41 PM, Jens Axboe wrote:
> Working on just changing it to a 64-bit type instead, then we can pass
> in both at once with res2 being the upper 32 bits. That'll keep the same
> API on the aio side.

Here's that as an incremental. Since we can only be passing in 32-bits
anyway across 32/64-bit, we can just make it an explicit 64-bit instead.
This generates the same code on 64-bit for calling ->ki_complete, and we
can trivially ignore the usb gadget issue as we now can pass in both
values (and fill them in on the aio side).

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 92b87aa8be86..66c6e0c5d638 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -550,7 +550,7 @@ static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
 		blk_mq_complete_request(rq);
 }
 
-static void lo_rw_aio_complete(struct kiocb *iocb, long ret)
+static void lo_rw_aio_complete(struct kiocb *iocb, u64 ret)
 {
 	struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb);
 
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 80a0f35ae1dc..83a2f5b0a3a0 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -123,7 +123,7 @@ static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
 	return call_iter(iocb, &iter);
 }
 
-static void nvmet_file_io_done(struct kiocb *iocb, long ret)
+static void nvmet_file_io_done(struct kiocb *iocb, u64 ret)
 {
 	struct nvmet_req *req = container_of(iocb, struct nvmet_req, f.iocb);
 	u16 status = NVME_SC_SUCCESS;
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 968ace2ddf64..c4ca7fa18e61 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -245,7 +245,7 @@ struct target_core_file_cmd {
 	struct bio_vec	bvecs[];
 };
 
-static void cmd_rw_aio_complete(struct kiocb *iocb, long ret)
+static void cmd_rw_aio_complete(struct kiocb *iocb, u64 ret)
 {
 	struct target_core_file_cmd *cmd;
 
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index e20c19a0f106..8536f19d3c9a 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, ret);
+	io_data->kiocb->ki_complete(io_data->kiocb, ((u64) ret << 32) | 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 ad1739dbfab9..d3deb23eb2ab 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, ret);
+	iocb->ki_complete(iocb, ((u64) ret << 32) | ret);
 
 	kfree(priv->buf);
 	kfree(priv->to_free);
@@ -492,14 +492,16 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
 	 * complete the aio request immediately.
 	 */
 	if (priv->to_free == NULL || unlikely(req->actual == 0)) {
+		u64 aio_ret;
+
 		kfree(req->buf);
 		kfree(priv->to_free);
 		kfree(priv);
 		iocb->private = NULL;
 		/* aio_complete() reports bytes-transferred _and_ faults */
-
-		iocb->ki_complete(iocb,
-				req->actual ? req->actual : (long)req->status);
+		aio_ret = req->actual ? req->actual : (long)req->status;
+		aio_ret |= (u64) req->status << 32;
+		iocb->ki_complete(iocb, aio_ret);
 	} else {
 		/* ep_copy_to_user() won't report both; we hide some faults */
 		if (unlikely(0 != req->status))
diff --git a/fs/aio.c b/fs/aio.c
index 836dc7e48db7..e39c61dccf37 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1417,7 +1417,7 @@ static void aio_remove_iocb(struct aio_kiocb *iocb)
 	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
 }
 
-static void aio_complete_rw(struct kiocb *kiocb, long res)
+static void aio_complete_rw(struct kiocb *kiocb, u64 res)
 {
 	struct aio_kiocb *iocb = container_of(kiocb, struct aio_kiocb, rw);
 
@@ -1436,8 +1436,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res)
 		file_end_write(kiocb->ki_filp);
 	}
 
-	iocb->ki_res.res = res;
-	iocb->ki_res.res2 = 0;
+	iocb->ki_res.res = res & 0xffffffff;
+	iocb->ki_res.res2 = res >> 32;
 	iocb_put(iocb);
 }
 
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index effe37ef8629..b2f44ff8eae2 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -37,11 +37,11 @@ static inline void cachefiles_put_kiocb(struct cachefiles_kiocb *ki)
 /*
  * Handle completion of a read from the cache.
  */
-static void cachefiles_read_complete(struct kiocb *iocb, long ret)
+static void cachefiles_read_complete(struct kiocb *iocb, u64 ret)
 {
 	struct cachefiles_kiocb *ki = container_of(iocb, struct cachefiles_kiocb, iocb);
 
-	_enter("%ld", ret);
+	_enter("%llu", (unsigned long long) ret);
 
 	if (ki->term_func) {
 		if (ret >= 0)
@@ -159,12 +159,12 @@ static int cachefiles_read(struct netfs_cache_resources *cres,
 /*
  * Handle completion of a write to the cache.
  */
-static void cachefiles_write_complete(struct kiocb *iocb, long ret)
+static void cachefiles_write_complete(struct kiocb *iocb, u64 ret)
 {
 	struct cachefiles_kiocb *ki = container_of(iocb, struct cachefiles_kiocb, iocb);
 	struct inode *inode = file_inode(ki->iocb.ki_filp);
 
-	_enter("%ld", ret);
+	_enter("%llu", (unsigned long long) ret);
 
 	/* Tell lockdep we inherited freeze protection from submission thread */
 	__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5ad046145f29..0ed6c199f394 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2672,7 +2672,7 @@ static void __io_complete_rw(struct io_kiocb *req, long res, long res2,
 	__io_req_complete(req, issue_flags, req->result, io_put_rw_kbuf(req));
 }
 
-static void io_complete_rw(struct kiocb *kiocb, long res)
+static void io_complete_rw(struct kiocb *kiocb, u64 res)
 {
 	struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw.kiocb);
 
@@ -2683,7 +2683,7 @@ static void io_complete_rw(struct kiocb *kiocb, long res)
 	io_req_task_work_add(req);
 }
 
-static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)
+static void io_complete_rw_iopoll(struct kiocb *kiocb, u64 res)
 {
 	struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw.kiocb);
 
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index ac461a499882..ff7db16aea2e 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -272,7 +272,7 @@ static void ovl_aio_cleanup_handler(struct ovl_aio_req *aio_req)
 	kmem_cache_free(ovl_aio_request_cachep, aio_req);
 }
 
-static void ovl_aio_rw_complete(struct kiocb *iocb, long res)
+static void ovl_aio_rw_complete(struct kiocb *iocb, u64 res)
 {
 	struct ovl_aio_req *aio_req = container_of(iocb,
 						   struct ovl_aio_req, iocb);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0dcb9020a7b3..3c809ce2518c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -330,7 +330,7 @@ struct kiocb {
 	randomized_struct_fields_start
 
 	loff_t			ki_pos;
-	void (*ki_complete)(struct kiocb *iocb, long ret);
+	void (*ki_complete)(struct kiocb *iocb, u64 ret);
 	void			*private;
 	int			ki_flags;
 	u16			ki_hint;

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On 10/20/21 12:41 PM, Jens Axboe wrote:
>> Working on just changing it to a 64-bit type instead, then we can pass
>> in both at once with res2 being the upper 32 bits. That'll keep the same
>> API on the aio side.
>
> Here's that as an incremental. Since we can only be passing in 32-bits
> anyway across 32/64-bit, we can just make it an explicit 64-bit instead.
> This generates the same code on 64-bit for calling ->ki_complete, and we
> can trivially ignore the usb gadget issue as we now can pass in both
> values (and fill them in on the aio side).

Yeah, I think that should work.

Cheers,
Jeff

>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 92b87aa8be86..66c6e0c5d638 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -550,7 +550,7 @@ static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
>  		blk_mq_complete_request(rq);
>  }
>  
> -static void lo_rw_aio_complete(struct kiocb *iocb, long ret)
> +static void lo_rw_aio_complete(struct kiocb *iocb, u64 ret)
>  {
>  	struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb);
>  
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 80a0f35ae1dc..83a2f5b0a3a0 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -123,7 +123,7 @@ static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
>  	return call_iter(iocb, &iter);
>  }
>  
> -static void nvmet_file_io_done(struct kiocb *iocb, long ret)
> +static void nvmet_file_io_done(struct kiocb *iocb, u64 ret)
>  {
>  	struct nvmet_req *req = container_of(iocb, struct nvmet_req, f.iocb);
>  	u16 status = NVME_SC_SUCCESS;
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 968ace2ddf64..c4ca7fa18e61 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -245,7 +245,7 @@ struct target_core_file_cmd {
>  	struct bio_vec	bvecs[];
>  };
>  
> -static void cmd_rw_aio_complete(struct kiocb *iocb, long ret)
> +static void cmd_rw_aio_complete(struct kiocb *iocb, u64 ret)
>  {
>  	struct target_core_file_cmd *cmd;
>  
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index e20c19a0f106..8536f19d3c9a 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, ret);
> +	io_data->kiocb->ki_complete(io_data->kiocb, ((u64) ret << 32) | 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 ad1739dbfab9..d3deb23eb2ab 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, ret);
> +	iocb->ki_complete(iocb, ((u64) ret << 32) | ret);
>  
>  	kfree(priv->buf);
>  	kfree(priv->to_free);
> @@ -492,14 +492,16 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
>  	 * complete the aio request immediately.
>  	 */
>  	if (priv->to_free == NULL || unlikely(req->actual == 0)) {
> +		u64 aio_ret;
> +
>  		kfree(req->buf);
>  		kfree(priv->to_free);
>  		kfree(priv);
>  		iocb->private = NULL;
>  		/* aio_complete() reports bytes-transferred _and_ faults */
> -
> -		iocb->ki_complete(iocb,
> -				req->actual ? req->actual : (long)req->status);
> +		aio_ret = req->actual ? req->actual : (long)req->status;
> +		aio_ret |= (u64) req->status << 32;
> +		iocb->ki_complete(iocb, aio_ret);
>  	} else {
>  		/* ep_copy_to_user() won't report both; we hide some faults */
>  		if (unlikely(0 != req->status))
> diff --git a/fs/aio.c b/fs/aio.c
> index 836dc7e48db7..e39c61dccf37 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1417,7 +1417,7 @@ static void aio_remove_iocb(struct aio_kiocb *iocb)
>  	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
>  }
>  
> -static void aio_complete_rw(struct kiocb *kiocb, long res)
> +static void aio_complete_rw(struct kiocb *kiocb, u64 res)
>  {
>  	struct aio_kiocb *iocb = container_of(kiocb, struct aio_kiocb, rw);
>  
> @@ -1436,8 +1436,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res)
>  		file_end_write(kiocb->ki_filp);
>  	}
>  
> -	iocb->ki_res.res = res;
> -	iocb->ki_res.res2 = 0;
> +	iocb->ki_res.res = res & 0xffffffff;
> +	iocb->ki_res.res2 = res >> 32;
>  	iocb_put(iocb);
>  }
>  
> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
> index effe37ef8629..b2f44ff8eae2 100644
> --- a/fs/cachefiles/io.c
> +++ b/fs/cachefiles/io.c
> @@ -37,11 +37,11 @@ static inline void cachefiles_put_kiocb(struct cachefiles_kiocb *ki)
>  /*
>   * Handle completion of a read from the cache.
>   */
> -static void cachefiles_read_complete(struct kiocb *iocb, long ret)
> +static void cachefiles_read_complete(struct kiocb *iocb, u64 ret)
>  {
>  	struct cachefiles_kiocb *ki = container_of(iocb, struct cachefiles_kiocb, iocb);
>  
> -	_enter("%ld", ret);
> +	_enter("%llu", (unsigned long long) ret);
>  
>  	if (ki->term_func) {
>  		if (ret >= 0)
> @@ -159,12 +159,12 @@ static int cachefiles_read(struct netfs_cache_resources *cres,
>  /*
>   * Handle completion of a write to the cache.
>   */
> -static void cachefiles_write_complete(struct kiocb *iocb, long ret)
> +static void cachefiles_write_complete(struct kiocb *iocb, u64 ret)
>  {
>  	struct cachefiles_kiocb *ki = container_of(iocb, struct cachefiles_kiocb, iocb);
>  	struct inode *inode = file_inode(ki->iocb.ki_filp);
>  
> -	_enter("%ld", ret);
> +	_enter("%llu", (unsigned long long) ret);
>  
>  	/* Tell lockdep we inherited freeze protection from submission thread */
>  	__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 5ad046145f29..0ed6c199f394 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2672,7 +2672,7 @@ static void __io_complete_rw(struct io_kiocb *req, long res, long res2,
>  	__io_req_complete(req, issue_flags, req->result, io_put_rw_kbuf(req));
>  }
>  
> -static void io_complete_rw(struct kiocb *kiocb, long res)
> +static void io_complete_rw(struct kiocb *kiocb, u64 res)
>  {
>  	struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw.kiocb);
>  
> @@ -2683,7 +2683,7 @@ static void io_complete_rw(struct kiocb *kiocb, long res)
>  	io_req_task_work_add(req);
>  }
>  
> -static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)
> +static void io_complete_rw_iopoll(struct kiocb *kiocb, u64 res)
>  {
>  	struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw.kiocb);
>  
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index ac461a499882..ff7db16aea2e 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -272,7 +272,7 @@ static void ovl_aio_cleanup_handler(struct ovl_aio_req *aio_req)
>  	kmem_cache_free(ovl_aio_request_cachep, aio_req);
>  }
>  
> -static void ovl_aio_rw_complete(struct kiocb *iocb, long res)
> +static void ovl_aio_rw_complete(struct kiocb *iocb, u64 res)
>  {
>  	struct ovl_aio_req *aio_req = container_of(iocb,
>  						   struct ovl_aio_req, iocb);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 0dcb9020a7b3..3c809ce2518c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -330,7 +330,7 @@ struct kiocb {
>  	randomized_struct_fields_start
>  
>  	loff_t			ki_pos;
> -	void (*ki_complete)(struct kiocb *iocb, long ret);
> +	void (*ki_complete)(struct kiocb *iocb, u64 ret);
>  	void			*private;
>  	int			ki_flags;
>  	u16			ki_hint;

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/20/21 1:11 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 10/20/21 12:41 PM, Jens Axboe wrote:
>>> Working on just changing it to a 64-bit type instead, then we can pass
>>> in both at once with res2 being the upper 32 bits. That'll keep the same
>>> API on the aio side.
>>
>> Here's that as an incremental. Since we can only be passing in 32-bits
>> anyway across 32/64-bit, we can just make it an explicit 64-bit instead.
>> This generates the same code on 64-bit for calling ->ki_complete, and we
>> can trivially ignore the usb gadget issue as we now can pass in both
>> values (and fill them in on the aio side).
> 
> Yeah, I think that should work.

Passed test and allmodconfig sanity check, sent out as v2 :)

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On 10/20/21 1:11 PM, Jeff Moyer wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>> 
>>> On 10/20/21 12:41 PM, Jens Axboe wrote:
>>>> Working on just changing it to a 64-bit type instead, then we can pass
>>>> in both at once with res2 being the upper 32 bits. That'll keep the same
>>>> API on the aio side.
>>>
>>> Here's that as an incremental. Since we can only be passing in 32-bits
>>> anyway across 32/64-bit, we can just make it an explicit 64-bit instead.
>>> This generates the same code on 64-bit for calling ->ki_complete, and we
>>> can trivially ignore the usb gadget issue as we now can pass in both
>>> values (and fill them in on the aio side).
>> 
>> Yeah, I think that should work.
>
> Passed test and allmodconfig sanity check, sent out as v2 :)

It passed the libaio tests on x64.  I'll do some more testing and review
the v2 posting.

Thanks!
Jeff

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/20/21 1:47 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 10/20/21 1:11 PM, Jeff Moyer wrote:
>>> Jens Axboe <axboe@kernel.dk> writes:
>>>
>>>> On 10/20/21 12:41 PM, Jens Axboe wrote:
>>>>> Working on just changing it to a 64-bit type instead, then we can pass
>>>>> in both at once with res2 being the upper 32 bits. That'll keep the same
>>>>> API on the aio side.
>>>>
>>>> Here's that as an incremental. Since we can only be passing in 32-bits
>>>> anyway across 32/64-bit, we can just make it an explicit 64-bit instead.
>>>> This generates the same code on 64-bit for calling ->ki_complete, and we
>>>> can trivially ignore the usb gadget issue as we now can pass in both
>>>> values (and fill them in on the aio side).
>>>
>>> Yeah, I think that should work.
>>
>> Passed test and allmodconfig sanity check, sent out as v2 :)
> 
> It passed the libaio tests on x64.  I'll do some more testing and review
> the v2 posting.

Thanks Jeff!

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[John Keeping]

On Wed, 20 Oct 2021 19:49:07 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Oct 20, 2021 at 11:35:27AM -0600, Jens Axboe wrote:
> > On 10/20/21 11:30 AM, Christoph Hellwig wrote:  
> > > On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:  
> > >> It's not used for anything, and we're wasting time passing in zeroes
> > >> where we could just ignore it instead. Update all ki_complete users in
> > >> the kernel to drop that last argument.
> > >>
> > >> The exception is the USB gadget code, which passes in non-zero. But
> > >> since nobody every looks at ret2, it's still pointless.  
> > > 
> > > Yes, the USB gadget passes non-zero, and aio passes that on to
> > > userspace.  So this is an ABI change.  Does it actually matter?
> > > I don't know, but you could CC the relevant maintainers and list
> > > to try to figure that out.  
> > 
> > True, guess it does go out to userspace. Greg, is anyone using
> > it on the userspace side?  
> 
> I really do not know (adding linux-usb@vger)  My interactions with the
> gadget code have not been through the aio api, thankfully :)
> 
> Odds are it's fine, I think that something had to be passed in there so
> that was chosen?  If the aio code didn't do anything with it, I can't
> see where the gadget code gets it back at anywhere, but I might be
> looking in the wrong place.
> 
> Anyone else here know?

I really doubt anyone uses io_event::res2 with FunctionFS gadgets.  The
examples in tools/usb/ffs-aio-example/ either check just "res" or ignore
the status completely.

The only other program I can find using aio FunctionFS is adbd which
also checks res and ignores res2 [1].  Other examples I know of just use
synchronous I/O.

[1] https://github.com/aosp-mirror/platform_system_core/blob/34a0e57a257f0081c672c9be0e87230762e677ca/adb/daemon/usb.cpp#L527

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/21/21 10:40 AM, John Keeping wrote:
> On Wed, 20 Oct 2021 19:49:07 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
>> On Wed, Oct 20, 2021 at 11:35:27AM -0600, Jens Axboe wrote:
>>> On 10/20/21 11:30 AM, Christoph Hellwig wrote:  
>>>> On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:  
>>>>> It's not used for anything, and we're wasting time passing in zeroes
>>>>> where we could just ignore it instead. Update all ki_complete users in
>>>>> the kernel to drop that last argument.
>>>>>
>>>>> The exception is the USB gadget code, which passes in non-zero. But
>>>>> since nobody every looks at ret2, it's still pointless.  
>>>>
>>>> Yes, the USB gadget passes non-zero, and aio passes that on to
>>>> userspace.  So this is an ABI change.  Does it actually matter?
>>>> I don't know, but you could CC the relevant maintainers and list
>>>> to try to figure that out.  
>>>
>>> True, guess it does go out to userspace. Greg, is anyone using
>>> it on the userspace side?  
>>
>> I really do not know (adding linux-usb@vger)  My interactions with the
>> gadget code have not been through the aio api, thankfully :)
>>
>> Odds are it's fine, I think that something had to be passed in there so
>> that was chosen?  If the aio code didn't do anything with it, I can't
>> see where the gadget code gets it back at anywhere, but I might be
>> looking in the wrong place.
>>
>> Anyone else here know?
> 
> I really doubt anyone uses io_event::res2 with FunctionFS gadgets.  The
> examples in tools/usb/ffs-aio-example/ either check just "res" or ignore
> the status completely.
> 
> The only other program I can find using aio FunctionFS is adbd which
> also checks res and ignores res2 [1].  Other examples I know of just use
> synchronous I/O.

We might consider doing a separate change to just skip status reporting,
and then once a few releases have passed and if nobody has complained,
then we can drop it entirely. If we do it separately, it'd be an easy
revert of just that part if we do run into trouble.

At least for now we'll just have the one argument bundling the two parts
for aio, so there's not a great need to eliminate the status thing on
the USB gadget side. But if it is unused, then it should get removed
regardless.

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/21/21 10:40 AM, John Keeping wrote:
> On Wed, 20 Oct 2021 19:49:07 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
>> On Wed, Oct 20, 2021 at 11:35:27AM -0600, Jens Axboe wrote:
>>> On 10/20/21 11:30 AM, Christoph Hellwig wrote:  
>>>> On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:  
>>>>> It's not used for anything, and we're wasting time passing in zeroes
>>>>> where we could just ignore it instead. Update all ki_complete users in
>>>>> the kernel to drop that last argument.
>>>>>
>>>>> The exception is the USB gadget code, which passes in non-zero. But
>>>>> since nobody every looks at ret2, it's still pointless.  
>>>>
>>>> Yes, the USB gadget passes non-zero, and aio passes that on to
>>>> userspace.  So this is an ABI change.  Does it actually matter?
>>>> I don't know, but you could CC the relevant maintainers and list
>>>> to try to figure that out.  
>>>
>>> True, guess it does go out to userspace. Greg, is anyone using
>>> it on the userspace side?  
>>
>> I really do not know (adding linux-usb@vger)  My interactions with the
>> gadget code have not been through the aio api, thankfully :)
>>
>> Odds are it's fine, I think that something had to be passed in there so
>> that was chosen?  If the aio code didn't do anything with it, I can't
>> see where the gadget code gets it back at anywhere, but I might be
>> looking in the wrong place.
>>
>> Anyone else here know?
> 
> I really doubt anyone uses io_event::res2 with FunctionFS gadgets.  The
> examples in tools/usb/ffs-aio-example/ either check just "res" or ignore
> the status completely.
> 
> The only other program I can find using aio FunctionFS is adbd which
> also checks res and ignores res2 [1].  Other examples I know of just use
> synchronous I/O.

So is there consensus on the USB side that we can just fill res2 with
zero? The single cases that does just do res == res2 puts the error
in res anyway, which is what you'd expect.

If so, then I do think that'd be cleaner than packing two values into
a u64.

-- 
Jens Axboe

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Greg Kroah-Hartman]

On Fri, Oct 22, 2021 at 09:44:32AM -0600, Jens Axboe wrote:
> On 10/21/21 10:40 AM, John Keeping wrote:
> > On Wed, 20 Oct 2021 19:49:07 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> >> On Wed, Oct 20, 2021 at 11:35:27AM -0600, Jens Axboe wrote:
> >>> On 10/20/21 11:30 AM, Christoph Hellwig wrote:  
> >>>> On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:  
> >>>>> It's not used for anything, and we're wasting time passing in zeroes
> >>>>> where we could just ignore it instead. Update all ki_complete users in
> >>>>> the kernel to drop that last argument.
> >>>>>
> >>>>> The exception is the USB gadget code, which passes in non-zero. But
> >>>>> since nobody every looks at ret2, it's still pointless.  
> >>>>
> >>>> Yes, the USB gadget passes non-zero, and aio passes that on to
> >>>> userspace.  So this is an ABI change.  Does it actually matter?
> >>>> I don't know, but you could CC the relevant maintainers and list
> >>>> to try to figure that out.  
> >>>
> >>> True, guess it does go out to userspace. Greg, is anyone using
> >>> it on the userspace side?  
> >>
> >> I really do not know (adding linux-usb@vger)  My interactions with the
> >> gadget code have not been through the aio api, thankfully :)
> >>
> >> Odds are it's fine, I think that something had to be passed in there so
> >> that was chosen?  If the aio code didn't do anything with it, I can't
> >> see where the gadget code gets it back at anywhere, but I might be
> >> looking in the wrong place.
> >>
> >> Anyone else here know?
> > 
> > I really doubt anyone uses io_event::res2 with FunctionFS gadgets.  The
> > examples in tools/usb/ffs-aio-example/ either check just "res" or ignore
> > the status completely.
> > 
> > The only other program I can find using aio FunctionFS is adbd which
> > also checks res and ignores res2 [1].  Other examples I know of just use
> > synchronous I/O.
> 
> So is there consensus on the USB side that we can just fill res2 with
> zero? The single cases that does just do res == res2 puts the error
> in res anyway, which is what you'd expect.
> 
> If so, then I do think that'd be cleaner than packing two values into
> a u64.

I think yes, we should try that, and if something breaks, be ready to
provide a fix for it.

thanks,

greg k-h

Re: [PATCH] fs: kill unused ret2 argument from iocb->ki_complete()

[Jens Axboe]

On 10/23/21 3:09 AM, Greg Kroah-Hartman wrote:
> On Fri, Oct 22, 2021 at 09:44:32AM -0600, Jens Axboe wrote:
>> On 10/21/21 10:40 AM, John Keeping wrote:
>>> On Wed, 20 Oct 2021 19:49:07 +0200
>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>>
>>>> On Wed, Oct 20, 2021 at 11:35:27AM -0600, Jens Axboe wrote:
>>>>> On 10/20/21 11:30 AM, Christoph Hellwig wrote:  
>>>>>> On Wed, Oct 20, 2021 at 10:49:07AM -0600, Jens Axboe wrote:  
>>>>>>> It's not used for anything, and we're wasting time passing in zeroes
>>>>>>> where we could just ignore it instead. Update all ki_complete users in
>>>>>>> the kernel to drop that last argument.
>>>>>>>
>>>>>>> The exception is the USB gadget code, which passes in non-zero. But
>>>>>>> since nobody every looks at ret2, it's still pointless.  
>>>>>>
>>>>>> Yes, the USB gadget passes non-zero, and aio passes that on to
>>>>>> userspace.  So this is an ABI change.  Does it actually matter?
>>>>>> I don't know, but you could CC the relevant maintainers and list
>>>>>> to try to figure that out.  
>>>>>
>>>>> True, guess it does go out to userspace. Greg, is anyone using
>>>>> it on the userspace side?  
>>>>
>>>> I really do not know (adding linux-usb@vger)  My interactions with the
>>>> gadget code have not been through the aio api, thankfully :)
>>>>
>>>> Odds are it's fine, I think that something had to be passed in there so
>>>> that was chosen?  If the aio code didn't do anything with it, I can't
>>>> see where the gadget code gets it back at anywhere, but I might be
>>>> looking in the wrong place.
>>>>
>>>> Anyone else here know?
>>>
>>> I really doubt anyone uses io_event::res2 with FunctionFS gadgets.  The
>>> examples in tools/usb/ffs-aio-example/ either check just "res" or ignore
>>> the status completely.
>>>
>>> The only other program I can find using aio FunctionFS is adbd which
>>> also checks res and ignores res2 [1].  Other examples I know of just use
>>> synchronous I/O.
>>
>> So is there consensus on the USB side that we can just fill res2 with
>> zero? The single cases that does just do res == res2 puts the error
>> in res anyway, which is what you'd expect.
>>
>> If so, then I do think that'd be cleaner than packing two values into
>> a u64.
> 
> I think yes, we should try that, and if something breaks, be ready to
> provide a fix for it.

I've split the change in two, one that gets rid of the special arguments
on the USB side, and one that then drops it for the whole kernel since
everybody passes 0 at that point. Should make it easy to pinpoint, if
need be.

-- 
Jens Axboe