[PATCH v4 0/8] scsi_debug: Enable lock context analysis

[PATCH v4 0/8] scsi_debug: Enable lock context analysis

[Bart Van Assche]

Hi Martin,

This patch series enables compiler-based lock context analysis for the
scsi_debug driver. Conditional locking is eliminated across command response
and error injection functions so that static analysis can verify lock
acquisitions and releases. Internal locking helper functions are annotated
with __acquires(),__releases(), __acquires_shared() and __releases_shared()
attributes. An error-path lock leak in resp_write_same() is fixed. Finally,
lock context analysis is enabled in drivers/scsi/Makefile.

Changes compared to v3:
- Add a new patch to split resp_atomic_write() and eliminate conditional
  locking in atomic write handling.
- Pass the LBP state boolean down into __resp_write_same(),
  __corrupt_lbas(), and __resp_write_dt0() to avoid repeated calls to
  scsi_debug_lbp().
- Drop unused function arguments from __resp_read_dt0_dix() and
  __resp_write_dt0_dix().
- Correct the return value descriptions in the header comments of
  __resp_read_dt0_dix() and __resp_write_dt0_dix().

Changes compared to v2:
 - Moved DIX/DIF verification into a helper function (__resp_read_dt0_dix())
   to reduce indentation.
 - Removed unnecessary 'else' branches after 'return'.
 - Set the SCSI status code only if do_device_access() fails instead of
   unconditionally setting it to DID_ERROR << 16 beforehand.

Changes compared to v1:
 - Fixed an issue where the error code was clobbered in resp_write_same() and
   __resp_write_same() when fetch_to_dev_buffer() fails (John Garry and
   sashiko-bot).
 - Fixed return value handling in resp_read_dt0() and resp_write_dt0():
   previously a positive byte count returned by do_device_access() was
   incorrectly treated as a SCSI status code and returned to the caller. Now,
   the SCSI status is passed via an output pointer and only negative return
   values trigger an error return (sashiko-bot).
 - Improved lock context annotations:
   * Annotated existing rwlock_t members (&sip->macc_data_lck,
     &sip->macc_sector_lck, &sip->macc_meta_lck) directly with __acquires /
     __releases instead of defining and embedding dummy context lock structs
     in struct sdeb_store_info.
   * Dropped the unwarranted __context_unsafe() annotation from
     sdeb_data_read_lock().
   * Updated the commit descriptions to match the code changes.

Bart Van Assche (8):
  scsi: scsi_debug: Fix a locking bug in resp_write_same()
  scsi: scsi_debug: Split resp_write_same()
  scsi: scsi_debug: Split corrupt_lbas()
  scsi: scsi_debug: Split resp_read_dt0()
  scsi: scsi_debug: Split resp_write_dt0()
  scsi: scsi_debug: Split resp_atomic_write()
  scsi: scsi_debug: Improve lock context annotations
  scsi: core: Enable lock context analysis for the scsi_debug driver

 drivers/scsi/Makefile     |   1 +
 drivers/scsi/scsi_debug.c | 418 +++++++++++++++++++++++---------------
 2 files changed, 254 insertions(+), 165 deletions(-)

[PATCH v4 1/8] scsi: scsi_debug: Fix a locking bug in resp_write_same()

[Bart Van Assche]

