[PATCH v2 0/7] scsi: sd/sd_zbc cleanup and improvements

[PATCH v2 0/7] scsi: sd/sd_zbc cleanup and improvements

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

This series of small patches cleanup code mainly in sd.c and sd_zbc.c, with
another small improvement in scsi_error.c.

Only the last patch of this series introduces a functional change. All other
patches do not change any functionality.

Changes from v1:
* Dropped white space patch and white space changes in other patches as
  requested by Bart.
* Dropped patch changing handling of sense_valid in sd_done() as requested by
  James.
* Fixed patch 4 (function declaration) to remove checkscript warnings

Bart Van Assche (1):
  sd_zbc: Remove superfluous assignments

Damien Le Moal (6):
  sd: Fix functions description
  sd: Improve sd_completed_bytes
  sd: Cleanup sd_done sense data handling
  scsi: Improve scsi_get_sense_info_fld
  sd_zbc: Rename sd_zbc_setup_write_cmnd
  sd_zbc: Do not write lock zones for reset

 drivers/scsi/scsi_error.c | 38 ++++++++-----------
 drivers/scsi/sd.c         | 97 ++++++++++++++++++++++++-----------------------
 drivers/scsi/sd.h         |  8 ++--
 drivers/scsi/sd_zbc.c     | 60 +++++++++++------------------
 include/scsi/scsi_eh.h    |  4 +-
 5 files changed, 93 insertions(+), 114 deletions(-)

-- 
2.9.3

[PATCH v2 1/7] sd: Fix functions description

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Fix argument names and description of functions documentation comments.
No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 00b168b..ce62d2c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -705,11 +705,10 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 
 /**
  * sd_setup_discard_cmnd - unmap blocks on thinly provisioned device
- * @sdp: scsi device to operate on
- * @rq: Request to prepare
+ * @cmd: command to prepare
  *
- * Will issue either UNMAP or WRITE SAME(16) depending on preference
- * indicated by target device.
+ * Will setup either UNMAP, WRITE SAME(16) or WRITE SAME(10) depending
+ * on the provisioning mode of the target device.
  **/
 static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 {
@@ -827,8 +826,8 @@ static void sd_config_write_same(struct scsi_disk *sdkp)
  * sd_setup_write_same_cmnd - write the same data to multiple blocks
  * @cmd: command to prepare
  *
- * Will issue either WRITE SAME(10) or WRITE SAME(16) depending on
- * preference indicated by target device.
+ * Will setup either WRITE SAME(10) or WRITE SAME(16) depending on
+ * the preference indicated by the target device.
  **/
 static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 {
@@ -1190,8 +1189,8 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
 
 /**
  *	sd_open - open a scsi disk device
- *	@inode: only i_rdev member may be used
- *	@filp: only f_mode and f_flags may be used
+ *	@bdev: Block device of the scsi disk to open
+ *	@mode: FMODE_* mask
  *
  *	Returns 0 if successful. Returns a negated errno value in case 
  *	of error.
@@ -1267,8 +1266,8 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
 /**
  *	sd_release - invoked when the (last) close(2) is called on this
  *	scsi disk.
- *	@inode: only i_rdev member may be used
- *	@filp: only f_mode and f_flags may be used
+ *	@disk: disk to release
+ *	@mode: FMODE_* mask
  *
  *	Returns 0. 
  *
@@ -1324,8 +1323,8 @@ static int sd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 
 /**
  *	sd_ioctl - process an ioctl
- *	@inode: only i_rdev/i_bdev members may be used
- *	@filp: only f_mode and f_flags may be used
+ *	@bdev: target block device
+ *	@mode: FMODE_* mask
  *	@cmd: ioctl command number
  *	@arg: this is third argument given to ioctl(2) system call.
  *	Often contains a pointer.
@@ -2713,7 +2712,7 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
 
 /**
  * sd_read_block_limits - Query disk device for preferred I/O sizes.
- * @disk: disk to query
+ * @sdkp: disk to query
  */
 static void sd_read_block_limits(struct scsi_disk *sdkp)
 {
@@ -2779,7 +2778,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 /**
  * sd_read_block_characteristics - Query block dev. characteristics
- * @disk: disk to query
+ * @sdkp: disk to query
  */
 static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 {
@@ -2827,7 +2826,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 
 /**
  * sd_read_block_provisioning - Query provisioning VPD page
- * @disk: disk to query
+ * @sdkp: disk to query
  */
 static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 {
-- 
2.9.3

Re: [PATCH v2 1/7] sd: Fix functions description

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 2/7] sd: Improve sd_completed_bytes

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Re-shuffle the code to be more efficient by not initializing
variables upfront (i.e. do it only when necessary).
Also replace the do_div calls with calls to sectors_to_logical().

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index ce62d2c..10c7657 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1758,41 +1758,45 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 
 static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
 {
-	u64 start_lba = blk_rq_pos(scmd->request);
-	u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
-	u64 factor = scmd->device->sector_size / 512;
-	u64 bad_lba;
-	int info_valid;
+	struct request *req = scmd->request;
+	struct scsi_device *sdev = scmd->device;
+	unsigned int transferred, good_bytes;
+	u64 start_lba, end_lba, bad_lba;
+
 	/*
-	 * resid is optional but mostly filled in.  When it's unused,
-	 * its value is zero, so we assume the whole buffer transferred
+	 * Some commands have a payload smaller than the device logical
+	 * block size (e.g. INQUIRY on a 4K disk).
 	 */
-	unsigned int transferred = scsi_bufflen(scmd) - scsi_get_resid(scmd);
-	unsigned int good_bytes;
-
-	info_valid = scsi_get_sense_info_fld(scmd->sense_buffer,
-					     SCSI_SENSE_BUFFERSIZE,
-					     &bad_lba);
-	if (!info_valid)
+	if (scsi_bufflen(scmd) <= sdev->sector_size)
 		return 0;
 
-	if (scsi_bufflen(scmd) <= scmd->device->sector_size)
+	/* Check if we have a 'bad_lba' information */
+	if (!scsi_get_sense_info_fld(scmd->sense_buffer,
+				     SCSI_SENSE_BUFFERSIZE,
+				     &bad_lba))
 		return 0;
 
-	/* be careful ... don't want any overflows */
-	do_div(start_lba, factor);
-	do_div(end_lba, factor);
-
-	/* The bad lba was reported incorrectly, we have no idea where
+	/*
+	 * If the bad lba was reported incorrectly, we have no idea where
 	 * the error is.
 	 */
-	if (bad_lba < start_lba  || bad_lba >= end_lba)
+	start_lba = sectors_to_logical(sdev, blk_rq_pos(req));
+	end_lba = sectors_to_logical(sdev,
+				blk_rq_pos(req) + (scsi_bufflen(scmd) >> 9));
+	if (bad_lba < start_lba || bad_lba >= end_lba)
 		return 0;
 
+	/*
+	 * resid is optional but mostly filled in.  When it's unused,
+	 * its value is zero, so we assume the whole buffer transferred
+	 */
+	transferred = scsi_bufflen(scmd) - scsi_get_resid(scmd);
+
 	/* This computation should always be done in terms of
 	 * the resolution of the device's medium.
 	 */
-	good_bytes = (bad_lba - start_lba) * scmd->device->sector_size;
+	good_bytes = logical_to_bytes(sdev, bad_lba - start_lba);
+
 	return min(good_bytes, transferred);
 }
 
-- 
2.9.3

Re: [PATCH v2 2/7] sd: Improve sd_completed_bytes

[Christoph Hellwig]

On Mon, Apr 24, 2017 at 04:51:10PM +0900, damien.lemoal@wdc.com wrote:
> From: Damien Le Moal <damien.lemoal@wdc.com>
> 
> Re-shuffle the code to be more efficient by not initializing
> variables upfront (i.e. do it only when necessary).
> Also replace the do_div calls with calls to sectors_to_logical().
> 
> No functional change is introduced by this patch.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 3/7] sd: Cleanup sd_done sense data handling

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

In sd_done(), for the ILLEGAL REQUEST sense key case, add an 'else if'
after the first 'if (sshdr.asc == 0x10)' test to avoid the second test
(the values tested are different).

Still for the same ILLEGAL REQUEST case, move the declarations of the
variables 'op' and 'unmap' within the scope of the 'else if' case since
these variables are only used there. At the same time, remove the
unnecessary good_bytes 0 assignment as this code can only be executed
with result != 0 and good_bytes is already set to 0 in that case.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>
---
 drivers/scsi/sd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 10c7657..2d1ac5c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1816,8 +1816,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	struct request *req = SCpnt->request;
 	int sense_valid = 0;
 	int sense_deferred = 0;
-	unsigned char op = SCpnt->cmnd[0];
-	unsigned char unmap = SCpnt->cmnd[1] & 8;
 
 	switch (req_op(req)) {
 	case REQ_OP_DISCARD:
@@ -1875,10 +1873,14 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 			good_bytes = sd_completed_bytes(SCpnt);
 		break;
 	case ILLEGAL_REQUEST:
-		if (sshdr.asc == 0x10)  /* DIX: Host detected corruption */
+		if (sshdr.asc == 0x10) {
+			/* DIX: Host detected corruption */
 			good_bytes = sd_completed_bytes(SCpnt);
-		/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
-		if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
+		} else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
+			/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
+			unsigned char op = SCpnt->cmnd[0];
+			unsigned char unmap = SCpnt->cmnd[1] & 8;
+
 			switch (op) {
 			case UNMAP:
 				sd_config_discard(sdkp, SD_LBP_DISABLE);
@@ -1890,8 +1892,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 				else {
 					sdkp->device->no_write_same = 1;
 					sd_config_write_same(sdkp);
-
-					good_bytes = 0;
 					req->__data_len = blk_rq_bytes(req);
 					req->rq_flags |= RQF_QUIET;
 				}
-- 
2.9.3

Re: [PATCH v2 3/7] sd: Cleanup sd_done sense data handling

[Christoph Hellwig]

I would just use a switch and kill the op and unmap variables entirely,
e.g. something like the patch below:

---
From: Christoph Hellwig <hch@lst.de>
Subject: sd: Cleanup sd_done sense data handling

Use a switch for the sense key, and remove two pointless variables
that are only used once.

Signed-off-by: Christoph Hellwig <hch@lst.de>
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8cf34a8e3eea..eec695107c99 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1866,8 +1866,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	struct request *req = SCpnt->request;
 	int sense_valid = 0;
 	int sense_deferred = 0;
-	unsigned char op = SCpnt->cmnd[0];
-	unsigned char unmap = SCpnt->cmnd[1] & 8;
 
 	switch (req_op(req)) {
 	case REQ_OP_DISCARD:
@@ -1941,19 +1939,21 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 			good_bytes = sd_completed_bytes(SCpnt);
 		break;
 	case ILLEGAL_REQUEST:
-		if (sshdr.asc == 0x10)  /* DIX: Host detected corruption */
+		switch (sshdr.asc) {
+		case 0x10:	/* DIX: Host detected corruption */
 			good_bytes = sd_completed_bytes(SCpnt);
-		/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
-		if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
-			switch (op) {
+			break;
+		case 0x20:	/* INVALID COMMAND OPCODE */
+		case 0x24:	/* INVALID FIELD IN CDB */
+			switch (SCpnt->cmnd[0]) {
 			case UNMAP:
 				sd_config_discard(sdkp, SD_LBP_DISABLE);
 				break;
 			case WRITE_SAME_16:
 			case WRITE_SAME:
-				if (unmap)
+				if (SCpnt->cmnd[1] & 8) {
 					sd_config_discard(sdkp, SD_LBP_DISABLE);
-				else {
+				} else {
 					sdkp->device->no_write_same = 1;
 					sd_config_write_same(sdkp);
 
@@ -1961,6 +1961,7 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 					req->__data_len = blk_rq_bytes(req);
 					req->rq_flags |= RQF_QUIET;
 				}
+				break;
 			}
 		}
 		break;

Re: [PATCH v2 3/7] sd: Cleanup sd_done sense data handling

[Martin K. Petersen]

Christoph,

> Use a switch for the sense key, and remove two pointless variables
> that are only used once.

> -				if (unmap)

The rationale behind the unmap variable was clarity and avoiding magic
values.

I'm OK with this, however:

> +				if (SCpnt->cmnd[1] & 8) { /* UNMAP */

So I committed with that change.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/7] sd: Cleanup sd_done sense data handling

[Damien Le Moal]

Martin,

On 4/25/17 08:11, Martin K. Petersen wrote:
> 
> Christoph,
> 
>> Use a switch for the sense key, and remove two pointless variables
>> that are only used once.
> 
>> -				if (unmap)
> 
> The rationale behind the unmap variable was clarity and avoiding magic
> values.
> 
> I'm OK with this, however:
> 
>> +				if (SCpnt->cmnd[1] & 8) { /* UNMAP */
> 
> So I committed with that change.

Thank you for the commit.

Just one remark:
You left the "good_bytes = 0;" in the else statement of the
ILLEGAL_REQUEST && asc == 0x24 && command == WRITE_SAME case. Is it
really necessary ?

good_bytes is already set to 0 at the beginning of sd_done() when result
!= 0 and again within the initial switch-case for REQ_OP_DISCARD and
REQ_OP_WRITE_SAME cases, which are the only commands that can lead to
hitting that "else" part in the sense data processing.
I may be missing something, but I think that that assignment is redundant.

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

Re: [PATCH v2 3/7] sd: Cleanup sd_done sense data handling

[Martin K. Petersen]

Damien,

> good_bytes is already set to 0 at the beginning of sd_done() when
> result != 0 and again within the initial switch-case for
> REQ_OP_DISCARD and REQ_OP_WRITE_SAME cases, which are the only
> commands that can lead to hitting that "else" part in the sense data
> processing.  I may be missing something, but I think that that
> assignment is redundant.

Fixed this up, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v2 4/7] scsi: Improve scsi_get_sense_info_fld

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Use get_unaligned_be32 and get_unaligned_be64 to obtain values from
the sense buffer instead of open coding the operations.
Also change the function return value to a bool and fix the function
signature declaration to remove spaces triggering checkpatch warnings.

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>
---
 drivers/scsi/scsi_error.c | 38 +++++++++++++++-----------------------
 include/scsi/scsi_eh.h    |  4 ++--
 2 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 53e3343..d70c67c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -46,6 +46,8 @@
 
 #include <trace/events/scsi.h>
 
+#include <asm/unaligned.h>
+
 static void scsi_eh_done(struct scsi_cmnd *scmd);
 
 /*
@@ -2361,44 +2363,34 @@ EXPORT_SYMBOL(scsi_command_normalize_sense);
  *			field will be placed if found.
  *
  * Return value:
- *	1 if information field found, 0 if not found.
+ *	true if information field found, false if not found.
  */
-int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
-			    u64 * info_out)
+bool scsi_get_sense_info_fld(const u8 *sense_buffer, int sb_len,
+			     u64 *info_out)
 {
-	int j;
 	const u8 * ucp;
-	u64 ull;
 
 	if (sb_len < 7)
-		return 0;
+		return false;
 	switch (sense_buffer[0] & 0x7f) {
 	case 0x70:
 	case 0x71:
 		if (sense_buffer[0] & 0x80) {
-			*info_out = (sense_buffer[3] << 24) +
-				    (sense_buffer[4] << 16) +
-				    (sense_buffer[5] << 8) + sense_buffer[6];
-			return 1;
-		} else
-			return 0;
+			*info_out = get_unaligned_be32(&sense_buffer[3]);
+			return true;
+		}
+		return false;
 	case 0x72:
 	case 0x73:
 		ucp = scsi_sense_desc_find(sense_buffer, sb_len,
 					   0 /* info desc */);
 		if (ucp && (0xa == ucp[1])) {
-			ull = 0;
-			for (j = 0; j < 8; ++j) {
-				if (j > 0)
-					ull <<= 8;
-				ull |= ucp[4 + j];
-			}
-			*info_out = ull;
-			return 1;
-		} else
-			return 0;
+			*info_out = get_unaligned_be64(&ucp[4]);
+			return true;
+		}
+		return false;
 	default:
-		return 0;
+		return false;
 	}
 }
 EXPORT_SYMBOL(scsi_get_sense_info_fld);
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index a25b328..64d30d8 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -23,8 +23,8 @@ static inline bool scsi_sense_is_deferred(const struct scsi_sense_hdr *sshdr)
 	return ((sshdr->response_code >= 0x70) && (sshdr->response_code & 1));
 }
 
-extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
-				   u64 * info_out);
+extern bool scsi_get_sense_info_fld(const u8 *sense_buffer, int sb_len,
+				    u64 *info_out);
 
 extern int scsi_ioctl_reset(struct scsi_device *, int __user *);
 
-- 
2.9.3

Re: [PATCH v2 4/7] scsi: Improve scsi_get_sense_info_fld

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 5/7] sd_zbc: Rename sd_zbc_setup_write_cmnd

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Rename sd_zbc_setup_write_cmnd() to sd_zbc_write_lock_zone() to be
clear about what the function actually does. To be consistent, also
rename sd_zbc_cancel_write_cmnd() to sd_zbc_write_unlock_zone().

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>
---
 drivers/scsi/sd.c     |  6 +++---
 drivers/scsi/sd.h     |  8 ++++----
 drivers/scsi/sd_zbc.c | 12 ++++--------
 3 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2d1ac5c..25a4ab1 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -846,7 +846,7 @@ static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 	BUG_ON(bio_offset(bio) || bio_iovec(bio).bv_len != sdp->sector_size);
 
 	if (sd_is_zoned(sdkp)) {
-		ret = sd_zbc_setup_write_cmnd(cmd);
+		ret = sd_zbc_write_lock_zone(cmd);
 		if (ret != BLKPREP_OK)
 			return ret;
 	}
@@ -918,7 +918,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	unsigned char protect;
 
 	if (zoned_write) {
-		ret = sd_zbc_setup_write_cmnd(SCpnt);
+		ret = sd_zbc_write_lock_zone(SCpnt);
 		if (ret != BLKPREP_OK)
 			return ret;
 	}
@@ -1145,7 +1145,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	ret = BLKPREP_OK;
  out:
 	if (zoned_write && ret != BLKPREP_OK)
-		sd_zbc_cancel_write_cmnd(SCpnt);
+		sd_zbc_write_unlock_zone(SCpnt);
 
 	return ret;
 }
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 0cf9680..2044e07a 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -267,8 +267,8 @@ static inline int sd_is_zoned(struct scsi_disk *sdkp)
 extern int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buffer);
 extern void sd_zbc_remove(struct scsi_disk *sdkp);
 extern void sd_zbc_print_zones(struct scsi_disk *sdkp);
