RFC: clean up command setup

RFC: clean up command setup

[Christoph Hellwig]

There has been some confusion because scsi_setup_blk_pc_cmnd is used to set
up various special TYPE_FS commands.  I've looked into the area and come
up with various cleanup, as well as few small bug fixes that were turned
up by this.

The series is against the core-for-3.17 branch of my scsi-queue tree.

[PATCH 01/10] scsi: move the nr_phys_segments assert into scsi_init_io

[Christoph Hellwig]

scsi_init_io should only be called for requests that transfer data,
so move the assert that a request has segments from the callers into
scsi_init_io.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c |   16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 37038fb..b505b06 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -996,8 +996,11 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
 	struct scsi_device *sdev = cmd->device;
 	struct request *rq = cmd->request;
+	int error;
 
-	int error = scsi_init_sgtable(rq, &cmd->sdb, gfp_mask);
+	BUG_ON(!rq->nr_phys_segments);
+
+	error = scsi_init_sgtable(rq, &cmd->sdb, gfp_mask);
 	if (error)
 		goto err_exit;
 
@@ -1088,11 +1091,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 	 * submit a request without an attached bio.
 	 */
 	if (req->bio) {
-		int ret;
-
-		BUG_ON(!req->nr_phys_segments);
-
-		ret = scsi_init_io(cmd, GFP_ATOMIC);
+		int ret = scsi_init_io(cmd, GFP_ATOMIC);
 		if (unlikely(ret))
 			return ret;
 	} else {
@@ -1131,11 +1130,6 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 			return ret;
 	}
 
-	/*
-	 * Filesystem requests must transfer data.
-	 */
-	BUG_ON(!req->nr_phys_segments);
-
 	memset(cmd->cmnd, 0, BLK_MAX_CDB);
 	return scsi_init_io(cmd, GFP_ATOMIC);
 }
-- 
1.7.10.4

Re: [PATCH 01/10] scsi: move the nr_phys_segments assert into scsi_init_io

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> scsi_init_io should only be called for requests that transfer data,
> so move the assert that a request has segments from the callers into
> scsi_init_io.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c |   16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 37038fb..b505b06 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -996,8 +996,11 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
>   {
>   	struct scsi_device *sdev = cmd->device;
>   	struct request *rq = cmd->request;
> +	int error;
>
> -	int error = scsi_init_sgtable(rq, &cmd->sdb, gfp_mask);
> +	BUG_ON(!rq->nr_phys_segments);
> +
> +	error = scsi_init_sgtable(rq, &cmd->sdb, gfp_mask);
>   	if (error)
>   		goto err_exit;
>
> @@ -1088,11 +1091,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>   	 * submit a request without an attached bio.
>   	 */
>   	if (req->bio) {
> -		int ret;
> -
> -		BUG_ON(!req->nr_phys_segments);
> -
> -		ret = scsi_init_io(cmd, GFP_ATOMIC);
> +		int ret = scsi_init_io(cmd, GFP_ATOMIC);
>   		if (unlikely(ret))
>   			return ret;
>   	} else {
> @@ -1131,11 +1130,6 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>   			return ret;
>   	}
>
> -	/*
> -	 * Filesystem requests must transfer data.
> -	 */
> -	BUG_ON(!req->nr_phys_segments);
> -
>   	memset(cmd->cmnd, 0, BLK_MAX_CDB);
>   	return scsi_init_io(cmd, GFP_ATOMIC);
>   }
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] scsi: move the nr_phys_segments assert into scsi_init_io

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> scsi_init_io should only be called for requests that transfer
Christoph> data, so move the assert that a request has segments from the
Christoph> callers into scsi_init_io.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 02/10] scsi: restructure command initialization for TYPE_FS requests

[Christoph Hellwig]

We should call the device handler prep_fn for all TYPE_FS requests,
not just simple read/write calls that are handled by the disk driver.

Restructure the common I/O code to call the prep_fn handler and zero
out the CDB, and just leave the call to scsi_init_io to the ULDs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c    |   22 ++++++++++++----------
 drivers/scsi/sd.c          |    2 +-
 drivers/scsi/sr.c          |    3 +--
 include/scsi/scsi_driver.h |    1 -
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b505b06..bc84811 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1115,11 +1115,10 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
 
 /*
- * Setup a REQ_TYPE_FS command.  These are simple read/write request
- * from filesystems that still need to be translated to SCSI CDBs from
- * the ULD.
+ * Setup a REQ_TYPE_FS command.  These are simple request from filesystems
+ * that still need to be translated to SCSI CDBs from the ULD.
  */
-int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
+static int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 {
 	struct scsi_cmnd *cmd = req->special;
 
@@ -1131,9 +1130,8 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	}
 
 	memset(cmd->cmnd, 0, BLK_MAX_CDB);
-	return scsi_init_io(cmd, GFP_ATOMIC);
+	return scsi_cmd_to_driver(cmd)->init_command(cmd);
 }
-EXPORT_SYMBOL(scsi_setup_fs_cmnd);
 
 static int
 scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
@@ -1238,12 +1236,16 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
 		goto out;
 	}
 
-	if (req->cmd_type == REQ_TYPE_FS)
-		ret = scsi_cmd_to_driver(cmd)->init_command(cmd);
-	else if (req->cmd_type == REQ_TYPE_BLOCK_PC)
+	switch (req->cmd_type) {
+	case REQ_TYPE_FS:
+		ret = scsi_setup_fs_cmnd(sdev, req);
+		break;
+	case REQ_TYPE_BLOCK_PC:
 		ret = scsi_setup_blk_pc_cmnd(sdev, req);
-	else
+		break;
+	default:
 		ret = BLKPREP_KILL;
+	}
 
 out:
 	return scsi_prep_return(q, req, ret);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 9c86e3d..001b3e8 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -894,7 +894,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
 		ret = scsi_setup_flush_cmnd(sdp, rq);
 		goto out;
 	}
-	ret = scsi_setup_fs_cmnd(sdp, rq);
+	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
 	if (ret != BLKPREP_OK)
 		goto out;
 	SCpnt = rq->special;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index a7ea27c..9feeb37 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -385,10 +385,9 @@ static int sr_init_command(struct scsi_cmnd *SCpnt)
 	int block = 0, this_count, s_size;
 	struct scsi_cd *cd;
 	struct request *rq = SCpnt->request;
-	struct scsi_device *sdp = SCpnt->device;
 	int ret;
 
-	ret = scsi_setup_fs_cmnd(sdp, rq);
+	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
 	if (ret != BLKPREP_OK)
 		goto out;
 	SCpnt = rq->special;
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 36c4114..009d2ae 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -30,6 +30,5 @@ extern int scsi_register_interface(struct class_interface *);
 	class_interface_unregister(intf)
 
 int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
-int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
 
 #endif /* _SCSI_SCSI_DRIVER_H */
-- 
1.7.10.4

