Re: tag handling refactor V2

[Jens Axboe <axboe@fb.com>]

On 2014-11-10 08:56, Christoph Hellwig wrote:
> The current SCSI handling suffers from large amounts of duplicate code, and
> a general confusion of multiple concepts of tagging.
>
> This series tries to reduce the amount of code, and introduce two separate
> clear concepts of tagging:
>
>   a) a driver can request block-level tagging to always have a valid index
>      assigned to cmd->request->tag by the block layer.  This is already
>      automatically done for the blk-mq case, but this gives the optional
>      legacy request taggign similar semantics.
>   b) the concept of a SCSI "tagged command" is now entirely separate from
>      providing an actual tag, and always provided in a flag in the SCSI
>      command.  The driver does not need to opt-in this information, but
>      it doesn't have to use it.
>
> While this series removes 750 lines of code and provides a much cleaner driver
> API it also opens up new questions:
>
>   - how is the change_queue_type API supposed to be used for most drivers?  It
>     only changes the tag type from none to simple or back, but except for the
>     special implementation in the 53c700 driver doesn't change the queue depth,
>     which might cause it to issue multiple non-tagged command.  Fortunately
>     most drivers never look at this information, but then again changing the
>     queue type is useless to start with.
>   - for those drivers looking at the command tagged information we'd need
>     to quiesce the LUN.  No driver but the 53c700 driver does that, and the
>     53c700 does it at a target-level, which despite a comment claiming it's
>     needed doesn't seem to make sense given the code.  If we can make sure
>     to quience all LUNs we could avoid the per-command flag for tagged commands
>     and always look at the scsi_device flag.
>   - similarly, do we need any synchronization when answering a tag message reject?
>   - queue ramp down may drop to untagged mode, but queue ramp up never reverseѕ
>     that.
>   - the ->tagged_supported and ->simple_tags flags are bit-fields without clear
>     locking protection.  We probably want to move these and similar fields to
>     a proper atomic bit flags member.
>   - should the MSG_*_TAG fields move to the SPI transport class?  That's where
>     they are defined, and besides SPI drivers they are only (ab)used by the
>     target code now.
>
> Changes since V1:
>   - small fixes for various review comments

Looks good to me, nice cleanup and the diffstat is excellent :-)

-- 
Jens Axboe

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