If fetch_to_dev_buffer() fails in resp_write_same(), the function jumps
to 'out' without releasing the data write lock acquired earlier via
sdeb_data_write_lock(). Jump to 'unlock' instead so that
sdeb_data_write_unlock() is called on the error path. This bug has been
discovered by building the scsi_debug driver with Clang and modified
lock context annotations. The modified lock context annotations are
available in patch "scsi: scsi_debug: Improve lock context annotations".

Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index e679df1993aa..44b4587b53c6 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5455,7 +5455,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 
 	if (-1 == ret) {
 		ret = DID_ERROR << 16;
-		goto out;
+		goto unlock;
 	} else if (sdebug_verbose && !ndob && (ret < lb_size))
 		sdev_printk(KERN_INFO, scp->device,
 			    "%s: %s: lb size=%u, IO sent=%d bytes\n",
@@ -5472,8 +5472,9 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 	/* If ZBC zone then bump its write pointer */
 	if (sdebug_dev_is_zoned(devip))
 		zbc_inc_wp(devip, lba, num);
-	sdeb_data_write_unlock(sip);
 	ret = 0;
+unlock:
+	sdeb_data_write_unlock(sip);
 out:
 	if (meta_data_locked)
 		sdeb_meta_write_unlock(sip);

[PATCH v4 2/8] scsi: scsi_debug: Split resp_write_same()

[Bart Van Assche]

Since the Clang thread-safety analyzer does not support conditional
locking, split the function resp_write_same() such that all locking
statements end up in the same branch of an if-statement.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 44b4587b53c6..cadd94e17109 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5412,8 +5412,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 	return ret;
 }
 
-static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
-			   u32 ei_lba, bool unmap, bool ndob)
+static int __resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
+			     u32 ei_lba, bool unmap, bool ndob, bool lbp)
 {
 	struct scsi_device *sdp = scp->device;
 	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
@@ -5425,21 +5425,14 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 						scp->device->hostdata, true);
 	u8 *fs1p;
 	u8 *fsp;
-	bool meta_data_locked = false;
-	bool lbp = scsi_debug_lbp();
-
-	if (sdebug_dev_is_zoned(devip) || lbp) {
-		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
 
 	ret = check_device_access_params(scp, lba, num, true);
 	if (ret)
-		goto out;
+		return ret;
 
 	if (unmap && lbp) {
 		unmap_region(sip, lba, num);
-		goto out;
+		return ret;
 	}
 	lbaa = lba;
 	block = do_div(lbaa, sdebug_store_sectors);
@@ -5475,9 +5468,28 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 	ret = 0;
 unlock:
 	sdeb_data_write_unlock(sip);
-out:
-	if (meta_data_locked)
+	return ret;
+}
+
+static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num, u32 ei_lba,
+			   bool unmap, bool ndob)
+{
+	struct scsi_device *sdp = scp->device;
+	struct sdebug_dev_info *devip = sdp->hostdata;
+	struct sdeb_store_info *sip = devip2sip(devip, true);
+	const bool lbp = scsi_debug_lbp();
+	int ret;
+
+	if (sdebug_dev_is_zoned(devip) || lbp) {
+		sdeb_meta_write_lock(sip);
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob,
+					lbp);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob,
+					lbp);
+	}
+
 	return ret;
 }
 

[PATCH v4 3/8] scsi: scsi_debug: Split corrupt_lbas()

[Bart Van Assche]

Move the inner logic of corrupt_lbas() into a new helper function
__corrupt_lbas() and call it from separate locked and unlocked branches
in corrupt_lbas(). This patch prepares for enabling context analysis.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 42 +++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index cadd94e17109..eafcadc6ef07 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4984,37 +4984,25 @@ static int resp_write_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
-static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
-		u32 nr_bit_errors, s32 reftag_adjust)
+static int __corrupt_lbas(struct sdeb_store_info *sip, u64 lba, u32 num,
+			  u32 nr_bit_errors, s32 reftag_adjust, bool lbp)
 {
-	struct sdeb_store_info *sip = devip2sip(devip, false);
-	bool meta_data_locked = false;
-	bool lbp = scsi_debug_lbp();
 	u32 block, num_mapped, b, i;
-	int error = 0;
-
-	if (sdebug_dev_is_zoned(devip) || sdebug_dix || lbp) {
-		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
 
 	if (!sip) {
 		pr_err("can't corrupt with fake_rw\n");
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	if (num > sdebug_capacity || lba > sdebug_capacity - num) {
 		pr_err("logical blocks out of bounds: %llu:%u", lba, num);
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	if (lbp && (!map_state(sip, lba, &num_mapped) || num > num_mapped)) {
 		pr_err("can't modify unmapped logical blocks: %llu:%u",
 			lba, num);
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	/*
@@ -5052,9 +5040,25 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 	}
 	sdeb_data_unlock(sip, false);
 
-out_unlock:
-	if (meta_data_locked)
+	return 0;
+}
+
+static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
+			u32 nr_bit_errors, s32 reftag_adjust)
+{
+	struct sdeb_store_info *sip = devip2sip(devip, false);
+	const bool lbp = scsi_debug_lbp();
+	int error = 0;
+
+	if (sdebug_dev_is_zoned(devip) || sdebug_dix || lbp) {
+		sdeb_meta_write_lock(sip);
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust, lbp);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust, lbp);
+	}
 	return error;
 }
 

[PATCH v4 4/8] scsi: scsi_debug: Split resp_read_dt0()

[Bart Van Assche]

Split this function to remove conditional locking. This patch prepares
for enabling context analysis.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 110 ++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index eafcadc6ef07..c66f8556896d 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4608,6 +4608,70 @@ static int resp_read_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
+/*
+ * Returns 0 upon success or -1 with *scsi_status set to a SCSI status code upon
+ * failure.
+ */
+static int __resp_read_dt0_dix(struct scsi_cmnd *scp, u64 lba, u32 num,
+			       u32 ei_lba, int *scsi_status)
+{
+	const u8 *const cmd = scp->cmnd;
+
+	switch (prot_verify_read(scp, lba, num, ei_lba)) {
+	case 1: /* Guard tag error */
+		if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		break;
+	case 3: /* Reference tag error */
+		if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		break;
+	}
+	return 0;
+}
+
+/*
+ * Returns the number of bytes transferred, or -1 with *scsi_status set to a
+ * SCSI status code on failure.
+ */
+static int __resp_read_dt0(struct scsi_cmnd *scp, struct sdeb_store_info *sip,
+			   u64 lba, u32 num, u32 ei_lba, int *scsi_status)
+{
+	int ret;
+
+	/* DIX + T10 DIF */
+	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
+		ret = __resp_read_dt0_dix(scp, lba, num, ei_lba, scsi_status);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = do_device_access(sip, scp, 0, lba, num, 0, false, false);
+	if (ret < 0)
+		*scsi_status = DID_ERROR << 16;
+	return ret;
+}
+
 static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -4617,7 +4681,7 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	u64 lba;
 	struct sdeb_store_info *sip = devip2sip(devip, true);
 	u8 *cmd = scp->cmnd;
-	bool meta_data_locked = false;
+	int scsi_status;
 
 	switch (cmd[0]) {
 	case READ_16:
@@ -4699,48 +4763,16 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	}
 
 	if (sdebug_dev_is_zoned(devip) ||
-	    (sdebug_dix && scsi_prot_sg_count(scp)))  {
+	    (sdebug_dix && scsi_prot_sg_count(scp))) {
 		sdeb_meta_read_lock(sip);
-		meta_data_locked = true;
-	}
-
-	/* DIX + T10 DIF */
-	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		switch (prot_verify_read(scp, lba, num, ei_lba)) {
-		case 1: /* Guard tag error */
-			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return check_condition_result;
-			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return illegal_condition_result;
-			}
-			break;
-		case 3: /* Reference tag error */
-			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-				      LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return check_condition_result;
-			} else if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-				      LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return illegal_condition_result;
-			}
-			break;
-		}
+		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba, &scsi_status);
+		sdeb_meta_read_unlock(sip);
+	} else {
+		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba, &scsi_status);
 	}
 
-	ret = do_device_access(sip, scp, 0, lba, num, 0, false, false);
-	if (meta_data_locked)
-		sdeb_meta_read_unlock(sip);
 	if (unlikely(ret == -1))
-		return DID_ERROR << 16;
+		return scsi_status;
 
 	scsi_set_resid(scp, scsi_bufflen(scp) - ret);
 

[PATCH v4 5/8] scsi: scsi_debug: Split resp_write_dt0()

[Bart Van Assche]

Prepare for enabling lock context analysis by eliminating conditional
locking.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 139 ++++++++++++++++++++++++--------------
 1 file changed, 87 insertions(+), 52 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index c66f8556896d..685949aedf9a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5094,6 +5094,85 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 	return error;
 }
 