Re: [PATCH 02/10] scsi: restructure command initialization for TYPE_FS requests

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> We should call the device handler prep_fn for all TYPE_FS requests,
> not just simple read/write calls that are handled by the disk driver.
>
> Restructure the common I/O code to call the prep_fn handler and zero
> out the CDB, and just leave the call to scsi_init_io to the ULDs.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c    |   22 ++++++++++++----------
>   drivers/scsi/sd.c          |    2 +-
>   drivers/scsi/sr.c          |    3 +--
>   include/scsi/scsi_driver.h |    1 -
>   4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index b505b06..bc84811 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1115,11 +1115,10 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>   EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
>
>   /*
> - * Setup a REQ_TYPE_FS command.  These are simple read/write request
> - * from filesystems that still need to be translated to SCSI CDBs from
> - * the ULD.
> + * Setup a REQ_TYPE_FS command.  These are simple request from filesystems
> + * that still need to be translated to SCSI CDBs from the ULD.
>    */
> -int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
> +static int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>   {
>   	struct scsi_cmnd *cmd = req->special;
>
> @@ -1131,9 +1130,8 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>   	}
>
>   	memset(cmd->cmnd, 0, BLK_MAX_CDB);
> -	return scsi_init_io(cmd, GFP_ATOMIC);
> +	return scsi_cmd_to_driver(cmd)->init_command(cmd);
>   }
> -EXPORT_SYMBOL(scsi_setup_fs_cmnd);
>
>   static int
>   scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
> @@ -1238,12 +1236,16 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
>   		goto out;
>   	}
>
> -	if (req->cmd_type == REQ_TYPE_FS)
> -		ret = scsi_cmd_to_driver(cmd)->init_command(cmd);
> -	else if (req->cmd_type == REQ_TYPE_BLOCK_PC)
> +	switch (req->cmd_type) {
> +	case REQ_TYPE_FS:
> +		ret = scsi_setup_fs_cmnd(sdev, req);
> +		break;
> +	case REQ_TYPE_BLOCK_PC:
>   		ret = scsi_setup_blk_pc_cmnd(sdev, req);
> -	else
> +		break;
> +	default:
>   		ret = BLKPREP_KILL;
> +	}
>
>   out:
>   	return scsi_prep_return(q, req, ret);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 9c86e3d..001b3e8 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -894,7 +894,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
>   		ret = scsi_setup_flush_cmnd(sdp, rq);
>   		goto out;
>   	}
> -	ret = scsi_setup_fs_cmnd(sdp, rq);
> +	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
>   	if (ret != BLKPREP_OK)
>   		goto out;
>   	SCpnt = rq->special;
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index a7ea27c..9feeb37 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -385,10 +385,9 @@ static int sr_init_command(struct scsi_cmnd *SCpnt)
>   	int block = 0, this_count, s_size;
>   	struct scsi_cd *cd;
>   	struct request *rq = SCpnt->request;
> -	struct scsi_device *sdp = SCpnt->device;
>   	int ret;
>
> -	ret = scsi_setup_fs_cmnd(sdp, rq);
> +	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
>   	if (ret != BLKPREP_OK)
>   		goto out;
>   	SCpnt = rq->special;
> diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
> index 36c4114..009d2ae 100644
> --- a/include/scsi/scsi_driver.h
> +++ b/include/scsi/scsi_driver.h
> @@ -30,6 +30,5 @@ extern int scsi_register_interface(struct class_interface *);
>   	class_interface_unregister(intf)
>
>   int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
> -int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
>
>   #endif /* _SCSI_SCSI_DRIVER_H */
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] scsi: restructure command initialization for TYPE_FS requests

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> We should call the device handler prep_fn for all TYPE_FS
Christoph> requests, not just simple read/write calls that are handled
Christoph> by the disk driver.

Christoph> Restructure the common I/O code to call the prep_fn handler
Christoph> and zero out the CDB, and just leave the call to scsi_init_io
Christoph> to the ULDs.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 03/10] scsi: set sc_data_direction in common code

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c |   14 +++++++-------
 drivers/scsi/sd.c       |    2 --
 drivers/scsi/sr.c       |    2 --
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index bc84811..ea23860 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1101,13 +1101,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 	}
 
 	cmd->cmd_len = req->cmd_len;
-	if (!blk_rq_bytes(req))
-		cmd->sc_data_direction = DMA_NONE;
-	else if (rq_data_dir(req) == WRITE)
-		cmd->sc_data_direction = DMA_TO_DEVICE;
-	else
-		cmd->sc_data_direction = DMA_FROM_DEVICE;
-	
 	cmd->transfersize = blk_rq_bytes(req);
 	cmd->allowed = req->retries;
 	return BLKPREP_OK;
@@ -1236,6 +1229,13 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
 		goto out;
 	}
 
+	if (!blk_rq_bytes(req))
+		cmd->sc_data_direction = DMA_NONE;
+	else if (rq_data_dir(req) == WRITE)
+		cmd->sc_data_direction = DMA_TO_DEVICE;
+	else
+		cmd->sc_data_direction = DMA_FROM_DEVICE;
+
 	switch (req->cmd_type) {
 	case REQ_TYPE_FS:
 		ret = scsi_setup_fs_cmnd(sdev, req);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 001b3e8..d88bdc8 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -994,14 +994,12 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
 			goto out;
 		}
 		SCpnt->cmnd[0] = WRITE_6;
-		SCpnt->sc_data_direction = DMA_TO_DEVICE;
 
 		if (blk_integrity_rq(rq))
 			sd_dif_prepare(rq, block, sdp->sector_size);
 
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_6;
-		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
 	} else {
 		scmd_printk(KERN_ERR, SCpnt, "Unknown command %llx\n", (unsigned long long) rq->cmd_flags);
 		goto out;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 9feeb37..cce4771 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -438,11 +438,9 @@ static int sr_init_command(struct scsi_cmnd *SCpnt)
 		if (!cd->device->writeable)
 			goto out;
 		SCpnt->cmnd[0] = WRITE_10;
-		SCpnt->sc_data_direction = DMA_TO_DEVICE;
 		cd->cdi.media_written = 1;
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_10;
-		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
 	} else {
 		blk_dump_rq_flags(rq, "Unknown sr command");
 		goto out;
-- 
1.7.10.4

Re: [PATCH 03/10] scsi: set sc_data_direction in common code

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c |   14 +++++++-------
>   drivers/scsi/sd.c       |    2 --
>   drivers/scsi/sr.c       |    2 --
>   3 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index bc84811..ea23860 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1101,13 +1101,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>   	}
>
>   	cmd->cmd_len = req->cmd_len;
> -	if (!blk_rq_bytes(req))
> -		cmd->sc_data_direction = DMA_NONE;
> -	else if (rq_data_dir(req) == WRITE)
> -		cmd->sc_data_direction = DMA_TO_DEVICE;
> -	else
> -		cmd->sc_data_direction = DMA_FROM_DEVICE;
> -	
>   	cmd->transfersize = blk_rq_bytes(req);
>   	cmd->allowed = req->retries;
>   	return BLKPREP_OK;
> @@ -1236,6 +1229,13 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
>   		goto out;
>   	}
>
> +	if (!blk_rq_bytes(req))
> +		cmd->sc_data_direction = DMA_NONE;
> +	else if (rq_data_dir(req) == WRITE)
> +		cmd->sc_data_direction = DMA_TO_DEVICE;
> +	else
> +		cmd->sc_data_direction = DMA_FROM_DEVICE;
> +
>   	switch (req->cmd_type) {
>   	case REQ_TYPE_FS:
>   		ret = scsi_setup_fs_cmnd(sdev, req);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 001b3e8..d88bdc8 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -994,14 +994,12 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
>   			goto out;
>   		}
>   		SCpnt->cmnd[0] = WRITE_6;
> -		SCpnt->sc_data_direction = DMA_TO_DEVICE;
>
>   		if (blk_integrity_rq(rq))
>   			sd_dif_prepare(rq, block, sdp->sector_size);
>
>   	} else if (rq_data_dir(rq) == READ) {
>   		SCpnt->cmnd[0] = READ_6;
> -		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>   	} else {
>   		scmd_printk(KERN_ERR, SCpnt, "Unknown command %llx\n", (unsigned long long) rq->cmd_flags);
>   		goto out;
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 9feeb37..cce4771 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -438,11 +438,9 @@ static int sr_init_command(struct scsi_cmnd *SCpnt)
>   		if (!cd->device->writeable)
>   			goto out;
>   		SCpnt->cmnd[0] = WRITE_10;
> -		SCpnt->sc_data_direction = DMA_TO_DEVICE;
>   		cd->cdi.media_written = 1;
>   	} else if (rq_data_dir(rq) == READ) {
>   		SCpnt->cmnd[0] = READ_10;
> -		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>   	} else {
>   		blk_dump_rq_flags(rq, "Unknown sr command");
>   		goto out;
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/10] scsi: set sc_data_direction in common code

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Could have a slightly more verbose patch description. Otherwise OK.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 04/10] sd: don't use scsi_setup_blk_pc_cmnd for flush requests

