Re: [RFC PATCH] blk-core: remove blk_put_request()

Re: [RFC PATCH] blk-core: remove blk_put_request()

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 2511 bytes --]

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 <chaitanya.kulkarni@wdc.com>
> ---
>  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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH] blk-core: remove blk_put_request()

[Jens Axboe]

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 <chaitanya.kulkarni@wdc.com>
>> ---
>>  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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH] blk-core: remove blk_put_request()

[Christoph Hellwig]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization