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

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

[FUJITA Tomonori]

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]);
+		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;
+
 	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;
-- 
1.6.5

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

[Boaz Harrosh]

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;

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

[FUJITA Tomonori]

On Wed, 02 Jun 2010 11:14:04 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> >  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.

Ok, I can remove it.


> > +		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?

Some LLDs already use scsi_get_lba(). They don't know scsi command
from pc or fs so they can't use scsi_get_lba() now. Well, they can see
scsi_cmnd->request to know pc or fs but it's layer violation. They
shouldn't access to the block layer stuff.


> 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.

I don't think that calling such function in LLDs is a clean design. We
set request->__sector for fs requests. Why not for pc requests? Then
we can use blk_rq_pos() cleanly. 


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

I'm not sure if this effects the performance notably.

I'll think about something different if James doesn't like the
approach.

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

[Boaz Harrosh]

On 06/02/2010 11:39 AM, FUJITA Tomonori wrote:
> On Wed, 02 Jun 2010 11:14:04 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>>  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.
> 
> Ok, I can remove it.
> 
> 
>>> +		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?
> 
> Some LLDs already use scsi_get_lba(). They don't know scsi command
> from pc or fs so they can't use scsi_get_lba() now. Well, they can see
> scsi_cmnd->request to know pc or fs but it's layer violation. They
> shouldn't access to the block layer stuff.
> 

right, so hide it all under an API that is implemented at the right
level, and protect them. The driver should call a single simple
API the magic will be done at the generic level

	if (blk_fs_rq(req))
		return blk_rq_pos(req);
	else
		return scsi_somthing();

> 
>> 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.
> 
> I don't think that calling such function in LLDs is a clean design. We
> set request->__sector for fs requests. Why not for pc requests? Then
> we can use blk_rq_pos() cleanly. 
> 

Because "fs requests" have a defined blk_rq_pos(). But "pc requests"
don't necessarily have one. Pretending to invent one is stupid and
dangerous. Better keep a -1 for all "pc requests".

scsi LLDs can be pass-through for disks, as well as lots of other
type of devices. They should not assume the role of a disk. The use
of blk_rq_pos() at LLDs is probably wrong in the first place. Unless
it is some kind of scsi emulation driver like scsi_debug.

> 
>> This is not accepted, for osd for instance, on the hot path like this.
>> The few users should be fixed.
> 
> I'm not sure if this effects the performance notably.
> 
> I'll think about something different if James doesn't like the
> approach.

NAK from me
Boaz

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

[FUJITA Tomonori]

On Wed, 02 Jun 2010 11:54:25 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> > Some LLDs already use scsi_get_lba(). They don't know scsi command
> > from pc or fs so they can't use scsi_get_lba() now. Well, they can see
> > scsi_cmnd->request to know pc or fs but it's layer violation. They
> > shouldn't access to the block layer stuff.
> > 
> 
> right, so hide it all under an API that is implemented at the right
> level, and protect them. The driver should call a single simple
> API the magic will be done at the generic level
> 
> 	if (blk_fs_rq(req))
> 		return blk_rq_pos(req);
> 	else
> 		return scsi_somthing();

The first version that I posted yesterday is something like that. But
I don't think that it's clean.


> >> 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.
> > 
> > I don't think that calling such function in LLDs is a clean design. We
> > set request->__sector for fs requests. Why not for pc requests? Then
> > we can use blk_rq_pos() cleanly. 
> > 
> 
> Because "fs requests" have a defined blk_rq_pos(). But "pc requests"
> don't necessarily have one. Pretending to invent one is stupid and
> dangerous. Better keep a -1 for all "pc requests".

I don't see your logic, why using blk_rq_pos() for pc is dangerous?


> scsi LLDs can be pass-through for disks, as well as lots of other
> type of devices. They should not assume the role of a disk. The use
> of blk_rq_pos() at LLDs is probably wrong in the first place. Unless
> it is some kind of scsi emulation driver like scsi_debug.

Not only scsi_debug. Have you seen how scsi_get_lba() is used? LLDs
sometime needs to know lba in scsi commands.

How about calling this function in sd_prep_fn, sr_prep_fn instead of
scsi_setup_blk_pc_cmnd? Then it doesn't affect OSD.

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

[Boaz Harrosh]