[Christoph Hellwig]

Simplify handling of flush requests by setting up the command directly
instead of initializing request fields and then calling
scsi_setup_blk_pc_cmnd to propagate the information into the command.

Also rename scsi_setup_flush_cmnd to sd_setup_flush_cmnd for consistency.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d88bdc8..620d32f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -844,14 +844,20 @@ static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq)
 	return ret;
 }
 
-static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
+static int sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
 {
-	rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
-	rq->retries = SD_MAX_RETRIES;
-	rq->cmd[0] = SYNCHRONIZE_CACHE;
-	rq->cmd_len = 10;
+	struct request *rq = cmd->request;
+
+	/* flush requests don't perform I/O, zero the S/G table */
+	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 
-	return scsi_setup_blk_pc_cmnd(sdp, rq);
+	cmd->cmnd[0] = SYNCHRONIZE_CACHE;
+	cmd->cmd_len = 10;
+	cmd->transfersize = 0;
+	cmd->allowed = SD_MAX_RETRIES;
+
+	rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
+	return BLKPREP_OK;
 }
 
 static void sd_uninit_command(struct scsi_cmnd *SCpnt)
@@ -891,7 +897,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
 		ret = sd_setup_write_same_cmnd(sdp, rq);
 		goto out;
 	} else if (rq->cmd_flags & REQ_FLUSH) {
-		ret = scsi_setup_flush_cmnd(sdp, rq);
+		ret = sd_setup_flush_cmnd(SCpnt);
 		goto out;
 	}
 	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
-- 
1.7.10.4

Re: [PATCH 04/10] sd: don't use scsi_setup_blk_pc_cmnd for flush requests

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Simplify handling of flush requests by setting up the command directly
> instead of initializing request fields and then calling
> scsi_setup_blk_pc_cmnd to propagate the information into the command.
>
> Also rename scsi_setup_flush_cmnd to sd_setup_flush_cmnd for consistency.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c |   20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d88bdc8..620d32f 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -844,14 +844,20 @@ static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq)
>   	return ret;
>   }
>
> -static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
> +static int sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
>   {
> -	rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> -	rq->retries = SD_MAX_RETRIES;
> -	rq->cmd[0] = SYNCHRONIZE_CACHE;
> -	rq->cmd_len = 10;
> +	struct request *rq = cmd->request;
> +
> +	/* flush requests don't perform I/O, zero the S/G table */
> +	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>
> -	return scsi_setup_blk_pc_cmnd(sdp, rq);
> +	cmd->cmnd[0] = SYNCHRONIZE_CACHE;
> +	cmd->cmd_len = 10;
> +	cmd->transfersize = 0;
> +	cmd->allowed = SD_MAX_RETRIES;
> +
> +	rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> +	return BLKPREP_OK;
>   }
>
>   static void sd_uninit_command(struct scsi_cmnd *SCpnt)
> @@ -891,7 +897,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
>   		ret = sd_setup_write_same_cmnd(sdp, rq);
>   		goto out;
>   	} else if (rq->cmd_flags & REQ_FLUSH) {
> -		ret = scsi_setup_flush_cmnd(sdp, rq);
> +		ret = sd_setup_flush_cmnd(SCpnt);
>   		goto out;
>   	}
>   	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] sd: don't use scsi_setup_blk_pc_cmnd for flush requests

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> Simplify handling of flush requests by setting up the command
Christoph> directly instead of initializing request fields and then
Christoph> calling scsi_setup_blk_pc_cmnd to propagate the information
Christoph> into the command.

Looks good.

Christoph> Also rename scsi_setup_flush_cmnd to sd_setup_flush_cmnd for
Christoph> consistency.

Been wanting to do that for ages...

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 05/10] sd: don't use scsi_setup_blk_pc_cmnd for write same requests

[Christoph Hellwig]

Simplify handling of write same requests by setting up the command directly
instead of initializing request fields and then calling
scsi_setup_blk_pc_cmnd to propagate the information into the command.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c |   44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 620d32f..25f25dd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -799,14 +799,15 @@ out:
 
 /**
  * sd_setup_write_same_cmnd - write the same data to multiple blocks
- * @sdp: scsi device to operate one
- * @rq: Request to prepare
+ * @cmd: command to prepare
  *
  * Will issue either WRITE SAME(10) or WRITE SAME(16) depending on
  * preference indicated by target device.
  **/
-static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq)
+static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 {
+	struct request *rq = cmd->request;
+	struct scsi_device *sdp = cmd->device;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 	struct bio *bio = rq->bio;
 	sector_t sector = blk_rq_pos(rq);
@@ -822,25 +823,36 @@ static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq)
 	sector >>= ilog2(sdp->sector_size) - 9;
 	nr_sectors >>= ilog2(sdp->sector_size) - 9;
 
-	rq->__data_len = sdp->sector_size;
 	rq->timeout = SD_WRITE_SAME_TIMEOUT;
-	memset(rq->cmd, 0, rq->cmd_len);
 
 	if (sdkp->ws16 || sector > 0xffffffff || nr_sectors > 0xffff) {
-		rq->cmd_len = 16;
-		rq->cmd[0] = WRITE_SAME_16;
-		put_unaligned_be64(sector, &rq->cmd[2]);
-		put_unaligned_be32(nr_sectors, &rq->cmd[10]);
+		cmd->cmd_len = 16;
+		cmd->cmnd[0] = WRITE_SAME_16;
+		put_unaligned_be64(sector, &cmd->cmnd[2]);
+		put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
 	} else {
-		rq->cmd_len = 10;
-		rq->cmd[0] = WRITE_SAME;
-		put_unaligned_be32(sector, &rq->cmd[2]);
-		put_unaligned_be16(nr_sectors, &rq->cmd[7]);
+		cmd->cmd_len = 10;
+		cmd->cmnd[0] = WRITE_SAME;
+		put_unaligned_be32(sector, &cmd->cmnd[2]);
+		put_unaligned_be16(nr_sectors, &cmd->cmnd[7]);
 	}
 
-	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
-	rq->__data_len = nr_bytes;
+	cmd->transfersize = sdp->sector_size;
+	cmd->allowed = rq->retries;
 