+/*
+ * Returns 0 upon success or -1 with *scsi_status set to a SCSI status code upon
+ * failure.
+ */
+static int __resp_write_dt0_dix(struct scsi_cmnd *scp,
+				u64 lba, u32 num,
+				u32 ei_lba, int *scsi_status)
+{
+	switch (prot_verify_write(scp, lba, num, ei_lba)) {
+	case 1: /* Guard tag error */
+		if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		break;
+	case 3: /* Reference tag error */
+		if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		break;
+	}
+
+	return 0;
+}
+
+/*
+ * Returns the number of bytes transferred, or -1 with *scsi_status set to a
+ * SCSI status code on failure.
+ */
+static int __resp_write_dt0(struct scsi_cmnd *scp,
+			    struct sdebug_dev_info *devip,
+			    struct sdeb_store_info *sip, u64 lba, u32 num,
+			    u32 ei_lba, u8 group, bool lbp, int *scsi_status)
+{
+	int ret;
+
+	*scsi_status = check_device_access_params(scp, lba, num, true);
+	if (*scsi_status)
+		return -1;
+
+	/* DIX + T10 DIF */
+	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
+		ret = __resp_write_dt0_dix(scp, lba, num, ei_lba,
+					   scsi_status);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
+	if (ret < 0)
+		*scsi_status = DID_ERROR << 16;
+
+	if (unlikely(lbp))
+		map_region(sip, lba, num);
+
+	/* If ZBC zone then bump its write pointer */
+	if (sdebug_dev_is_zoned(devip) && ret > 0)
+		zbc_inc_wp(devip, lba, ret >> ilog2(sdebug_sector_size));
+
+	return ret;
+}
+
 static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -5104,7 +5183,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	u64 lba;
 	struct sdeb_store_info *sip = devip2sip(devip, true);
 	u8 *cmd = scp->cmnd;
-	bool meta_data_locked = false;
+	int scsi_status = DID_ERROR << 16;
 	bool lbp = scsi_debug_lbp();
 
 	if (unlikely(sdebug_opts & SDEBUG_OPT_UNALIGNED_WRITE &&
@@ -5174,60 +5253,16 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	if (sdebug_dev_is_zoned(devip) ||
 	    (sdebug_dix && scsi_prot_sg_count(scp)) || lbp) {
 		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
-
-	ret = check_device_access_params(scp, lba, num, true);
-	if (ret) {
-		if (meta_data_locked)
-			sdeb_meta_write_unlock(sip);
-		return ret;
-	}
-
-	/* DIX + T10 DIF */
-	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		switch (prot_verify_write(scp, lba, num, ei_lba)) {
-		case 1: /* Guard tag error */
-			if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return illegal_condition_result;
-			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return check_condition_result;
-			}
-			break;
-		case 3: /* Reference tag error */
-			if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return illegal_condition_result;
-			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return check_condition_result;
-			}
-			break;
-		}
-	}
-
-	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
-	if (unlikely(lbp))
-		map_region(sip, lba, num);
-
-	/* If ZBC zone then bump its write pointer over the data written */
-	if (sdebug_dev_is_zoned(devip) && ret > 0)
-		zbc_inc_wp(devip, lba, ret >> ilog2(sdebug_sector_size));
-	if (meta_data_locked)
+		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
+				       lbp, &scsi_status);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
+				       lbp, &scsi_status);
+	}
 
 	if (unlikely(-1 == ret))
