Re: [PATCH] scsi: fix scsi_get_lba helper function for pc command

[Boaz Harrosh <bharrosh@panasas.com>]

On 06/02/2010 10:33 AM, FUJITA Tomonori wrote:
> This fixes scsi_get_lba() helper function for PC commands.
> 
> Only the block layer is supposed to touch rq->__sector. We could
> create a new accessor for it. But it seems overdoing a bit? Jens?
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] scsi: fix scsi_get_lba helper function for pc command
> 
> scsi_get_lba() can be used to get the lba info from
> scsi_cmnd. scsi_get_lba() gets the lba info from rq->__sector so
> scsi_get_lba() returns a bogus value for PC commands (we don't use
> rq->__sector for PC commands).
> 
> This patch sets rq->__sector in scsi_setup_blk_pc_cmnd() so that
> scsi_get_lba() works with PC commands.
> 
> scsi_get_data_transfer_info() is taken from
> scsi_debug. scsi_get_data_transfer_info() looks useful for some
> (scsi_debug, scsi_trace, libata, etc). So I export it. I'll convert
> them to use scsi_get_data_transfer_info().
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/scsi_lib.c  |   54 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/scsi/scsi_cmnd.h |    4 +++
>  2 files changed, 57 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 1646fe7..b9b7f40 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -19,6 +19,7 @@
>  #include <linux/delay.h>
>  #include <linux/hardirq.h>
>  #include <linux/scatterlist.h>
> +#include <asm/unaligned.h>
>  
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
> @@ -69,6 +70,52 @@ struct kmem_cache *scsi_sdb_cache;
>  
>  static void scsi_run_queue(struct request_queue *q);
>  
> +int scsi_get_data_transfer_info(unsigned char *cmd, unsigned long long *lba,
> +				unsigned int *num, unsigned int *ei_lba)
> +{
> +	int ret = 0;
> +
> +	switch (*cmd) {
> +	case VARIABLE_LENGTH_CMD:
> +		*lba = get_unaligned_be64(&cmd[12]);
> +		*num = get_unaligned_be32(&cmd[28]);
> +		*ei_lba = get_unaligned_be32(&cmd[20]);

This is true for scsi_debug maybe but totally wrong
for any none disk command set.

There is nothing you can do here leave it -1.

> +		break;
> +	case WRITE_16:
> +	case READ_16:
> +	case VERIFY_16:
> +	case WRITE_SAME_16:
> +		*lba = get_unaligned_be64(&cmd[2]);
> +		*num = get_unaligned_be32(&cmd[10]);
> +		break;
> +	case WRITE_12:
> +	case READ_12:
> +	case VERIFY_12:
> +		*lba = get_unaligned_be32(&cmd[2]);
> +		*num = get_unaligned_be32(&cmd[6]);
> +		break;
> +	case WRITE_10:
> +	case READ_10:
> +	case XDWRITEREAD_10:
> +	case VERIFY:
> +	case WRITE_SAME:
> +		*lba = get_unaligned_be32(&cmd[2]);
> +		*num = get_unaligned_be16(&cmd[7]);
> +		break;
> +	case WRITE_6:
> +	case READ_6:
> +		*lba = (u32)cmd[3] | (u32)cmd[2] << 8 |
> +			(u32)(cmd[1] & 0x1f) << 16;
> +		*num = (0 == cmd[4]) ? 256 : cmd[4];
> +		break;
> +	default:
> +		ret = -1;
> +		break;
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL(scsi_get_data_transfer_info);
> +
>  /*
>   * Function:	scsi_unprep_request()
>   *
> @@ -1046,6 +1093,8 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>  {
>  	struct scsi_cmnd *cmd;
>  	int ret = scsi_prep_state_check(sdev, req);
> +	unsigned int num, ei_lba;
> +	unsigned long long lba;
>  
>  	if (ret != BLKPREP_OK)
>  		return ret;
> @@ -1082,7 +1131,10 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>  		cmd->sc_data_direction = DMA_TO_DEVICE;
>  	else
>  		cmd->sc_data_direction = DMA_FROM_DEVICE;
> -	
> +
> +	scsi_get_data_transfer_info(cmd->cmnd, &lba, &num, &ei_lba);
> +	req->__sector = lba;
> +

Why do it for every command. Even if no one is using it?
Only drivers/code that actually cares should call this member.

It should be easy enough to search for __sector and change them
to a function call.

This is not accepted, for osd for instance, on the hot path like this.
The few users should be fixed.

>  	cmd->transfersize = blk_rq_bytes(req);
>  	cmd->allowed = req->retries;
>  	return BLKPREP_OK;
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index a5e885a..be3c785 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -151,6 +151,10 @@ extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
>  struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
>  void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
>  
> +extern int scsi_get_data_transfer_info(unsigned char *cmd,
> +				       unsigned long long *lba,
> +				       unsigned int *num, unsigned int *ei_lba);
> +
>  static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
>  {
>  	return cmd->sdb.table.nents;