-extern int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd);
-extern void sd_zbc_cancel_write_cmnd(struct scsi_cmnd *cmd);
+extern int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd);
+extern void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd);
 extern int sd_zbc_setup_report_cmnd(struct scsi_cmnd *cmd);
 extern int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd);
 extern void sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
@@ -286,13 +286,13 @@ static inline void sd_zbc_remove(struct scsi_disk *sdkp) {}
 
 static inline void sd_zbc_print_zones(struct scsi_disk *sdkp) {}
 
-static inline int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd)
+static inline int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd)
 {
 	/* Let the drive fail requests */
 	return BLKPREP_OK;
 }
 
-static inline void sd_zbc_cancel_write_cmnd(struct scsi_cmnd *cmd) {}
+static inline void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd) {}
 
 static inline int sd_zbc_setup_report_cmnd(struct scsi_cmnd *cmd)
 {
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 92620c8..a792682 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -269,7 +269,7 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
 	return BLKPREP_OK;
 }
 
-int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd)
+int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd)
 {
 	struct request *rq = cmd->request;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
@@ -303,8 +303,9 @@ int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd)
 	return BLKPREP_OK;
 }
 
-static void sd_zbc_unlock_zone(struct request *rq)
+void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd)
 {
+	struct request *rq = cmd->request;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 
 	if (sdkp->zones_wlock) {
@@ -315,11 +316,6 @@ static void sd_zbc_unlock_zone(struct request *rq)
 	}
 }
 