-		return DID_ERROR << 16;
+		return scsi_status;
 	else if (unlikely(sdebug_verbose &&
 			  (ret < (num * sdebug_sector_size))))
 		sdev_printk(KERN_INFO, scp->device,

[PATCH v4 6/8] scsi: scsi_debug: Split resp_atomic_write()

[Bart Van Assche]

Prepare for enabling lock context analysis by eliminating conditional
locking.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 685949aedf9a..244250ba460b 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6352,13 +6352,13 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 	if (scsi_bufflen(scp) < len * sdebug_sector_size)
 		return DID_ERROR << 16;
 
-	if (lbp)
-		sdeb_meta_write_lock(sip);
-
-	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
 	if (lbp) {
+		sdeb_meta_write_lock(sip);
+		ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
 		map_region(sip, lba, len);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
 	}
 
 	if (unlikely(ret == -1))

[PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche]

Prepare the scsi_debug driver for enabling compiler-based lock context
analysis:

- Annotate the lock and unlock helper functions with __acquires,
  __releases, __acquires_shared, and __releases_shared.

- Remove sparse __acquire() and __release() calls from the helper
  functions since these mislead the lock context analyzer.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 76 ++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 244250ba460b..2edb70013cbc 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4052,42 +4052,43 @@ static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
 
 static inline void
 sdeb_read_lock(rwlock_t *lock)
+	__acquires_shared(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
 		read_lock(lock);
 }
 
 static inline void
 sdeb_read_unlock(rwlock_t *lock)
+	__releases_shared(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
 		read_unlock(lock);
 }
 
 static inline void
 sdeb_write_lock(rwlock_t *lock)
+	__acquires(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
 		write_lock(lock);
 }
 
 static inline void
 sdeb_write_unlock(rwlock_t *lock)
+	__releases(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
 		write_unlock(lock);
 }
 
 static inline void
 sdeb_data_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4096,6 +4097,7 @@ sdeb_data_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4104,6 +4106,7 @@ sdeb_data_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4112,6 +4115,7 @@ sdeb_data_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4120,6 +4124,7 @@ sdeb_data_write_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4128,6 +4133,7 @@ sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4136,6 +4142,7 @@ sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4144,6 +4151,7 @@ sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4165,6 +4173,8 @@ sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
+	__acquires(&sip->macc_data_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (atomic)
 		sdeb_data_write_lock(sip);
@@ -4174,6 +4184,8 @@ sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
 
 static inline void
 sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
+	__releases(&sip->macc_data_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (atomic)
 		sdeb_data_write_unlock(sip);
@@ -4184,6 +4196,8 @@ sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
 /* Allow many reads but only 1x write per sector */
 static inline void
 sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
+	__acquires(&sip->macc_sector_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (do_write)
 		sdeb_data_sector_write_lock(sip);
@@ -4193,6 +4207,8 @@ sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
 
 static inline void
 sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
+	__releases(&sip->macc_sector_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (do_write)
 		sdeb_data_sector_write_unlock(sip);
@@ -4202,13 +4218,10 @@ sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
 
 static inline void
 sdeb_meta_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			read_lock(&sip->macc_meta_lck);
 		else
@@ -4218,13 +4231,10 @@ sdeb_meta_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			read_unlock(&sip->macc_meta_lck);
 		else
@@ -4234,13 +4244,10 @@ sdeb_meta_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			write_lock(&sip->macc_meta_lck);
 		else
@@ -4250,13 +4257,10 @@ sdeb_meta_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			write_unlock(&sip->macc_meta_lck);
 		else

[PATCH v4 8/8] scsi: core: Enable lock context analysis for the scsi_debug driver

[Bart Van Assche]

Enable lock context analysis now that all lock context analysis warnings
have been fixed.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 533623382eca..32d816e942d6 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -18,6 +18,7 @@ CONTEXT_ANALYSIS_constants.o := y
 CONTEXT_ANALYSIS_hosts.o := y
 CONTEXT_ANALYSIS_scsi.o := y
 CONTEXT_ANALYSIS_scsi_common.o := y
+CONTEXT_ANALYSIS_scsi_debug.o := y
 CONTEXT_ANALYSIS_scsi_devinfo.o := y
 CONTEXT_ANALYSIS_scsi_dh.o := y
 CONTEXT_ANALYSIS_scsi_error.o := y

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: deadlock or oops due to unprotected sdebug_no_rwlock modification
--

commit c129c8944c668498a44ba3a38c88f4214c8204f7
Author: Bart Van Assche <bvanassche@acm.org>

scsi: scsi_debug: Improve lock context annotations

Prepare the scsi_debug driver for enabling compiler-based lock context
analysis:

- Annotate the lock and unlock helper functions with __acquires,
  __releases, __acquires_shared, and __releases_shared.

- Remove sparse __acquire() and __release() calls from the helper
  functions since these mislead the lock context analyzer.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 244250ba460bc..2edb70013cbc2 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -4052,42 +4052,43 @@ static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
>  
>  static inline void
>  sdeb_read_lock(rwlock_t *lock)
> +	__acquires_shared(lock)
> +	__context_unsafe(/*conditional locking*/)
>  {
> -	if (sdebug_no_rwlock)
> -		__acquire(lock);
> -	else
> +	if (!sdebug_no_rwlock)
>  		read_lock(lock);
>  }

[Severity: High]
Does this conditional lock acquisition create a potential deadlock or oops?

The lock acquisition in sdeb_read_lock() is conditional on the unprotected
runtime value of sdebug_no_rwlock. If a user writes to the no_rwlock
attribute while I/O operations are actively being queued or processed, the
variable can change mid-flight.

drivers/scsi/scsi_debug.c:no_rwlock_store() {
	...
	if (kstrtobool(buf, &v))
		return -EINVAL;

	sdebug_no_rwlock = v;
	return count;
}

This sysfs store function modifies the global sdebug_no_rwlock variable
concurrently without any locking or quiescent state wait, allowing the race
condition to occur.

>  
>  static inline void
>  sdeb_read_unlock(rwlock_t *lock)
> +	__releases_shared(lock)
> +	__context_unsafe(/*conditional locking*/)
>  {
> -	if (sdebug_no_rwlock)
> -		__release(lock);
> -	else
> +	if (!sdebug_no_rwlock)
>  		read_unlock(lock);
>  }

[Severity: High]
Does this lock release rely on the same unprotected global variable?

If sdebug_no_rwlock changed to true since acquisition, the unlock is
skipped, causing a permanent deadlock. If changed to false while inside
a lockless critical section, read_unlock() is called without a prior
read_lock(), causing a kernel oops.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791349129.git.bvanassche@acm.org?part=7

Re: [PATCH v4 5/8] scsi: scsi_debug: Split resp_write_dt0()

[Christoph Hellwig]

On Tue, Oct 06, 2026 at 10:07:27PM -0700, Bart Van Assche wrote:
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index c66f8556896d..685949aedf9a 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5094,6 +5094,85 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
>  	return error;
>  }
>  
> +/*
> + * Returns 0 upon success or -1 with *scsi_status set to a SCSI status code upon

Overly long line.

The rest looks pretty sane.

Re: [PATCH v4 6/8] scsi: scsi_debug: Split resp_atomic_write()

[Christoph Hellwig]

On Tue, Oct 06, 2026 at 10:07:28PM -0700, Bart Van Assche wrote:
> Prepare for enabling lock context analysis by eliminating conditional
> locking.

Looks good:

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

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Christoph Hellwig]

On Tue, Oct 06, 2026 at 10:07:29PM -0700, Bart Van Assche wrote:
>  static inline void
>  sdeb_read_lock(rwlock_t *lock)
> +	__acquires_shared(lock)
> +	__context_unsafe(/*conditional locking*/)
>  {
> -	if (sdebug_no_rwlock)
> -		__acquire(lock);
> -	else
> +	if (!sdebug_no_rwlock)
>  		read_lock(lock);
>  }

Is there no way we can make the spare-style __acquire/__release work for
clang?  That would be this and similar patterns so much better and
safer than the blanket __context_unsafe.

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Marco Elver]

On Wed, 7 Oct 2026 at 15:14, Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Oct 06, 2026 at 10:07:29PM -0700, Bart Van Assche wrote:
> >  static inline void
> >  sdeb_read_lock(rwlock_t *lock)
> > +     __acquires_shared(lock)
> > +     __context_unsafe(/*conditional locking*/)
> >  {
> > -     if (sdebug_no_rwlock)
> > -             __acquire(lock);
> > -     else
> > +     if (!sdebug_no_rwlock)
> >               read_lock(lock);
> >  }
>
> Is there no way we can make the spare-style __acquire/__release work for
> clang?  That would be this and similar patterns so much better and
> safer than the blanket __context_unsafe.

__acquire/__release/etc. fake acquire/release functions do already
work with Clang Context Analysis. I spent quite a while making them
work because of patterns like above, and lots of macros which rely on
them. See definition of context_lock_struct() in
include/linux/compiler-context-analysis.h where they are implemented
(tldr; they are "overloaded" empty inline functions which exist per
context lock type).

Thanks,
-- Marco

Re: [PATCH v4 5/8] scsi: scsi_debug: Split resp_write_dt0()

[Bart Van Assche]

On 10/7/26 6:12 AM, Christoph Hellwig wrote:
> On Tue, Oct 06, 2026 at 10:07:27PM -0700, Bart Van Assche wrote:
>> + * Returns 0 upon success or -1 with *scsi_status set to a SCSI status code upon
> 
> Overly long line.

Hi Christoph,

This line should be exactly 80 columns wide. Is that too wide? I'm fine 
with reformatting this comment but I'm surprised to read that 80 columns
is too wide?

Thanks,

Bart.

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche]

On 10/7/26 6:14 AM, Christoph Hellwig wrote:
> On Tue, Oct 06, 2026 at 10:07:29PM -0700, Bart Van Assche wrote:
>>   static inline void
>>   sdeb_read_lock(rwlock_t *lock)
>> +	__acquires_shared(lock)
>> +	__context_unsafe(/*conditional locking*/)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		__acquire(lock);
>> -	else
>> +	if (!sdebug_no_rwlock)
>>   		read_lock(lock);
>>   }
> 
> Is there no way we can make the spare-style __acquire/__release work for
> clang?  That would be this and similar patterns so much better and
> safer than the blanket __context_unsafe.

