linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* tag handling refactor V2
@ 2014-11-10 15:56 Christoph Hellwig
  2014-11-10 15:56 ` [PATCH 01/11] scsi: provide a generic change_queue_type method Christoph Hellwig
                   ` (14 more replies)
  0 siblings, 15 replies; 27+ messages in thread
From: Christoph Hellwig @ 2014-11-10 15:56 UTC (permalink / raw)
  To: linux-scsi
  Cc: James Bottomley, Robert Elliott, Hannes Reinecke,
	Martin K. Petersen, Bart van Assche, Jens Axboe, Kashyap Desai,
	Sreekanth Reddy, Mike Christie, Guennadi Liakhovetski,
	usb-storage

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

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

^ permalink raw reply	[flat|nested] 27+ messages in thread
* tag handling refactor
@ 2014-11-04  7:54 Christoph Hellwig
  2014-11-04  7:54 ` [PATCH 09/11] scsi: don't set tagging state from scsi_adjust_queue_depth Christoph Hellwig
  0 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2014-11-04  7:54 UTC (permalink / raw)
  To: linux-scsi
  Cc: James Bottomley, Robert Elliott, Hannes Reinecke,
	Martin K. Petersen, Bart van Assche, Jens Axboe, Kashyap Desai,
	Sreekanth Reddy, Mike Christie, Guennadi Liakhovetski,
	usb-storage

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.

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2014-11-13 11:28 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-10 15:56 tag handling refactor V2 Christoph Hellwig
2014-11-10 15:56 ` [PATCH 01/11] scsi: provide a generic change_queue_type method Christoph Hellwig
2014-11-10 15:56 ` [PATCH 02/11] scsi: add new scsi-command flag for tagged commands Christoph Hellwig
2014-11-10 15:56 ` [PATCH 03/11] scsi: remove ordered_tags scsi_device field Christoph Hellwig
2014-11-11 15:37   ` Hannes Reinecke
2014-11-10 15:56 ` [PATCH 04/11] scsi: remove ordered_tag host template field Christoph Hellwig
2014-11-10 15:56 ` [PATCH 05/11] scsi: remove abuses of scsi_populate_tag Christoph Hellwig
2014-11-11 15:39   ` Hannes Reinecke
2014-11-10 15:56 ` [PATCH 06/11] mptfusion: don't change queue type in ->change_queue_depth Christoph Hellwig
2014-11-10 15:56 ` [PATCH 07/11] scsi: remove use_blk_tcq Scsi_Host field Christoph Hellwig
2014-11-10 15:56 ` [PATCH 08/11] scsi: always assign block layer tags if enabled Christoph Hellwig
2014-11-10 15:56 ` [PATCH 09/11] scsi: don't set tagging state from scsi_adjust_queue_depth Christoph Hellwig
2014-11-11 15:41   ` Hannes Reinecke
2014-11-10 15:56 ` [PATCH 10/11] scsi: don't force tagged_supported in drivers Christoph Hellwig
2014-11-11 15:42   ` Hannes Reinecke
2014-11-10 15:56 ` [PATCH 11/11] ufs: remove spurious scsi_set_tag_type call Christoph Hellwig
2014-11-11 15:42   ` Hannes Reinecke
2014-11-10 18:06 ` tag handling refactor V2 Mike Christie
2014-11-10 22:28 ` Jens Axboe
2014-11-12 17:41 ` making the queue_type attribute read only, was: " Christoph Hellwig
2014-11-13  0:33   ` Elliott, Robert (Server Storage)
2014-11-13 11:28     ` Christoph Hellwig
2014-11-13  1:15 ` James Bottomley
  -- strict thread matches above, loose matches on Subject: below --
2014-11-04  7:54 tag handling refactor Christoph Hellwig
2014-11-04  7:54 ` [PATCH 09/11] scsi: don't set tagging state from scsi_adjust_queue_depth Christoph Hellwig
2014-11-04  9:26   ` Hannes Reinecke
2014-11-04 10:47     ` Christoph Hellwig
2014-11-05  2:50   ` Martin K. Petersen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).