-void sd_zbc_cancel_write_cmnd(struct scsi_cmnd *cmd)
-{
-	sd_zbc_unlock_zone(cmd->request);
-}
-
 void sd_zbc_complete(struct scsi_cmnd *cmd,
 		     unsigned int good_bytes,
 		     struct scsi_sense_hdr *sshdr)
@@ -333,7 +329,7 @@ void sd_zbc_complete(struct scsi_cmnd *cmd,
 	case REQ_OP_ZONE_RESET:
 
 		/* Unlock the zone */
-		sd_zbc_unlock_zone(rq);
+		sd_zbc_write_unlock_zone(cmd);
 
 		if (!result ||
 		    sshdr->sense_key != ILLEGAL_REQUEST)
-- 
2.9.3

Re: [PATCH v2 5/7] sd_zbc: Rename sd_zbc_setup_write_cmnd

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 6/7] sd_zbc: Remove superfluous assignments

[damien.lemoal]

From: Bart Van Assche <bart.vanassche@sandisk.com>

A value is assigned to the variable 'capacity' in sd_zbc_read_zones()
but that value is never used. Hence remove the variable 'capacity'.

[Damien: There is no need to initialize to 0 the variable 'ret'
in sd_zbc_read_zones()]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd_zbc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index a792682..29ba1d7 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -560,8 +560,7 @@ static int sd_zbc_setup(struct scsi_disk *sdkp)
 int sd_zbc_read_zones(struct scsi_disk *sdkp,
 		      unsigned char *buf)
 {
-	sector_t capacity;
-	int ret = 0;
+	int ret;
 
 	if (!sd_is_zoned(sdkp))
 		/*
@@ -593,7 +592,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp,
 	ret = sd_zbc_check_capacity(sdkp, buf);
 	if (ret)
 		goto err;
-	capacity = logical_to_sectors(sdkp->device, sdkp->capacity);
 
 	/*
 	 * Check zone size: only devices with a constant zone size (except
-- 
2.9.3

Re: [PATCH v2 6/7] sd_zbc: Remove superfluous assignments

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 7/7] sd_zbc: Do not write lock zones for reset

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Resetting a zone write pointer is equivalent to discarding sectors:
after a reset, the zone sectors will contain zeros (or the format
pattern). So there is no need for mutual exclusion between a zone reset
and write. Similarly to discard, make it the responsability of the user
to properly synchronize between reset and write (as is done now for
discard and write).

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd_zbc.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 29ba1d7..fcf0fad 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -237,7 +237,6 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 	sector_t sector = blk_rq_pos(rq);
 	sector_t block = sectors_to_logical(sdkp->device, sector);
-	unsigned int zno = block >> sdkp->zone_shift;
 
 	if (!sd_is_zoned(sdkp))
 		/* Not a zoned device */
@@ -250,11 +249,6 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
 		/* Unaligned request */
 		return BLKPREP_KILL;
 
-	/* Do not allow concurrent reset and writes */
-	if (sdkp->zones_wlock &&
-	    test_and_set_bit(zno, sdkp->zones_wlock))
-		return BLKPREP_DEFER;
-
 	cmd->cmd_len = 16;
 	memset(cmd->cmnd, 0, cmd->cmd_len);
 	cmd->cmnd[0] = ZBC_OUT;
@@ -324,38 +318,34 @@ void sd_zbc_complete(struct scsi_cmnd *cmd,
 	struct request *rq = cmd->request;
 
 	switch (req_op(rq)) {
+	case REQ_OP_ZONE_RESET:
+
+		if (result &&
+		    sshdr->sense_key == ILLEGAL_REQUEST &&
+		    sshdr->asc == 0x24)
+			/*
+			 * INVALID FIELD IN CDB error: reset of a conventional
+			 * zone was attempted. Nothing to worry about, so be
+			 * quiet about the error.
+			 */
+			rq->rq_flags |= RQF_QUIET;
+		break;
+
 	case REQ_OP_WRITE:
 	case REQ_OP_WRITE_SAME:
-	case REQ_OP_ZONE_RESET:
 
 		/* Unlock the zone */
 		sd_zbc_write_unlock_zone(cmd);
 
-		if (!result ||
-		    sshdr->sense_key != ILLEGAL_REQUEST)
-			break;
-
-		switch (sshdr->asc) {
-		case 0x24:
-			/*
-			 * INVALID FIELD IN CDB error: For a zone reset,
-			 * this means that a reset of a conventional
-			 * zone was attempted. Nothing to worry about in
-			 * this case, so be quiet about the error.
-			 */
-			if (req_op(rq) == REQ_OP_ZONE_RESET)
-				rq->rq_flags |= RQF_QUIET;
-			break;
-		case 0x21:
+		if (result &&
+		    sshdr->sense_key == ILLEGAL_REQUEST &&
+		    sshdr->asc == 0x21)
 			/*
 			 * INVALID ADDRESS FOR WRITE error: It is unlikely that
 			 * retrying write requests failed with any kind of
 			 * alignement error will result in success. So don't.
 			 */
 			cmd->allowed = 0;
-			break;
-		}
-
 		break;
 
 	case REQ_OP_ZONE_REPORT:
-- 
2.9.3

Re: [PATCH v2 7/7] sd_zbc: Do not write lock zones for reset

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>