Hi Christoph,

Do you like the alternative below better? If so then I will replace
patch 7/8 with the changes below.

Thanks,

Bart.


diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index b339f022af26..e8d8b693751e 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4052,42 +4052,47 @@ static inline struct sdeb_store_info 
*devip2sip(struct sdebug_dev_info *devip,

  static inline void
  sdeb_read_lock(rwlock_t *lock)
+	__acquires_shared(lock)
  {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
  		read_lock(lock);
+	else
+		__acquire_shared(lock);
  }

  static inline void
  sdeb_read_unlock(rwlock_t *lock)
+	__releases_shared(lock)
  {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
  		read_unlock(lock);
+	else
+		__release_shared(lock);
  }

  static inline void
  sdeb_write_lock(rwlock_t *lock)
+	__acquires(lock)
  {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
  		write_lock(lock);
+	else
+		__acquire(lock);
  }

  static inline void
  sdeb_write_unlock(rwlock_t *lock)
+	__releases(lock)
  {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
  		write_unlock(lock);
+	else
+		__release(lock);
  }

  static inline void
  sdeb_data_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4096,6 +4101,7 @@ sdeb_data_read_lock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4104,6 +4110,7 @@ sdeb_data_read_unlock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4112,6 +4119,7 @@ sdeb_data_write_lock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4120,6 +4128,7 @@ sdeb_data_write_unlock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4128,6 +4137,7 @@ sdeb_data_sector_read_lock(struct sdeb_store_info 
*sip)

  static inline void
  sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4136,6 +4146,7 @@ sdeb_data_sector_read_unlock(struct 
sdeb_store_info *sip)

  static inline void
  sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4144,6 +4155,7 @@ sdeb_data_sector_write_lock(struct sdeb_store_info 
*sip)

  static inline void
  sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4165,102 +4177,122 @@ sdeb_data_sector_write_unlock(struct 
sdeb_store_info *sip)

  static inline void
  sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
+	__acquires(&sip->macc_data_lck)
  {
-	if (atomic)
+	if (atomic) {
  		sdeb_data_write_lock(sip);
-	else
+	} else {
  		sdeb_data_read_lock(sip);
+		__release_shared(&sip->macc_data_lck);
+		__acquire(&sip->macc_data_lck);
+	}
  }

  static inline void
  sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
+	__releases(&sip->macc_data_lck)
  {
-	if (atomic)
+	if (atomic) {
  		sdeb_data_write_unlock(sip);
-	else
+	} else {
+		__release(&sip->macc_data_lck);
+		__acquire_shared(&sip->macc_data_lck);
  		sdeb_data_read_unlock(sip);
+	}
  }

  /* Allow many reads but only 1x write per sector */
  static inline void
  sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
+	__acquires(&sip->macc_sector_lck)
  {
-	if (do_write)
+	if (do_write) {
  		sdeb_data_sector_write_lock(sip);
-	else
+	} else {
  		sdeb_data_sector_read_lock(sip);
+		__release_shared(&sip->macc_sector_lck);
+		__acquire(&sip->macc_sector_lck);
+	}
  }

  static inline void
  sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
+	__releases(&sip->macc_sector_lck)
  {
-	if (do_write)
+	if (do_write) {
  		sdeb_data_sector_write_unlock(sip);
-	else
+	} else {
+		__release(&sip->macc_sector_lck);
+		__acquire_shared(&sip->macc_sector_lck);
  		sdeb_data_sector_read_unlock(sip);
+	}
  }

  static inline void
  sdeb_meta_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			read_lock(&sip->macc_meta_lck);
-		else
+		} else {
  			read_lock(&sdeb_fake_rw_lck);
+			__release_shared(&sdeb_fake_rw_lck);
+			__acquire_shared(&sip->macc_meta_lck);
+		}
+	} else {
+		__acquire_shared(&sip->macc_meta_lck);
  	}
  }

  static inline void
  sdeb_meta_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			read_unlock(&sip->macc_meta_lck);