+	/*
+	 * For WRITE_SAME the data transferred in the DATA IN buffer is
+	 * different from the amount of data actually written to the target.
+	 *
+	 * We set up __data_len to the amount of data transferred from the
+	 * DATA IN buffer so that blk_rq_map_sg set up the proper S/G list
+	 * to transfer a single sector of data first, but then reset it to
+	 * the amount of data to be written right after so that the I/O path
+	 * knows how much to actually write.
+	 */
+	rq->__data_len = sdp->sector_size;
+	ret = scsi_init_io(cmd, GFP_ATOMIC);
+	rq->__data_len = nr_bytes;
 	return ret;
 }
 
@@ -894,7 +906,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
 		ret = sd_setup_discard_cmnd(sdp, rq);
 		goto out;
 	} else if (rq->cmd_flags & REQ_WRITE_SAME) {
-		ret = sd_setup_write_same_cmnd(sdp, rq);
+		ret = sd_setup_write_same_cmnd(SCpnt);
 		goto out;
 	} else if (rq->cmd_flags & REQ_FLUSH) {
 		ret = sd_setup_flush_cmnd(SCpnt);
-- 
1.7.10.4

Re: [PATCH 05/10] sd: don't use scsi_setup_blk_pc_cmnd for write same requests

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Simplify handling of write same requests by setting up the command directly
> instead of initializing request fields and then calling
> scsi_setup_blk_pc_cmnd to propagate the information into the command.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c |   44 ++++++++++++++++++++++++++++----------------
>   1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 620d32f..25f25dd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -799,14 +799,15 @@ out:
>
>   /**
>    * sd_setup_write_same_cmnd - write the same data to multiple blocks
> - * @sdp: scsi device to operate one
> - * @rq: Request to prepare
> + * @cmd: command to prepare
>    *
>    * Will issue either WRITE SAME(10) or WRITE SAME(16) depending on
>    * preference indicated by target device.
>    **/
> -static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq)
> +static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
>   {
> +	struct request *rq = cmd->request;
> +	struct scsi_device *sdp = cmd->device;
>   	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
>   	struct bio *bio = rq->bio;
>   	sector_t sector = blk_rq_pos(rq);
> @@ -822,25 +823,36 @@ static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq)
>   	sector >>= ilog2(sdp->sector_size) - 9;
>   	nr_sectors >>= ilog2(sdp->sector_size) - 9;
>
> -	rq->__data_len = sdp->sector_size;
>   	rq->timeout = SD_WRITE_SAME_TIMEOUT;
> -	memset(rq->cmd, 0, rq->cmd_len);
>
>   	if (sdkp->ws16 || sector > 0xffffffff || nr_sectors > 0xffff) {
> -		rq->cmd_len = 16;
> -		rq->cmd[0] = WRITE_SAME_16;
> -		put_unaligned_be64(sector, &rq->cmd[2]);
> -		put_unaligned_be32(nr_sectors, &rq->cmd[10]);
> +		cmd->cmd_len = 16;
> +		cmd->cmnd[0] = WRITE_SAME_16;
> +		put_unaligned_be64(sector, &cmd->cmnd[2]);
> +		put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
>   	} else {
> -		rq->cmd_len = 10;
> -		rq->cmd[0] = WRITE_SAME;
> -		put_unaligned_be32(sector, &rq->cmd[2]);
> -		put_unaligned_be16(nr_sectors, &rq->cmd[7]);
> +		cmd->cmd_len = 10;
> +		cmd->cmnd[0] = WRITE_SAME;
> +		put_unaligned_be32(sector, &cmd->cmnd[2]);
> +		put_unaligned_be16(nr_sectors, &cmd->cmnd[7]);
>   	}
>
> -	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> -	rq->__data_len = nr_bytes;
> +	cmd->transfersize = sdp->sector_size;
> +	cmd->allowed = rq->retries;
>
> +	/*
> +	 * For WRITE_SAME the data transferred in the DATA IN buffer is
> +	 * different from the amount of data actually written to the target.
> +	 *
> +	 * We set up __data_len to the amount of data transferred from the
> +	 * DATA IN buffer so that blk_rq_map_sg set up the proper S/G list
> +	 * to transfer a single sector of data first, but then reset it to
> +	 * the amount of data to be written right after so that the I/O path
> +	 * knows how much to actually write.
> +	 */
> +	rq->__data_len = sdp->sector_size;
> +	ret = scsi_init_io(cmd, GFP_ATOMIC);
> +	rq->__data_len = nr_bytes;
>   	return ret;
>   }
>
Hmm? __data_len is the amount of data written _on the target_.
Do we actually care about it?
And if so, why didn't it break with the original version?
In either case a short description in the patch would be nice.

> @@ -894,7 +906,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
>   		ret = sd_setup_discard_cmnd(sdp, rq);
>   		goto out;
>   	} else if (rq->cmd_flags & REQ_WRITE_SAME) {
> -		ret = sd_setup_write_same_cmnd(sdp, rq);
> +		ret = sd_setup_write_same_cmnd(SCpnt);
>   		goto out;
>   	} else if (rq->cmd_flags & REQ_FLUSH) {
>   		ret = sd_setup_flush_cmnd(SCpnt);
>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] sd: don't use scsi_setup_blk_pc_cmnd for write same requests

[Christoph Hellwig]

>> -	rq->__data_len = sdp->sector_size;

>> +	rq->__data_len = sdp->sector_size;
>> +	ret = scsi_init_io(cmd, GFP_ATOMIC);
>> +	rq->__data_len = nr_bytes;
>>   	return ret;
>>   }
>>
> Hmm? __data_len is the amount of data written _on the target_.
> Do we actually care about it?
> And if so, why didn't it break with the original version?
> In either case a short description in the patch would be nice.

The drivers care about it, and scsi_init_io uses it as transfer size,
thus we have to set it to the tranfer length before the scsi_init_io
call, and to the full number of bytes to be written after it.

We already do this before the patch, I just moved the first assginment
next to the call to scsi_init_io so that it's more obvious.

Re: [PATCH 05/10] sd: don't use scsi_setup_blk_pc_cmnd for write same requests

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> Simplify handling of write same requests by setting up the
Christoph> command directly instead of initializing request fields and
Christoph> then calling scsi_setup_blk_pc_cmnd to propagate the
Christoph> information into the command.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 05/10] sd: don't use scsi_setup_blk_pc_cmnd for write same requests

[Christoph Hellwig]

Can I get a second review in addition to the one from Martin for this
one?  This is the last blocker for merging the series and pushing an
updated branch out!

[PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Christoph Hellwig]

Simplify handling of discard requests by setting up the command directly
instead of initializing request fields and then calling
scsi_setup_blk_pc_cmnd to propagate the information into the command.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c |   43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 25f25dd..9737e78 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -691,8 +691,10 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
  * Will issue either UNMAP or WRITE SAME(16) depending on preference
  * indicated by target device.
  **/
