Re: [PATCH] sd_zbc: Write unlock zone from sd_uninit_cmnd()

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Wed, 2017-08-09 at 14:15 +0900, Damien Le Moal wrote:
> Overall, yes, that is possible. However, considering only write commands
> to a single zone, since at any time only one command is dequeued (the
> one holding the zone lock), having the "lock+dequeue" and
> "unlock+requeue" both atomic prevent any reordering of write commands
> directed to that zone. The first command in the queue for the zone will
> either (1) get the zone lock and be dequeued OR (2) stay in the queue.
> In case (1), the next write for the zone will hit case (2) until the
> dispatch command completes. If the dispatch command is requeued (at the
> dispatch queue head), we hit back again the (1) or (2) cases, without
> any change in the order. Isnt't it ?

Hello Damien,

Commands that get requeued are not reinserted at their original position
in the request queue but usually at a different position. This is what makes
requeueing cause request reordering. Anyway, can you check whether replacing
patch "sd_zbc: Write unlock zone from sd_uninit_cmnd()" with the (untested)
patch below does not trigger any lockups and prevents request reordering?
Since that patch keeps the zone lock across requeuing attempts it should
prevent request reordering.

Thanks,

Bart.

---
 drivers/scsi/sd_zbc.c    | 8 ++++++++
 include/scsi/scsi_cmnd.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 96855df9f49d..6d18b1bd3b26 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -290,10 +290,15 @@ int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd)
 	 * threads running on different CPUs write to the same
 	 * zone (with a synchronized sequential pattern).
 	 */
+	if (cmd->flags & SCMD_IN_PROGRESS)
+		return BLKPREP_OK;
+
 	if (sdkp->zones_wlock &&
 	    test_and_set_bit(zno, sdkp->zones_wlock))
 		return BLKPREP_DEFER;
 
+	cmd->flags |= SCMD_IN_PROGRESS;
+
 	return BLKPREP_OK;
 }
 
@@ -302,6 +307,9 @@ void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd)
 	struct request *rq = cmd->request;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 
+	WARN_ON_ONCE(!(cmd->flags & SCMD_IN_PROGRESS));
+	cmd->flags &= ~SCMD_IN_PROGRESS;
+
 	if (sdkp->zones_wlock) {
 		unsigned int zno = sd_zbc_zone_no(sdkp, blk_rq_pos(rq));
 		WARN_ON_ONCE(!test_bit(zno, sdkp->zones_wlock));
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a1266d318c85..f18c812094b5 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -57,6 +57,9 @@ struct scsi_pointer {
 /* for scmd->flags */
 #define SCMD_TAGGED		(1 << 0)
 #define SCMD_UNCHECKED_ISA_DMA	(1 << 1)
+#ifdef CONFIG_BLK_DEV_ZONED
+#define SCMD_IN_PROGRESS	(1 << 2)
+#endif
 
 struct scsi_cmnd {
 	struct scsi_request req;