-		else
+		} else {
+			__acquire_shared(&sdeb_fake_rw_lck);
  			read_unlock(&sdeb_fake_rw_lck);
+			__release_shared(&sip->macc_meta_lck);
+		}
+	} else {
+		__release_shared(&sip->macc_meta_lck);
  	}
  }

  static inline void
  sdeb_meta_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			write_lock(&sip->macc_meta_lck);
-		else
+		} else {
  			write_lock(&sdeb_fake_rw_lck);
+			__release(&sdeb_fake_rw_lck);
+			__acquire(&sip->macc_meta_lck);
+		}
+	} else {
+		__acquire(&sip->macc_meta_lck);
  	}
  }

  static inline void
  sdeb_meta_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			write_unlock(&sip->macc_meta_lck);
-		else
+		} else {
+			__acquire(&sdeb_fake_rw_lck);
  			write_unlock(&sdeb_fake_rw_lck);
+			__release(&sip->macc_meta_lck);
+		}
+	} else {
+		__release(&sip->macc_meta_lck);
  	}
  }

Re: [PATCH v4 5/8] scsi: scsi_debug: Split resp_write_dt0()

[Christoph Hellwig]

On Wed, Oct 07, 2026 at 01:08:52PM -0700, Bart Van Assche wrote:
> On 10/7/26 6:12 AM, Christoph Hellwig wrote:
>> On Tue, Oct 06, 2026 at 10:07:27PM -0700, Bart Van Assche wrote:
>>> + * Returns 0 upon success or -1 with *scsi_status set to a SCSI status code upon
>>
>> Overly long line.
>
> Hi Christoph,
>
> This line should be exactly 80 columns wide. Is that too wide? I'm fine 
> with reformatting this comment but I'm surprised to read that 80 columns
> is too wide?

It's fine, sorry.

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Christoph Hellwig]

On Wed, Oct 07, 2026 at 01:10:27PM -0700, Bart Van Assche wrote:
> Do you like the alternative below better? If so then I will replace
> patch 7/8 with the changes below.

Much better, although I'd drop the pointless inversion of the
condition and keep the flow as in the old version:

static inline void
sdeb_read_lock(rwlock_t *lock)
	__acquires_shared(lock)
{
	if (sdebug_no_rwlock)
		__acquire_shared(lock);
	else
 		read_lock(lock);
}