-static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
+static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 {
+	struct request *rq = cmd->request;
+	struct scsi_device *sdp = cmd->device;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 	sector_t sector = blk_rq_pos(rq);
 	unsigned int nr_sectors = blk_rq_sectors(rq);
@@ -704,9 +706,6 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 
 	sector >>= ilog2(sdp->sector_size) - 9;
 	nr_sectors >>= ilog2(sdp->sector_size) - 9;
-	rq->timeout = SD_TIMEOUT;
-
-	memset(rq->cmd, 0, rq->cmd_len);
 
 	page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
 	if (!page)
@@ -716,9 +715,9 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	case SD_LBP_UNMAP:
 		buf = page_address(page);
 
-		rq->cmd_len = 10;
-		rq->cmd[0] = UNMAP;
-		rq->cmd[8] = 24;
+		cmd->cmd_len = 10;
+		cmd->cmnd[0] = UNMAP;
+		cmd->cmnd[8] = 24;
 
 		put_unaligned_be16(6 + 16, &buf[0]);
 		put_unaligned_be16(16, &buf[2]);
@@ -729,23 +728,23 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 		break;
 
 	case SD_LBP_WS16:
-		rq->cmd_len = 16;
-		rq->cmd[0] = WRITE_SAME_16;
-		rq->cmd[1] = 0x8; /* UNMAP */
-		put_unaligned_be64(sector, &rq->cmd[2]);
-		put_unaligned_be32(nr_sectors, &rq->cmd[10]);
+		cmd->cmd_len = 16;
+		cmd->cmnd[0] = WRITE_SAME_16;
+		cmd->cmnd[1] = 0x8; /* UNMAP */
+		put_unaligned_be64(sector, &cmd->cmnd[2]);
+		put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
 
 		len = sdkp->device->sector_size;
 		break;
 
 	case SD_LBP_WS10:
 	case SD_LBP_ZERO:
-		rq->cmd_len = 10;
-		rq->cmd[0] = WRITE_SAME;
+		cmd->cmd_len = 10;
+		cmd->cmnd[0] = WRITE_SAME;
 		if (sdkp->provisioning_mode == SD_LBP_WS10)
-			rq->cmd[1] = 0x8; /* UNMAP */
-		put_unaligned_be32(sector, &rq->cmd[2]);
-		put_unaligned_be16(nr_sectors, &rq->cmd[7]);
+			cmd->cmnd[1] = 0x8; /* UNMAP */
+		put_unaligned_be32(sector, &cmd->cmnd[2]);
+		put_unaligned_be16(nr_sectors, &cmd->cmnd[7]);
 
 		len = sdkp->device->sector_size;
 		break;
@@ -756,8 +755,14 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	}
 
 	rq->completion_data = page;
+	rq->timeout = SD_TIMEOUT;
+
 	blk_add_request_payload(rq, page, len);
-	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
+
+	cmd->transfersize = len;
+	cmd->allowed = rq->retries;
+
+	ret = scsi_init_io(cmd, GFP_ATOMIC);
 	rq->__data_len = nr_bytes;
 
 out:
@@ -903,7 +908,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
 	 * block PC requests to make life easier.
 	 */
 	if (rq->cmd_flags & REQ_DISCARD) {
-		ret = sd_setup_discard_cmnd(sdp, rq);
+		ret = sd_setup_discard_cmnd(SCpnt);
 		goto out;
 	} else if (rq->cmd_flags & REQ_WRITE_SAME) {
 		ret = sd_setup_write_same_cmnd(SCpnt);
-- 
1.7.10.4

RE: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Sunday, 29 June, 2014 8:35 AM
> To: James Bottomley
> Cc: Martin K. Petersen; linux-scsi@vger.kernel.org
> Subject: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard
> requests
> 
> Simplify handling of discard requests by setting up the command directly
> instead of initializing request fields and then calling
> scsi_setup_blk_pc_cmnd to propagate the information into the command.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/sd.c |   43 ++++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 25f25dd..9737e78 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -691,8 +691,10 @@ static void sd_config_discard(struct scsi_disk *sdkp,
> unsigned int mode)
>   * Will issue either UNMAP or WRITE SAME(16) depending on preference
>   * indicated by target device.
>   **/
> -static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request
> *rq)
> +static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
>  {
> +	struct request *rq = cmd->request;
> +	struct scsi_device *sdp = cmd->device;
>  	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
>  	sector_t sector = blk_rq_pos(rq);
>  	unsigned int nr_sectors = blk_rq_sectors(rq);
> @@ -704,9 +706,6 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp,
> struct request *rq)
> 
>  	sector >>= ilog2(sdp->sector_size) - 9;
>  	nr_sectors >>= ilog2(sdp->sector_size) - 9;
> -	rq->timeout = SD_TIMEOUT;
> -
> -	memset(rq->cmd, 0, rq->cmd_len);

Is *cmd guaranteed to be zero at this point?

With scsi-mq.2 I've seen UNMAP CDBs with garbage in CDB bytes 
2 to 5 (which causes a drive that checks reserved fields to
reject the command), which suggests there is a case in which 
that is not guaranteed.

[  295.280838] sd 0:0:0:0: [sda] Done: scmd ffff88042b564770 SUCCESS (0x1)
[  295.283368] sd 0:0:0:0: [sda] Result: hostbyte=DID_TARGET_FAILURE driverbyte=DRIVER_OK
[  295.286652] Unmap/Read sub-channel
[  295.287823] sd 0:0:0:0: [sda] scmd ffff88042b564770 CDB:  42 00 09 6a ed 10 00 00 18 00
[  295.290696] sd 0:0:0:0: [sda] Sense Key : Illegal Request [current]
[  295.293621] sd 0:0:0:0: [sda] Add. Sense: Invalid field in cdb
[  295.296206] sd 0:0:0:0: [sda] scmd ffff88042b564770 Discard failure result DID_OK DRIVER_SENSE
[  295.299296] sd 0:0:0:0: [sda] Sense Key : Illegal Request [current]
[  295.302104] sd 0:0:0:0: [sda] Add. Sense: Invalid field in cdb
[  295.304719] Unmap/Read sub-channel
[  295.305829] sd 0:0:0:0: [sda] scmd ffff88042b564770 CDB:  42 00 09 6a ed 10 00 00 18 00
[  295.308777] end_request: critical target error, dev sda, sector 385875968

If so, then getting rid of the double memset is another 
benefit, and removing it from scsi_setup_fs_cmnd would
be good too.  Since that function is EXPORTED, does that
prevent removing its memset?

---
Rob Elliott    HP Server Storage

RE: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Elliott, Robert (Server Storage)
> Sent: Sunday, 06 July, 2014 7:02 PM
> To: Christoph Hellwig; James Bottomley
> Cc: Martin K. Petersen; linux-scsi@vger.kernel.org
> Subject: RE: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard
> requests
> 
> 
> 
> > -----Original Message-----
> > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> > owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> > Sent: Sunday, 29 June, 2014 8:35 AM
> > To: James Bottomley
> > Cc: Martin K. Petersen; linux-scsi@vger.kernel.org
> > Subject: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard
> > requests
> >
> > Simplify handling of discard requests by setting up the command directly
> > instead of initializing request fields and then calling
> > scsi_setup_blk_pc_cmnd to propagate the information into the command.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  drivers/scsi/sd.c |   43 ++++++++++++++++++++++++-------------------
> >  1 file changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 25f25dd..9737e78 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -691,8 +691,10 @@ static void sd_config_discard(struct scsi_disk *sdkp,
> > unsigned int mode)
> >   * Will issue either UNMAP or WRITE SAME(16) depending on preference
> >   * indicated by target device.
> >   **/
> > -static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request
> > *rq)
> > +static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
> >  {
> > +	struct request *rq = cmd->request;
> > +	struct scsi_device *sdp = cmd->device;
> >  	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
> >  	sector_t sector = blk_rq_pos(rq);
> >  	unsigned int nr_sectors = blk_rq_sectors(rq);
> > @@ -704,9 +706,6 @@ static int sd_setup_discard_cmnd(struct scsi_device
> *sdp,
> > struct request *rq)
> >
> >  	sector >>= ilog2(sdp->sector_size) - 9;
> >  	nr_sectors >>= ilog2(sdp->sector_size) - 9;
> > -	rq->timeout = SD_TIMEOUT;
> > -
> > -	memset(rq->cmd, 0, rq->cmd_len);
> 
> Is *cmd guaranteed to be zero at this point?
> 
> With scsi-mq.2 I've seen UNMAP CDBs with garbage in CDB bytes
> 2 to 5 (which causes a drive that checks reserved fields to
> reject the command), which suggests there is a case in which
> that is not guaranteed.

The problem in the existing sd_setup_discard_cmnd is that the 
memset runs before rq->cmd_len is set, so it zeroes out no bytes.

sd_setup_write_same_ doesn't have that issue.

---
Rob Elliott    HP Server Storage

Re: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Christoph Hellwig]