On 06/02/2010 12:05 PM, FUJITA Tomonori wrote:
> On Wed, 02 Jun 2010 11:54:25 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>> Some LLDs already use scsi_get_lba(). They don't know scsi command
>>> from pc or fs so they can't use scsi_get_lba() now. Well, they can see
>>> scsi_cmnd->request to know pc or fs but it's layer violation. They
>>> shouldn't access to the block layer stuff.
>>>
>>
>> right, so hide it all under an API that is implemented at the right
>> level, and protect them. The driver should call a single simple
>> API the magic will be done at the generic level
>>
>> 	if (blk_fs_rq(req))
>> 		return blk_rq_pos(req);
>> 	else
>> 		return scsi_somthing();
> 
> The first version that I posted yesterday is something like that. But
> I don't think that it's clean.
> 

I can't find it. I'd like to see

> 
>>>> 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.
>>>
>>> I don't think that calling such function in LLDs is a clean design. We
>>> set request->__sector for fs requests. Why not for pc requests? Then
>>> we can use blk_rq_pos() cleanly. 
>>>
>>
>> Because "fs requests" have a defined blk_rq_pos(). But "pc requests"
>> don't necessarily have one. Pretending to invent one is stupid and
>> dangerous. Better keep a -1 for all "pc requests".
> 
> I don't see your logic, why using blk_rq_pos() for pc is dangerous?

I already explained. because you must know the device type before
you assume the presence of lba.

> 
>> scsi LLDs can be pass-through for disks, as well as lots of other
>> type of devices. They should not assume the role of a disk. The use
>> of blk_rq_pos() at LLDs is probably wrong in the first place. Unless
>> it is some kind of scsi emulation driver like scsi_debug.
> 
> Not only scsi_debug. Have you seen how scsi_get_lba() is used? LLDs
> sometime needs to know lba in scsi commands.
> 
> How about calling this function in sd_prep_fn, sr_prep_fn instead of
> scsi_setup_blk_pc_cmnd? Then it doesn't affect OSD.

That's fine because these know that they are disk and ROM/RAM device types.

Watch out with sr because it supports other command sets. there might
be more too it.

Boaz

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

[Martin K. Petersen]

>>>>> "Tomo" == FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:

>> scsi LLDs can be pass-through for disks, as well as lots of other
>> type of devices. They should not assume the role of a disk. The use
>> of blk_rq_pos() at LLDs is probably wrong in the first place. Unless
>> it is some kind of scsi emulation driver like scsi_debug.

Tomo> Not only scsi_debug. Have you seen how scsi_get_lba() is used?
Tomo> LLDs sometime needs to know lba in scsi commands.

scsi_get_lba() is used by DIX/DIF-aware HBA drivers to get the first LBA
for use in reference tag verification/preparation.

This code path is only executed by the drivers if the command protection
mode is > SCSI_PROT_NORMAL which can only be true for a READ/WRITE
command.  I don't see any drivers using it for other purposes.

I have some changes in the pipeline to better support protection
interleaving in DIF Type 2.  I was contemplating renaming scsi_get_lba()
to scsi_get_reference_tag() or something to that effect.

As as for the reason scsi_get_lba() looks at the start sector directly:
I was simply trying to avoid growing struct scsi_cmnd.  The "correct"
thing would have been to add an initial reference tag field to scsi_cmnd
and have that be filled out by sd_prot_op().

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[FUJITA Tomonori]

On Wed, 02 Jun 2010 09:13:51 -0400
"Martin K. Petersen" <martin.petersen@oracle.com> wrote:

> >>>>> "Tomo" == FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:
> 
> >> scsi LLDs can be pass-through for disks, as well as lots of other
> >> type of devices. They should not assume the role of a disk. The use
> >> of blk_rq_pos() at LLDs is probably wrong in the first place. Unless
> >> it is some kind of scsi emulation driver like scsi_debug.
> 
> Tomo> Not only scsi_debug. Have you seen how scsi_get_lba() is used?
> Tomo> LLDs sometime needs to know lba in scsi commands.
> 
> scsi_get_lba() is used by DIX/DIF-aware HBA drivers to get the first LBA
> for use in reference tag verification/preparation.
> 
> This code path is only executed by the drivers if the command protection
> mode is > SCSI_PROT_NORMAL which can only be true for a READ/WRITE
> command.  I don't see any drivers using it for other purposes.

Yeah, however, seems that there are several drivers that get lba and
sector info from cdb.

Some does it for something like statistics, debug printk, etc.

Some does it to build hardware descriptor (e.g. 3w, megaraid, several
drivers in usb/storage/, etc).

So it would be useful to have scsi_get_lba() that always works, I think.

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

[Prakash, Sathya]

Actually the name scsi_get_lba is misleading that the LLDs can use this function to get the lba from a scsi_cmnd, irrespective of whether it is FS request or not. And I did use it for getting lba in my development driver and faced the problem with non-fs requests and raised the flag.

