Re: [RFC PATCH] blk-core: remove blk_put_request()
On 2/24/21 10:56, Christoph Hellwig wrote: > On Wed, Feb 24, 2021 at 09:48:21AM -0700, Jens Axboe wrote: >> Would make sense to rename blk_get_request() to blk_mq_alloc_request() >> and then we have API symmetry. The get/put don't make sense when there >> are no references involved. >> >> But it's a lot of churn for very little reward, which is always kind >> of annoying. Especially for the person that has to carry the patches. > Let's do the following: > > - move the initialize_rq_fn call from blk_get_request into >blk_mq_alloc_request and make the former a trivial alias for the >latter > - migrate to the blk_mq_* versions on a per-driver/subsystem basis. >The scsi migration depends on the first item above, so it will have >to go with that or wait for the next merge window > - don't migrate the legacy ide driver, as it is about to be removed and >has a huge number of blk_get_request calls > Okay, thanks for the feedback, will try and get something together.
Re: [RFC PATCH] blk-core: remove blk_put_request()
On Wed, Feb 24, 2021 at 09:48:21AM -0700, Jens Axboe wrote: > Would make sense to rename blk_get_request() to blk_mq_alloc_request() > and then we have API symmetry. The get/put don't make sense when there > are no references involved. > > But it's a lot of churn for very little reward, which is always kind > of annoying. Especially for the person that has to carry the patches. Let's do the following: - move the initialize_rq_fn call from blk_get_request into blk_mq_alloc_request and make the former a trivial alias for the latter - migrate to the blk_mq_* versions on a per-driver/subsystem basis. The scsi migration depends on the first item above, so it will have to go with that or wait for the next merge window - don't migrate the legacy ide driver, as it is about to be removed and has a huge number of blk_get_request calls
Re: [RFC PATCH] blk-core: remove blk_put_request()
On 2/24/21 4:53 AM, Stefan Hajnoczi wrote:
> On Mon, Feb 22, 2021 at 01:11:15PM -0800, Chaitanya Kulkarni wrote:
>> The function blk_put_request() is just a wrapper to
>> blk_mq_free_request(), remove the unnecessary wrapper.
>>
>> Any feedback is welcome on this RFC.
>>
>> Signed-off-by: Chaitanya Kulkarni
>> ---
>> block/blk-core.c | 6 --
>> block/blk-merge.c | 2 +-
>> block/bsg-lib.c| 4 ++--
>> block/bsg.c| 4 ++--
>> block/scsi_ioctl.c | 6 +++---
>> drivers/block/paride/pd.c | 2 +-
>> drivers/block/pktcdvd.c| 2 +-
>> drivers/block/virtio_blk.c | 2 +-
>> drivers/cdrom/cdrom.c | 4 ++--
>> drivers/ide/ide-atapi.c| 2 +-
>> drivers/ide/ide-cd.c | 4 ++--
>> drivers/ide/ide-cd_ioctl.c | 2 +-
>> drivers/ide/ide-devsets.c | 2 +-
>> drivers/ide/ide-disk.c | 2 +-
>> drivers/ide/ide-ioctls.c | 4 ++--
>> drivers/ide/ide-park.c | 2 +-
>> drivers/ide/ide-pm.c | 4 ++--
>> drivers/ide/ide-tape.c | 2 +-
>> drivers/ide/ide-taskfile.c | 2 +-
>> drivers/md/dm-mpath.c | 2 +-
>> drivers/mmc/core/block.c | 10 +-
>> drivers/scsi/scsi_error.c | 2 +-
>> drivers/scsi/scsi_lib.c| 2 +-
>> drivers/scsi/sg.c | 6 +++---
>> drivers/scsi/st.c | 4 ++--
>> drivers/scsi/ufs/ufshcd.c | 6 +++---
>> drivers/target/target_core_pscsi.c | 4 ++--
>> fs/nfsd/blocklayout.c | 4 ++--
>> include/linux/blkdev.h | 1 -
>> 29 files changed, 46 insertions(+), 53 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index fc60ff208497..1754f5e7cc80 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -642,12 +642,6 @@ struct request *blk_get_request(struct request_queue
>> *q, unsigned int op,
>> }
>> EXPORT_SYMBOL(blk_get_request);
>>
>> -void blk_put_request(struct request *req)
>> -{
>> -blk_mq_free_request(req);
>> -}
>> -EXPORT_SYMBOL(blk_put_request);
>
> blk_get_request() still exists after this patch. A "get" API usually has
> a corresponding "put" API. I'm not sure this patch helps the consistency
> and clarity of the code.
>
> If you do go ahead, please update the blk_get_request() doc comment
> explicitly mentioning that blk_mq_free_request() needs to be called.
Would make sense to rename blk_get_request() to blk_mq_alloc_request()
and then we have API symmetry. The get/put don't make sense when there
are no references involved.
But it's a lot of churn for very little reward, which is always kind
of annoying. Especially for the person that has to carry the patches.
--
Jens Axboe
Re: [RFC PATCH] blk-core: remove blk_put_request()
On Mon, Feb 22, 2021 at 01:11:15PM -0800, Chaitanya Kulkarni wrote:
> The function blk_put_request() is just a wrapper to
> blk_mq_free_request(), remove the unnecessary wrapper.
>
> Any feedback is welcome on this RFC.
>
> Signed-off-by: Chaitanya Kulkarni
> ---
> block/blk-core.c | 6 --
> block/blk-merge.c | 2 +-
> block/bsg-lib.c| 4 ++--
> block/bsg.c| 4 ++--
> block/scsi_ioctl.c | 6 +++---
> drivers/block/paride/pd.c | 2 +-
> drivers/block/pktcdvd.c| 2 +-
> drivers/block/virtio_blk.c | 2 +-
> drivers/cdrom/cdrom.c | 4 ++--
> drivers/ide/ide-atapi.c| 2 +-
> drivers/ide/ide-cd.c | 4 ++--
> drivers/ide/ide-cd_ioctl.c | 2 +-
> drivers/ide/ide-devsets.c | 2 +-
> drivers/ide/ide-disk.c | 2 +-
> drivers/ide/ide-ioctls.c | 4 ++--
> drivers/ide/ide-park.c | 2 +-
> drivers/ide/ide-pm.c | 4 ++--
> drivers/ide/ide-tape.c | 2 +-
> drivers/ide/ide-taskfile.c | 2 +-
> drivers/md/dm-mpath.c | 2 +-
> drivers/mmc/core/block.c | 10 +-
> drivers/scsi/scsi_error.c | 2 +-
> drivers/scsi/scsi_lib.c| 2 +-
> drivers/scsi/sg.c | 6 +++---
> drivers/scsi/st.c | 4 ++--
> drivers/scsi/ufs/ufshcd.c | 6 +++---
> drivers/target/target_core_pscsi.c | 4 ++--
> fs/nfsd/blocklayout.c | 4 ++--
> include/linux/blkdev.h | 1 -
> 29 files changed, 46 insertions(+), 53 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index fc60ff208497..1754f5e7cc80 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -642,12 +642,6 @@ struct request *blk_get_request(struct request_queue *q,
> unsigned int op,
> }
> EXPORT_SYMBOL(blk_get_request);
>
> -void blk_put_request(struct request *req)
> -{
> - blk_mq_free_request(req);
> -}
> -EXPORT_SYMBOL(blk_put_request);
blk_get_request() still exists after this patch. A "get" API usually has
a corresponding "put" API. I'm not sure this patch helps the consistency
and clarity of the code.
If you do go ahead, please update the blk_get_request() doc comment
explicitly mentioning that blk_mq_free_request() needs to be called.
Stefan
signature.asc
Description: PGP signature
[RFC PATCH] blk-core: remove blk_put_request()
The function blk_put_request() is just a wrapper to
blk_mq_free_request(), remove the unnecessary wrapper.
Any feedback is welcome on this RFC.
Signed-off-by: Chaitanya Kulkarni
---
block/blk-core.c | 6 --
block/blk-merge.c | 2 +-
block/bsg-lib.c| 4 ++--
block/bsg.c| 4 ++--
block/scsi_ioctl.c | 6 +++---
drivers/block/paride/pd.c | 2 +-
drivers/block/pktcdvd.c| 2 +-
drivers/block/virtio_blk.c | 2 +-
drivers/cdrom/cdrom.c | 4 ++--
drivers/ide/ide-atapi.c| 2 +-
drivers/ide/ide-cd.c | 4 ++--
drivers/ide/ide-cd_ioctl.c | 2 +-
drivers/ide/ide-devsets.c | 2 +-
drivers/ide/ide-disk.c | 2 +-
drivers/ide/ide-ioctls.c | 4 ++--
drivers/ide/ide-park.c | 2 +-
drivers/ide/ide-pm.c | 4 ++--
drivers/ide/ide-tape.c | 2 +-
drivers/ide/ide-taskfile.c | 2 +-
drivers/md/dm-mpath.c | 2 +-
drivers/mmc/core/block.c | 10 +-
drivers/scsi/scsi_error.c | 2 +-
drivers/scsi/scsi_lib.c| 2 +-
drivers/scsi/sg.c | 6 +++---
drivers/scsi/st.c | 4 ++--
drivers/scsi/ufs/ufshcd.c | 6 +++---
drivers/target/target_core_pscsi.c | 4 ++--
fs/nfsd/blocklayout.c | 4 ++--
include/linux/blkdev.h | 1 -
29 files changed, 46 insertions(+), 53 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index fc60ff208497..1754f5e7cc80 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -642,12 +642,6 @@ struct request *blk_get_request(struct request_queue *q,
unsigned int op,
}
EXPORT_SYMBOL(blk_get_request);
-void blk_put_request(struct request *req)
-{
- blk_mq_free_request(req);
-}
-EXPORT_SYMBOL(blk_put_request);
-
static void handle_bad_sector(struct bio *bio, sector_t maxsector)
{
char b[BDEVNAME_SIZE];
diff --git a/block/blk-merge.c b/block/blk-merge.c
index ffb4aa0ea68b..f3493ee243fd 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -845,7 +845,7 @@ int blk_attempt_req_merge(struct request_queue *q, struct
request *rq,
free = attempt_merge(q, rq, next);
if (free) {
- blk_put_request(free);
+ blk_mq_free_request(free);
return 1;
}
diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index 330fede77271..8ea2b33783fb 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -67,7 +67,7 @@ static int bsg_transport_fill_hdr(struct request *rq, struct
sg_io_v4 *hdr,
out_free_bidi_rq:
if (job->bidi_rq)
- blk_put_request(job->bidi_rq);
+ blk_mq_free_request(job->bidi_rq);
out:
kfree(job->request);
return ret;
@@ -128,7 +128,7 @@ static void bsg_transport_free_rq(struct request *rq)
if (job->bidi_rq) {
blk_rq_unmap_user(job->bidi_bio);
- blk_put_request(job->bidi_rq);
+ blk_mq_free_request(job->bidi_rq);
}
kfree(job->request);
diff --git a/block/bsg.c b/block/bsg.c
index bd10922d5cbb..4ddc84aebecf 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -158,7 +158,7 @@ static int bsg_sg_io(struct request_queue *q, fmode_t mode,
void __user *uarg)
ret = q->bsg_dev.ops->fill_hdr(rq, , mode);
if (ret) {
- blk_put_request(rq);
+ blk_mq_free_request(rq);
return ret;
}
@@ -189,7 +189,7 @@ static int bsg_sg_io(struct request_queue *q, fmode_t mode,
void __user *uarg)
out_free_rq:
rq->q->bsg_dev.ops->free_rq(rq);
- blk_put_request(rq);
+ blk_mq_free_request(rq);
if (!ret && copy_to_user(uarg, , sizeof(hdr)))
return -EFAULT;
return ret;
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 6599bac0a78c..52cd3fd924fc 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -366,7 +366,7 @@ static int sg_io(struct request_queue *q, struct gendisk
*bd_disk,
out_free_cdb:
scsi_req_free_cmd(req);
out_put_request:
- blk_put_request(rq);
+ blk_mq_free_request(rq);
return ret;
}
@@ -509,7 +509,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk
*disk, fmode_t mode,
}
error:
- blk_put_request(rq);
+ blk_mq_free_request(rq);
error_free_buffer:
kfree(buffer);
@@ -534,7 +534,7 @@ static int __blk_send_generic(struct request_queue *q,
struct gendisk *bd_disk,
scsi_req(rq)->cmd_len = 6;
blk_execute_rq(bd_disk, rq, 0);
err = scsi_req(rq)->result ? -EIO : 0;
- blk_put_request(rq);
+ blk_mq_free_request(rq);
return err;
}
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index 897acda20ac8..381fc0cf0b35 100644
--- a/drivers/block/paride/pd.c
+++