On Mon, Jul 07, 2014 at 12:01:53AM +0000, Elliott, Robert (Server Storage) wrote:
> >  	nr_sectors >>= ilog2(sdp->sector_size) - 9;
> > -	rq->timeout = SD_TIMEOUT;
> > -
> > -	memset(rq->cmd, 0, rq->cmd_len);
> 
> Is *cmd guaranteed to be zero at this point?

Yes, scsi_setup_fs_cmnd takes care of this before calling into the driver.

> With scsi-mq.2 I've seen UNMAP CDBs with garbage in CDB bytes 
> 2 to 5 (which causes a drive that checks reserved fields to
> reject the command), which suggests there is a case in which 
> that is not guaranteed.

Indeed, we only do it up to the actual CDB length, which is due to:

	sd: don't use rq->cmd_len before setting it up

I had to add this because blk-mq doesn't initialize cmd_len, unlike the
old path.

Either we need to merge this series as well to take care of all this,
or I should do a replacement for the above patch that just memsets up to
BLK_MAX_CDB.

> If so, then getting rid of the double memset is another 
> benefit, and removing it from scsi_setup_fs_cmnd would
> be good too.  Since that function is EXPORTED, does that
> prevent removing its memset?

We do need one of the memsets.  With this series I'm keeping the one
in scsi_setup_fs_cmnd so that it's done in one single place, which
seems preferable over having it duplicated in various spots.  At the
end of the series scsi_setup_fs_cmnd ceases to be exported.

Re: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Simplify handling of discard requests by setting up the command directly
> instead of initializing request fields and then calling
> scsi_setup_blk_pc_cmnd to propagate the information into the command.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c |   43 ++++++++++++++++++++++++-------------------
>   1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 25f25dd..9737e78 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -691,8 +691,10 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
>    * Will issue either UNMAP or WRITE SAME(16) depending on preference
>    * indicated by target device.
>    **/
> -static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
> +static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
>   {
> +	struct request *rq = cmd->request;
> +	struct scsi_device *sdp = cmd->device;
>   	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
>   	sector_t sector = blk_rq_pos(rq);
>   	unsigned int nr_sectors = blk_rq_sectors(rq);
> @@ -704,9 +706,6 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
>
>   	sector >>= ilog2(sdp->sector_size) - 9;
>   	nr_sectors >>= ilog2(sdp->sector_size) - 9;
> -	rq->timeout = SD_TIMEOUT;
> -
> -	memset(rq->cmd, 0, rq->cmd_len);
>
>   	page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
>   	if (!page)
> @@ -716,9 +715,9 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
>   	case SD_LBP_UNMAP:
>   		buf = page_address(page);
>
> -		rq->cmd_len = 10;
> -		rq->cmd[0] = UNMAP;
> -		rq->cmd[8] = 24;
> +		cmd->cmd_len = 10;
> +		cmd->cmnd[0] = UNMAP;
> +		cmd->cmnd[8] = 24;
>
>   		put_unaligned_be16(6 + 16, &buf[0]);
>   		put_unaligned_be16(16, &buf[2]);
> @@ -729,23 +728,23 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
>   		break;
>
>   	case SD_LBP_WS16:
> -		rq->cmd_len = 16;
> -		rq->cmd[0] = WRITE_SAME_16;
> -		rq->cmd[1] = 0x8; /* UNMAP */
> -		put_unaligned_be64(sector, &rq->cmd[2]);
> -		put_unaligned_be32(nr_sectors, &rq->cmd[10]);
> +		cmd->cmd_len = 16;
> +		cmd->cmnd[0] = WRITE_SAME_16;
> +		cmd->cmnd[1] = 0x8; /* UNMAP */
> +		put_unaligned_be64(sector, &cmd->cmnd[2]);
> +		put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
>
>   		len = sdkp->device->sector_size;
>   		break;
>
>   	case SD_LBP_WS10:
>   	case SD_LBP_ZERO:
> -		rq->cmd_len = 10;
> -		rq->cmd[0] = WRITE_SAME;
> +		cmd->cmd_len = 10;
> +		cmd->cmnd[0] = WRITE_SAME;
>   		if (sdkp->provisioning_mode == SD_LBP_WS10)
> -			rq->cmd[1] = 0x8; /* UNMAP */
> -		put_unaligned_be32(sector, &rq->cmd[2]);
> -		put_unaligned_be16(nr_sectors, &rq->cmd[7]);
> +			cmd->cmnd[1] = 0x8; /* UNMAP */
> +		put_unaligned_be32(sector, &cmd->cmnd[2]);
> +		put_unaligned_be16(nr_sectors, &cmd->cmnd[7]);
>
>   		len = sdkp->device->sector_size;
>   		break;
> @@ -756,8 +755,14 @@ static int sd_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
>   	}
>
>   	rq->completion_data = page;
> +	rq->timeout = SD_TIMEOUT;
> +
>   	blk_add_request_payload(rq, page, len);
> -	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> +
> +	cmd->transfersize = len;
> +	cmd->allowed = rq->retries;
> +
> +	ret = scsi_init_io(cmd, GFP_ATOMIC);
>   	rq->__data_len = nr_bytes;
>
>   out:
Ah. Here it's done properly.

> @@ -903,7 +908,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
>   	 * block PC requests to make life easier.
>   	 */
>   	if (rq->cmd_flags & REQ_DISCARD) {
> -		ret = sd_setup_discard_cmnd(sdp, rq);
> +		ret = sd_setup_discard_cmnd(SCpnt);
>   		goto out;
>   	} else if (rq->cmd_flags & REQ_WRITE_SAME) {
>   		ret = sd_setup_write_same_cmnd(SCpnt);
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Christoph Hellwig]

On Fri, Jul 11, 2014 at 02:26:24PM +0200, Hannes Reinecke wrote:
>>   	blk_add_request_payload(rq, page, len);
>> -	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
>> +
>> +	cmd->transfersize = len;
>> +	cmd->allowed = rq->retries;
>> +
>> +	ret = scsi_init_io(cmd, GFP_ATOMIC);
>>   	rq->__data_len = nr_bytes;
>>
>>   out:
> Ah. Here it's done properly.

blk_add_request_payload also sets up rq->__data_len..

Re: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> Simplify handling of discard requests by setting up the
Christoph> command directly instead of initializing request fields and
Christoph> then calling scsi_setup_blk_pc_cmnd to propagate the
Christoph> information into the command.