So IMHO it would be better either to fix the function to return proper value or name it to a different and restrict the usage only for DIF purpose.

Thanks
Sathya

-----Original Message-----
From: FUJITA Tomonori [mailto:fujita.tomonori@lab.ntt.co.jp] 
Sent: Thursday, June 03, 2010 4:29 PM
To: martin.petersen@oracle.com
Cc: fujita.tomonori@lab.ntt.co.jp; bharrosh@panasas.com; James.Bottomley@suse.de; jaxboe@fusionio.com; linux-scsi@vger.kernel.org; Prakash, Sathya
Subject: Re: [PATCH] scsi: fix scsi_get_lba helper function for pc command

On Wed, 02 Jun 2010 09:13:51 -0400
"Martin K. Petersen" <martin.petersen@oracle.com> wrote:

> >>>>> "Tomo" == FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:
> 
> >> scsi LLDs can be pass-through for disks, as well as lots of other
> >> type of devices. They should not assume the role of a disk. The use
> >> of blk_rq_pos() at LLDs is probably wrong in the first place. Unless
> >> it is some kind of scsi emulation driver like scsi_debug.
> 
> Tomo> Not only scsi_debug. Have you seen how scsi_get_lba() is used?
> Tomo> LLDs sometime needs to know lba in scsi commands.
> 
> scsi_get_lba() is used by DIX/DIF-aware HBA drivers to get the first LBA
> for use in reference tag verification/preparation.
> 
> This code path is only executed by the drivers if the command protection
> mode is > SCSI_PROT_NORMAL which can only be true for a READ/WRITE
> command.  I don't see any drivers using it for other purposes.

Yeah, however, seems that there are several drivers that get lba and
sector info from cdb.

Some does it for something like statistics, debug printk, etc.

Some does it to build hardware descriptor (e.g. 3w, megaraid, several
drivers in usb/storage/, etc).

So it would be useful to have scsi_get_lba() that always works, I think.

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

[James Bottomley]

On Wed, 2010-06-02 at 16:33 +0900, 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]);
> +		break;

You can't do this ... unless you know the format of the command.  For
instance, if you knew this was a 32 byte CDB, then SPC does define where
the LBA is.

> +	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;

I really wouldn't do it like this because there are going to be
unexpected gaps.  What about using the CDB groups instead?  Each CDB
group has a well defined location for the LBA.  The only problem is the
16 byte commands group because they have two forms: ordinary and long
LBA.

James

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

[Douglas Gilbert]

On 10-06-02 10:07 AM, James Bottomley wrote:
> On Wed, 2010-06-02 at 16:33 +0900, 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]);
>> +		break;
>
> You can't do this ... unless you know the format of the command.  For
> instance, if you knew this was a 32 byte CDB, then SPC does define where
> the LBA is.
>
>> +	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;
>
> I really wouldn't do it like this because there are going to be
> unexpected gaps.  What about using the CDB groups instead?  Each CDB
> group has a well defined location for the LBA.  The only problem is the
> 16 byte commands group because they have two forms: ordinary and long
> LBA.

The 16 byte SCSI command might be ATA PASS THROUGH (16)!

Is it so important to have scsi_get_lba() for "PC" commands?
Why not just return 0 or MAX_UINT and be done with it.

A "PC" command might be lots of things. For example:
   - a tunnelled ATA command
   - a non SCSI protocol
   - a SCSI vendor specific or obsolete command
     (e.g. some SCSI PRINTER commands share opcodes with
      READ and WRITE)
   - from a lesser used command set (e.g. OSD or SSC)

A LLD may well be able to narrow the field if it has a
reason to decode the command to find a LBA or other info.

Doug Gilbert

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

[FUJITA Tomonori]

On Wed, 02 Jun 2010 10:55:17 -0400
Douglas Gilbert <dgilbert@interlog.com> wrote:

> Is it so important to have scsi_get_lba() for "PC" commands?
> Why not just return 0 or MAX_UINT and be done with it.

Might not be so important. However, as I wrote to Martin, we already
have duplicated code that does the same thing. It's nice if we can
clean them up.


> A "PC" command might be lots of things. For example:
>    - a tunnelled ATA command
>    - a non SCSI protocol
>    - a SCSI vendor specific or obsolete command
>      (e.g. some SCSI PRINTER commands share opcodes with
>       READ and WRITE)
>    - from a lesser used command set (e.g. OSD or SSC)
> 
> A LLD may well be able to narrow the field if it has a
> reason to decode the command to find a LBA or other info.

Yeah, but it would be better to set such info in the common place?
If a LLD likes, it gets such info again in any way for itself.