Re: [PATCH] megaraid_sas: Enable shared tag map

[James Bottomley <jbottomley@parallels.com>]

On Thu, 2014-10-02 at 14:14 +0200, Hannes Reinecke wrote:
> On 10/02/2014 02:05 PM, James Bottomley wrote:
> > On Thu, 2014-10-02 at 08:51 +0200, Hannes Reinecke wrote:
> >> On 10/01/2014 11:10 PM, Christoph Hellwig wrote:
> >>> On Wed, Oct 01, 2014 at 02:51:31PM -0400, Webb Scales wrote:
> >>>> Hannes,
> >>>>
> >>>> In megasas_change_queue_type(), is it possible for sdev->queue_depth to be
> >>>> greater than 256?
> >>>>
> >>>> Unless I'm misunderstanding the SCSI code, we can request a
> >>>> queue-depth/tag-map-size which is greater than 256, but, since the
> >>>> scsi_cmnd::tag field is an unsigned char, depths greater than 256 may
> >>>> overflow the field when high-numbered tags are used...do I have that right?
> >>>
> >>> Yes, we really need to increase the size of the tag field.  SAM allows
> >>> a transport specific limit of up to 64 _bytes_ for it, although I don't
> >>> know implementation that large.  Given that the block layer can generate
> >>> up to 32-bit tags both for the old blk-tag.c code and the new
> >>> blk-mq-tag.c version it would be good to use a u32 there.  Can you send
> >>> me a patch?
> >>>
> >> Weeelll ...
> >>
> >> I'm afraid it's not _that_ easy.
> >> SCSI-II tagged queueing has some specific tag values:
> >>
> >> #define SIMPLE_QUEUE_TAG    0x20
> >> #define HEAD_OF_QUEUE_TAG   0x21
> >> #define ORDERED_QUEUE_TAG   0x22
> >>
> >> drivers/scsi/vmw_pvscsi.c:
> >> 	e->tag = SIMPLE_QUEUE_TAG;
> >> 	if (sdev->tagged_supported &&
> >> 	    (cmd->tag == HEAD_OF_QUEUE_TAG ||
> >> 	     cmd->tag == ORDERED_QUEUE_TAG))
> >> 		e->tag = cmd->tag;
> > 
> > A SCSI-2 tag is a SPI two byte message.  The first byte is the message
> > type.  The values you have above identify the message type for simple,
> > ordered and head of queue tags.  The *second* byte is the tag value.
> > 
> > See page 55 and 56 of the SCSI-2 standard.  There's no connection (or
> > shouldn't be) between the message type and the tag value, so it looks
> > like a bug in the pvscsi driver.
> > 
> It is. I've already sent a patch.
> 
> But that just proves the scmd->tag is essentially a duplicate
> and we should be removing it.

Yes, it was the original tag field, which then got replaced by the block
one and the accessor commands.  scsi_populate_tag_msg (which is really
an SPI construct) meant that drivers fully adopting the model didn't
even need to look at cmd->tag any more.  I'm fine with dropping it in
favour of req->tag.

James