Would be nice to add a comment similar to the WRITE SAME one. Something
like:

/* 
 * Initially __data_len is set to the amount of data that needs to be
 * transferred to the target. This amount depends on whether WRITE SAME
 * or UNMAP is being used. After the scatterlist has been mapped by
 * scsi_init_io() we set __data_len to the size of the area to be
 * discarded on disk. This allows us to report completion on the full
 * amount of blocks described by the request.
 */

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Douglas Gilbert]

On 14-07-13 10:35 AM, Martin K. Petersen wrote:
>>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:
>
> Christoph> Simplify handling of discard requests by setting up the
> Christoph> command directly instead of initializing request fields and
> Christoph> then calling scsi_setup_blk_pc_cmnd to propagate the
> Christoph> information into the command.
>
> Would be nice to add a comment similar to the WRITE SAME one. Something
> like:
>
> /*
>   * Initially __data_len is set to the amount of data that needs to be
>   * transferred to the target. This amount depends on whether WRITE SAME
>   * or UNMAP is being used. After the scatterlist has been mapped by
>   * scsi_init_io() we set __data_len to the size of the area to be
>   * discarded on disk. This allows us to report completion on the full
>   * amount of blocks described by the request.
>   */
>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

To make things clearer when referring to the WRITE SAME command
I would suggest "WRITE SAME(unmap)" for the case when its UNMAP
field is 1 (as I assume it is in this case).

Doug Gilbert

Re: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Christoph Hellwig]

On Sun, Jul 13, 2014 at 10:52:53AM -0400, Douglas Gilbert wrote:
> To make things clearer when referring to the WRITE SAME command
> I would suggest "WRITE SAME(unmap)" for the case when its UNMAP
> field is 1 (as I assume it is in this case).

Actually it could be WRITE SAME (10) with or without unmap,
or WRITE SAME (16) with unmap.  So just leaving it without qualifier here
seems to be least confusing.

Re: [PATCH 06/10] sd: don't use scsi_setup_blk_pc_cmnd for discard requests

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> On Sun, Jul 13, 2014 at 10:52:53AM -0400, Douglas Gilbert wrote:
>> To make things clearer when referring to the WRITE SAME command I
>> would suggest "WRITE SAME(unmap)" for the case when its UNMAP field
>> is 1 (as I assume it is in this case).

Christoph> Actually it could be WRITE SAME (10) with or without unmap,
Christoph> or WRITE SAME (16) with unmap.  So just leaving it without
Christoph> qualifier here seems to be least confusing.

Yeah. I tried to come up with a way to be accurate but simpler seemed
clearer in this case.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 07/10] sd: retry write same commands

[Christoph Hellwig]

Currently cmd->allowed is initialized from rq->retries for write same
commands, but retries is always 0 for non-BLOCK_PC requests.  Set it
to the standard number of retries instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 9737e78..777b141 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -843,7 +843,7 @@ static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 	}
 
 	cmd->transfersize = sdp->sector_size;
-	cmd->allowed = rq->retries;
+	cmd->allowed = SD_MAX_RETRIES;
 
 	/*
 	 * For WRITE_SAME the data transferred in the DATA IN buffer is
-- 
1.7.10.4

Re: [PATCH 07/10] sd: retry write same commands

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Currently cmd->allowed is initialized from rq->retries for write same
> commands, but retries is always 0 for non-BLOCK_PC requests.  Set it
> to the standard number of retries instead.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 9737e78..777b141 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -843,7 +843,7 @@ static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
>   	}
>
>   	cmd->transfersize = sdp->sector_size;
> -	cmd->allowed = rq->retries;
> +	cmd->allowed = SD_MAX_RETRIES;
>
>   	/*
>   	 * For WRITE_SAME the data transferred in the DATA IN buffer is
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] sd: retry write same commands

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> Currently cmd->allowed is initialized from rq->retries for
Christoph> write same commands, but retries is always 0 for non-BLOCK_PC
Christoph> requests.  Set it to the standard number of retries instead.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 08/10] sd: retry discard commands

[Christoph Hellwig]

Currently cmd->allowed is initialized from rq->retries for discard
commands, but retries is always 0 for non-BLOCK_PC requests.  Set it
to the standard number of retries instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 777b141..5f85102 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -760,7 +760,7 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 	blk_add_request_payload(rq, page, len);
 
 	cmd->transfersize = len;
-	cmd->allowed = rq->retries;
+	cmd->allowed = SD_MAX_RETRIES;
 
 	ret = scsi_init_io(cmd, GFP_ATOMIC);
 	rq->__data_len = nr_bytes;
-- 
1.7.10.4

Re: [PATCH 08/10] sd: retry discard commands

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Currently cmd->allowed is initialized from rq->retries for discard
> commands, but retries is always 0 for non-BLOCK_PC requests.  Set it
> to the standard number of retries instead.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 777b141..5f85102 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -760,7 +760,7 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
>   	blk_add_request_payload(rq, page, len);
>
>   	cmd->transfersize = len;
> -	cmd->allowed = rq->retries;
> +	cmd->allowed = SD_MAX_RETRIES;
>
>   	ret = scsi_init_io(cmd, GFP_ATOMIC);
>   	rq->__data_len = nr_bytes;
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] sd: retry discard commands

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> Currently cmd->allowed is initialized from rq->retries for
Christoph> discard commands, but retries is always 0 for non-BLOCK_PC
Christoph> requests.  Set it to the standard number of retries instead.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 09/10] sd: split sd_init_command

[Christoph Hellwig]

Factor out a function to initialize regular read/write commands and leave
sd_init_command as a simple dispatcher to the different prepare routines.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c |   58 ++++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5f85102..ba756b1 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -877,21 +877,7 @@ static int sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
 	return BLKPREP_OK;
 }
 
-static void sd_uninit_command(struct scsi_cmnd *SCpnt)
-{
-	struct request *rq = SCpnt->request;
-
-	if (rq->cmd_flags & REQ_DISCARD)
-		__free_page(rq->completion_data);
-
-	if (SCpnt->cmnd != rq->cmd) {
-		mempool_free(SCpnt->cmnd, sd_cdb_pool);
-		SCpnt->cmnd = NULL;
-		SCpnt->cmd_len = 0;
-	}
-}
-
-static int sd_init_command(struct scsi_cmnd *SCpnt)
+static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 {
 	struct request *rq = SCpnt->request;
 	struct scsi_device *sdp = SCpnt->device;
@@ -903,20 +889,6 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
 	int ret, host_dif;
 	unsigned char protect;
 
-	/*
-	 * Discard request come in as REQ_TYPE_FS but we turn them into
-	 * block PC requests to make life easier.
-	 */
-	if (rq->cmd_flags & REQ_DISCARD) {
-		ret = sd_setup_discard_cmnd(SCpnt);
-		goto out;
-	} else if (rq->cmd_flags & REQ_WRITE_SAME) {
-		ret = sd_setup_write_same_cmnd(SCpnt);
-		goto out;
-	} else if (rq->cmd_flags & REQ_FLUSH) {
-		ret = sd_setup_flush_cmnd(SCpnt);
-		goto out;
-	}
 	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
 	if (ret != BLKPREP_OK)
 		goto out;
@@ -1148,6 +1120,34 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
 	return ret;
 }
 
