Re: [PATCH 3/8] scsi: remove the cmd field from struct scsi_request

From: John Garry <john.garry@huawei.com>
To: Christoph Hellwig <hch@lst.de>,
	"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: <linux-scsi@vger.kernel.org>, <target-devel@vger.kernel.org>,
	"Bart Van Assche" <bvanassche@acm.org>
Subject: Re: [PATCH 3/8] scsi: remove the cmd field from struct scsi_request
Date: Thu, 24 Feb 2022 18:19:13 +0000	[thread overview]
Message-ID: <1231b97e-88aa-1e1b-4fec-9728ce066284@huawei.com> (raw)
In-Reply-To: <20220224175552.988286-4-hch@lst.de>

On 24/02/2022 17:55, Christoph Hellwig wrote:
> Now that each scsi_request is backed by a scsi_cmnd, there is no
> need to indirect the CDB storage.  Change all submitters of SCSI
> passthrough requests to store the CDB information directly in the
> scsi_cmnd, and while doing so allocate the full 32 bytes that cover
> all Linux supported SCSI hosts instead of requiring dynamic allocation
> for > 16 byte CDBs.  On 64-bit systems this does not change the size of
> the scsi_cmnd at all, while on 32-bit systems it slightly increases it
> for now, but that increase will be made up by the removal of the
> remaining scsi_request fields.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

I had just finished going through the v1 of this patch and this seems 
identical, so, FWIW:

Reviewed-by: John Garry <john.garry@huawei.com>

Just a minor comment below (feel free to ignore).

> ---
>   drivers/ata/libata-scsi.c           |  4 +++-
>   drivers/block/pktcdvd.c             |  6 +++--
>   drivers/scsi/scsi_bsg.c             | 23 +++++++++---------
>   drivers/scsi/scsi_debugfs.c         |  4 +---
>   drivers/scsi/scsi_error.c           | 28 +++++++++++-----------
>   drivers/scsi/scsi_ioctl.c           | 37 +++++++++++++----------------
>   drivers/scsi/scsi_lib.c             | 19 +++++++--------
>   drivers/scsi/scsi_logging.c         |  5 +---
>   drivers/scsi/sd.c                   | 28 +---------------------
>   drivers/scsi/sg.c                   | 30 +++++++++--------------
>   drivers/scsi/sr.c                   | 24 ++++++++++---------
>   drivers/scsi/st.c                   | 12 ++++++----
>   drivers/scsi/ufs/ufshpb.c           | 22 ++++++++---------
>   drivers/target/target_core_pscsi.c  | 12 ++++++----
>   drivers/usb/storage/cypress_atacb.c |  1 -
>   drivers/usb/storage/isd200.c        |  4 ++--
>   include/scsi/scsi_cmnd.h            |  7 +-----
>   include/scsi/scsi_eh.h              |  4 +---
>   include/scsi/scsi_request.h         | 11 ---------
>   19 files changed, 113 insertions(+), 168 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index ed8be585a98f7..c8283bd4ffd15 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1022,7 +1022,9 @@ void ata_scsi_sdev_config(struct scsi_device *sdev)
>    */
>   bool ata_scsi_dma_need_drain(struct request *rq)
>   {
> -	return atapi_cmd_type(scsi_req(rq)->cmd[0]) == ATAPI_MISC;
> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
> +
> +	return atapi_cmd_type(scmd->cmnd[0]) == ATAPI_MISC;
>   }
>   EXPORT_SYMBOL_GPL(ata_scsi_dma_need_drain);
>   
> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index 2b6b70a39e760..42c284b2d7f93 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -693,6 +693,7 @@ static void pkt_rbtree_insert(struct pktcdvd_device *pd, struct pkt_rb_node *nod
>   static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *cgc)
>   {
>   	struct request_queue *q = bdev_get_queue(pd->bdev);
> +	struct scsi_cmnd *scmd;
>   	struct request *rq;
>   	int ret = 0;
>   
> @@ -700,6 +701,7 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
>   			     REQ_OP_DRV_OUT : REQ_OP_DRV_IN, 0);
>   	if (IS_ERR(rq))
>   		return PTR_ERR(rq);
> +	scmd = blk_mq_rq_to_pdu(rq);
>   
>   	if (cgc->buflen) {
>   		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
> @@ -708,8 +710,8 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
>   			goto out;
>   	}
>   
> -	scsi_req(rq)->cmd_len = COMMAND_SIZE(cgc->cmd[0]);
> -	memcpy(scsi_req(rq)->cmd, cgc->cmd, CDROM_PACKET_SIZE);
> +	scmd->cmd_len = COMMAND_SIZE(cgc->cmd[0]);
> +	memcpy(scmd->cmnd, cgc->cmd, CDROM_PACKET_SIZE);
>   
>   	rq->timeout = 60*HZ;
>   	if (cgc->quiet)
> diff --git a/drivers/scsi/scsi_bsg.c b/drivers/scsi/scsi_bsg.c
> index b7a464383cc0b..f8b65bd75ee10 100644
> --- a/drivers/scsi/scsi_bsg.c
> +++ b/drivers/scsi/scsi_bsg.c
> @@ -13,6 +13,7 @@ static int scsi_bsg_sg_io_fn(struct request_queue *q, struct sg_io_v4 *hdr,
>   		fmode_t mode, unsigned int timeout)
>   {
>   	struct scsi_request *sreq;
> +	struct scsi_cmnd *scmd;
>   	struct request *rq;
>   	struct bio *bio;
>   	int ret;
> @@ -33,19 +34,19 @@ static int scsi_bsg_sg_io_fn(struct request_queue *q, struct sg_io_v4 *hdr,
>   
>   	ret = -ENOMEM;
>   	sreq = scsi_req(rq);
> -	sreq->cmd_len = hdr->request_len;
> -	if (sreq->cmd_len > BLK_MAX_CDB) {
> -		sreq->cmd = kzalloc(sreq->cmd_len, GFP_KERNEL);
> -		if (!sreq->cmd)
> -			goto out_put_request;
> +	scmd = blk_mq_rq_to_pdu(rq);
> +	scmd->cmd_len = hdr->request_len;
> +	if (scmd->cmd_len > sizeof(scmd->cmnd)) {

You might be able to move this check earlier (before the request 
allocation), but no big deal.