+static int sd_init_command(struct scsi_cmnd *cmd)
+{
+	struct request *rq = cmd->request;
+
+	if (rq->cmd_flags & REQ_DISCARD)
+		return sd_setup_discard_cmnd(cmd);
+	else if (rq->cmd_flags & REQ_WRITE_SAME)
+		return sd_setup_write_same_cmnd(cmd);
+	else if (rq->cmd_flags & REQ_FLUSH)
+		return sd_setup_flush_cmnd(cmd);
+	else
+		return sd_setup_read_write_cmnd(cmd);
+}
+
+static void sd_uninit_command(struct scsi_cmnd *SCpnt)
+{
+	struct request *rq = SCpnt->request;
+
+	if (rq->cmd_flags & REQ_DISCARD)
+		__free_page(rq->completion_data);
+
+	if (SCpnt->cmnd != rq->cmd) {
+		mempool_free(SCpnt->cmnd, sd_cdb_pool);
+		SCpnt->cmnd = NULL;
+		SCpnt->cmd_len = 0;
+	}
+}
+
 /**
  *	sd_open - open a scsi disk device
  *	@inode: only i_rdev member may be used
-- 
1.7.10.4

Re: [PATCH 09/10] sd: split sd_init_command

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Factor out a function to initialize regular read/write commands and leave
> sd_init_command as a simple dispatcher to the different prepare routines.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c |   58 ++++++++++++++++++++++++++---------------------------
>   1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5f85102..ba756b1 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -877,21 +877,7 @@ static int sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
>   	return BLKPREP_OK;
>   }
>
> -static void sd_uninit_command(struct scsi_cmnd *SCpnt)
> -{
> -	struct request *rq = SCpnt->request;
> -
> -	if (rq->cmd_flags & REQ_DISCARD)
> -		__free_page(rq->completion_data);
> -
> -	if (SCpnt->cmnd != rq->cmd) {
> -		mempool_free(SCpnt->cmnd, sd_cdb_pool);
> -		SCpnt->cmnd = NULL;
> -		SCpnt->cmd_len = 0;
> -	}
> -}
> -
> -static int sd_init_command(struct scsi_cmnd *SCpnt)
> +static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>   {
>   	struct request *rq = SCpnt->request;
>   	struct scsi_device *sdp = SCpnt->device;
> @@ -903,20 +889,6 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
>   	int ret, host_dif;
>   	unsigned char protect;
>
> -	/*
> -	 * Discard request come in as REQ_TYPE_FS but we turn them into
> -	 * block PC requests to make life easier.
> -	 */
> -	if (rq->cmd_flags & REQ_DISCARD) {
> -		ret = sd_setup_discard_cmnd(SCpnt);
> -		goto out;
> -	} else if (rq->cmd_flags & REQ_WRITE_SAME) {
> -		ret = sd_setup_write_same_cmnd(SCpnt);
> -		goto out;
> -	} else if (rq->cmd_flags & REQ_FLUSH) {
> -		ret = sd_setup_flush_cmnd(SCpnt);
> -		goto out;
> -	}
>   	ret = scsi_init_io(SCpnt, GFP_ATOMIC);
>   	if (ret != BLKPREP_OK)
>   		goto out;
> @@ -1148,6 +1120,34 @@ static int sd_init_command(struct scsi_cmnd *SCpnt)
>   	return ret;
>   }
>
> +static int sd_init_command(struct scsi_cmnd *cmd)
> +{
> +	struct request *rq = cmd->request;
> +
> +	if (rq->cmd_flags & REQ_DISCARD)
> +		return sd_setup_discard_cmnd(cmd);
> +	else if (rq->cmd_flags & REQ_WRITE_SAME)
> +		return sd_setup_write_same_cmnd(cmd);
> +	else if (rq->cmd_flags & REQ_FLUSH)
> +		return sd_setup_flush_cmnd(cmd);
> +	else
> +		return sd_setup_read_write_cmnd(cmd);
> +}
> +
> +static void sd_uninit_command(struct scsi_cmnd *SCpnt)
> +{
> +	struct request *rq = SCpnt->request;
> +
> +	if (rq->cmd_flags & REQ_DISCARD)
> +		__free_page(rq->completion_data);
> +
> +	if (SCpnt->cmnd != rq->cmd) {
> +		mempool_free(SCpnt->cmnd, sd_cdb_pool);
> +		SCpnt->cmnd = NULL;
> +		SCpnt->cmd_len = 0;
> +	}
> +}
> +
>   /**
>    *	sd_open - open a scsi disk device
>    *	@inode: only i_rdev member may be used
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] sd: split sd_init_command

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> Factor out a function to initialize regular read/write
Christoph> commands and leave sd_init_command as a simple dispatcher to
Christoph> the different prepare routines.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 10/10] scsi: mark scsi_setup_blk_pc_cmnd static

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c    |    3 +--
 include/scsi/scsi_driver.h |    2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ea23860..f0a3ef1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1080,7 +1080,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 	return cmd;
 }
 
-int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
+static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 {
 	struct scsi_cmnd *cmd = req->special;
 
@@ -1105,7 +1105,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 	cmd->allowed = req->retries;
 	return BLKPREP_OK;
 }
-EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
 
 /*
  * Setup a REQ_TYPE_FS command.  These are simple request from filesystems
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 009d2ae..c2b7598 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -29,6 +29,4 @@ extern int scsi_register_interface(struct class_interface *);
 #define scsi_unregister_interface(intf) \
 	class_interface_unregister(intf)
 
-int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
-
 #endif /* _SCSI_SCSI_DRIVER_H */
-- 
1.7.10.4

Re: [PATCH 10/10] scsi: mark scsi_setup_blk_pc_cmnd static

[Hannes Reinecke]

On 06/29/2014 03:34 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c    |    3 +--
>   include/scsi/scsi_driver.h |    2 --
>   2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ea23860..f0a3ef1 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1080,7 +1080,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
>   	return cmd;
>   }
>
> -int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> +static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>   {
>   	struct scsi_cmnd *cmd = req->special;
>
> @@ -1105,7 +1105,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>   	cmd->allowed = req->retries;
>   	return BLKPREP_OK;
>   }
> -EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
>
>   /*
>    * Setup a REQ_TYPE_FS command.  These are simple request from filesystems
> diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
> index 009d2ae..c2b7598 100644
> --- a/include/scsi/scsi_driver.h
> +++ b/include/scsi/scsi_driver.h
> @@ -29,6 +29,4 @@ extern int scsi_register_interface(struct class_interface *);
>   #define scsi_unregister_interface(intf) \
>   	class_interface_unregister(intf)
>
> -int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
> -
>   #endif /* _SCSI_SCSI_DRIVER_H */
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/10] scsi: mark scsi_setup_blk_pc_cmnd static

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: RFC: clean up command setup

[Christoph Hellwig]

On Sun, Jun 29, 2014 at 03:34:31PM +0200, Christoph Hellwig wrote:
> There has been some confusion because scsi_setup_blk_pc_cmnd is used to set
> up various special TYPE_FS commands.  I've looked into the area and come
> up with various cleanup, as well as few small bug fixes that were turned
> up by this.
> 
> The series is against the core-for-3.17 branch of my scsi-queue tree.

I'd like to drop the RFC and officially propose this for inclusion.  As
noted elsewhere this also helps as a preparation for handling the WRITE
SAME and discard requests properly for scsi-mq, so I'd love to get some
reviews for it, and will report scsi-mq on top of it.