[PATCH v4 00/10] scsi: scsi_debug: fix zoned write validation

[PATCH v4 00/10] scsi: scsi_debug: fix zoned write validation

[Niklas Cassel]

This series fixes scsi_debug with regards to writes to a ZBC drive, and
to WRITE ATOMIC (16). Each patch describes the problem that it fixes.

Two configurations are refused rather than emulated, and those two
patches come first, as the rest of the series relies on what they
exclude: a zoned device whose lowest aligned LBA is not zero, which no
host writing in units of the reported zone write granularity could ever
write, and atomic writes on a zoned device, which no drive supports.

Little of this is reachable in a default configuration. Most of it needs
physblk_exp, atomic_wr or logical block provisioning to be set, or an
initiator that does not provide a data buffer matching the transfer
length of the command.

The series is based on 7.4/scsi-staging rather than 7.3/scsi-fixes (as
Damien suggested on v1) to avoid a build failure that would have
happened if this series was simply merged with linux-next:

  error: too many arguments to function 'mk_sense_buffer'

Tested on a zoned scsi_debug device with 512 byte logical blocks and a
4096 byte physical block, and on a device that is not zoned for the
WRITE ATOMIC (16) patches. The notes below each patch describe what was
observed before and after it.

Changes since v3:
- Patch 1 is new, so that the physical block boundary which patch 6
  checks is the one that the device reports.
- Patch 2 is new and replaces v3 patch 6, which taught WRITE ATOMIC (16)
  about zones instead of refusing the combination.
- Patches 3 and 4 are new. They are the two problems that the review of
  v3 patch 5 found, both of which are older than this series.
- Patch 5 guards against sg_off exceeding the length of the buffer in
  resp_write_scat(), which would have reported a residual of nearly 4G.
- Patch 7 applies only to sequential write required zones now, as
  physical block unaligned writes are fine everywhere else.
- Patch 10 is new. It keeps the part of v3 patch 6 that has nothing to
  do with zones, without which WRITE ATOMIC (16) would go back to
  writing past the end of the device and ignoring wp.
- Picked up Damien's Reviewed-by on patches 5, 8 and 9.

Niklas Cassel (10):
  scsi: scsi_debug: Refuse a zoned device with a non-zero lowest aligned
    LBA
  scsi: scsi_debug: Make atomic writes and ZBC emulation mutually
    exclusive
  scsi: scsi_debug: Take the zone metadata lock before the data lock
  scsi: scsi_debug: Evaluate scsi_debug_lbp() only once
  scsi: scsi_debug: Report the residual of a write
  scsi: scsi_debug: Enforce physical block alignment of zoned writes
  scsi: scsi_debug: Do not write a partial physical block to a zoned
    device
  scsi: scsi_debug: Advance the write pointer over the data written
  scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)
  scsi: scsi_debug: Validate the access parameters of WRITE ATOMIC (16)

 drivers/scsi/scsi_debug.c | 103 ++++++++++++++++++++++++++++++++------
 1 file changed, 87 insertions(+), 16 deletions(-)


base-commit: c3cff7fac01638ab58e85fe7df41a04fa25c5bae
-- 
2.55.0

[PATCH v4 01/10] scsi: scsi_debug: Refuse a zoned device with a non-zero lowest aligned LBA

[Niklas Cassel]

The LOWEST ALIGNED LOGICAL BLOCK ADDRESS field indicates the LBA of the
first logical block that is located at the beginning of a physical
block, see SBC-6 r02 (T10/BSR INCITS 587), 5.20.2. scsi_debug reports
the lowest_aligned module parameter in that field of the READ CAPACITY
(16) parameter data, but nothing else in the driver acts on it: the data
it stores begins at LBA 0 and has no physical block structure behind it.

That does not matter for a device that is not zoned, where a write need
not end on a physical block boundary at all. It does matter for a zoned
device: ZBC requires a write to a sequential write required zone to end
on a physical block boundary, and a zone begins at a multiple of the
zone size, so a non-zero lowest aligned LBA puts the beginning of every
zone in the middle of a physical block. A host that writes in units of
the zone write granularity that it was given can then never satisfy the
requirement, and such a zone cannot be written at all.

Refuse the combination rather than emulate a device that cannot be used.

Assisted-by: LLM
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested by loading the module with each combination:

  zbc=managed lowest_aligned=8    refused, -EINVAL
  zbc=aware   lowest_aligned=8    refused, -EINVAL
  zbc=managed lowest_aligned=0    loads
  lowest_aligned=8                loads, as the device is not zoned
---
 drivers/scsi/scsi_debug.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6941809dfdb7..c664966e95cb 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -8734,6 +8734,11 @@ static int __init scsi_debug_init(void)
 			sdebug_dev_size_mb = DEF_ZBC_DEV_SIZE_MB;
 	}
 
+	if (sdeb_zbc_in_use && sdebug_lowest_aligned) {
+		pr_err("lowest_aligned is not supported by a zoned device\n");
+		return -EINVAL;
+	}
+
 	if (sdebug_dev_size_mb == DEF_DEV_SIZE_PRE_INIT)
 		sdebug_dev_size_mb = DEF_DEV_SIZE_MB;
 	if (sdebug_dev_size_mb < 1)

base-commit: c3cff7fac01638ab58e85fe7df41a04fa25c5bae
-- 
2.55.0

[PATCH v4 02/10] scsi: scsi_debug: Make atomic writes and ZBC emulation mutually exclusive

[Niklas Cassel]

There are no SMR drives that support atomic writes. Emulating a device
that supports both is emulating something that a user cannot encounter,
rather than the drives that exist, and it means maintaining the zone
handling of a command that no zoned device implements.

Nothing in the standards forbids the combination. For what would be by
far the most common case, a host managed SMR drive whose logical block
size equals its physical block size, there is no problem in the two
features being combined: the drive vendor only has to give atomic
writes an alignment that makes sense with regards to ZBC, for instance
equal to the physical block size. This is therefore a choice not to
complicate scsi_debug, rather than a restriction that the standards
impose.

Refuse the combination at initialisation. If SMR drives that support
atomic writes ever become a thing, this can be changed back.

Suggested-by: Damien Le Moal <dlemoal@kernel.org>
Assisted-by: LLM
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested by loading the module with each combination:

  zbc=managed atomic_wr=1    refused, -EINVAL
  zbc=aware   atomic_wr=1    refused, -EINVAL
  zbc=managed                loads
  atomic_wr=1                loads
---
 drivers/scsi/scsi_debug.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index c664966e95cb..8e45a57ae406 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -8739,6 +8739,11 @@ static int __init scsi_debug_init(void)
 		return -EINVAL;
 	}
 
+	if (sdeb_zbc_in_use && sdebug_atomic_wr) {
+		pr_err("atomic_wr is not supported by a zoned device\n");
+		return -EINVAL;
+	}
+
 	if (sdebug_dev_size_mb == DEF_DEV_SIZE_PRE_INIT)
 		sdebug_dev_size_mb = DEF_DEV_SIZE_MB;
 	if (sdebug_dev_size_mb < 1)
-- 
2.55.0

[PATCH v4 03/10] scsi: scsi_debug: Take the zone metadata lock before the data lock

[Niklas Cassel]

resp_comp_write() takes the data lock and then the zone metadata lock.
Every other command that takes both takes them the other way round:
resp_write_dt0(), resp_write_scat(), resp_write_same() and
resp_write_tape() take the metadata lock and then call
do_device_access(), which takes the data lock.

Two commands that take the same two locks in opposite orders can
deadlock against each other, so a COMPARE AND WRITE and any other write
to the same store can hang one another.

Take the locks in the same order as everywhere else, and release them in
the reverse of that order.

Correct the comment above map_region() while here. The provisioning map
is covered by the metadata lock rather than by the data lock, which is
why resp_unmap() takes only the metadata lock while unmap_region()
clears map bits.

Assisted-by: LLM
Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug sector_size=512 dev_size_mb=128 lbpu=1 lbpws=1

COMPARE AND WRITE completes, and a READ(16) of the block that it wrote
returns, so the reordered locks are still taken and released correctly.

The deadlock itself was not reproduced. This kernel is built without
lockdep, and the window needs a COMPARE AND WRITE and another write to
the same store to interleave between the two acquisitions. Found by
review.
---
 drivers/scsi/scsi_debug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 8e45a57ae406..18aefe83b7b6 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5575,8 +5575,8 @@ static int resp_comp_write(struct scsi_cmnd *scp,
 			    "indicated=%u, IO sent=%d bytes\n", my_name,
 			    dnum * lb_size, ret);
 
-	sdeb_data_write_lock(sip);
 	sdeb_meta_write_lock(sip);
+	sdeb_data_write_lock(sip);
 	if (!comp_write_worker(sip, lba, num, arr, false)) {
 		mk_sense_buffer(scp, MISCOMPARE,
 				MISCOMPARE_DURING_VERIFY_OPERATION);
@@ -5584,12 +5584,12 @@ static int resp_comp_write(struct scsi_cmnd *scp,
 		goto cleanup_unlock;
 	}
 
-	/* Cover sip->map_storep (which map_region()) sets with data lock */
+	/* Cover sip->map_storep (which map_region() sets) with the meta lock */
 	if (scsi_debug_lbp())
 		map_region(sip, lba, num);
 cleanup_unlock:
-	sdeb_meta_write_unlock(sip);
 	sdeb_data_write_unlock(sip);
+	sdeb_meta_write_unlock(sip);
 cleanup_free:
 	kfree(arr);
 	return retval;
-- 
2.55.0

[PATCH v4 04/10] scsi: scsi_debug: Evaluate scsi_debug_lbp() only once

[Niklas Cassel]

corrupt_lbas(), resp_write_dt0() and resp_write_same() call
scsi_debug_lbp() to decide whether to take the zone metadata lock, and
then call it again to decide whether to read or write the provisioning
map that the lock protects.

The result is not a constant. scsi_debug_lbp() is false while the
fake_rw module parameter is set, and fake_rw can be written at any time,
both as a module parameter and through its driver attribute in sysfs.
The two calls can therefore disagree, and the later one can decide to
touch the provisioning map after the earlier one decided not to take the
lock that protects it.

Call it once and use the result throughout.

Assisted-by: LLM
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug sector_size=512 dev_size_mb=128 lbpu=1 lbpws=1

A WRITE(16) completes and GET LBA STATUS then reports the region as
mapped, which covers resp_write_dt0(). A WRITE SAME and a WRITE SAME
with the UNMAP bit set complete, and GET LBA STATUS reports the first
region as mapped and the second as deallocated, which covers all three
uses of the result in resp_write_same().

corrupt_lbas() is not covered. It is reached by writing to the corrupt
file in debugfs, and that interface rejected the requests that were
tried, for a reason that has nothing to do with this patch.

The window that the change closes was not reproduced. It needs fake_rw
to be written between two calls, and was found by review.
---
 drivers/scsi/scsi_debug.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 18aefe83b7b6..c68dba6dbbdd 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4953,12 +4953,13 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 {
 	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 ||
-	    scsi_debug_lbp())  {
+	    lbp)  {
 		sdeb_meta_write_lock(sip);
 		meta_data_locked = true;
 	}
@@ -4975,7 +4976,7 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 		goto out_unlock;
 	}
 
-	if (scsi_debug_lbp() &&
+	if (lbp &&
 	    (!map_state(sip, lba, &num_mapped) || num > num_mapped)) {
 		pr_err("can't modify unmapped logical blocks: %llu:%u",
 			lba, num);
@@ -5035,6 +5036,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	struct sdeb_store_info *sip = devip2sip(devip, true);
 	u8 *cmd = scp->cmnd;
 	bool meta_data_locked = false;
+	bool lbp = scsi_debug_lbp();
 
 	if (unlikely(sdebug_opts & SDEBUG_OPT_UNALIGNED_WRITE &&
 		     atomic_read(&sdeb_inject_pending))) {
@@ -5102,7 +5104,7 @@ 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)) ||
-	    scsi_debug_lbp())  {
+	    lbp)  {
 		sdeb_meta_write_lock(sip);
 		meta_data_locked = true;
 	}
@@ -5147,7 +5149,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	}
 
 	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
-	if (unlikely(scsi_debug_lbp()))
+	if (unlikely(lbp))
 		map_region(sip, lba, num);
 
 	/* If ZBC zone then bump its write pointer */
@@ -5374,8 +5376,9 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 	u8 *fs1p;
 	u8 *fsp;
 	bool meta_data_locked = false;
+	bool lbp = scsi_debug_lbp();
 
-	if (sdebug_dev_is_zoned(devip) || scsi_debug_lbp()) {
+	if (sdebug_dev_is_zoned(devip) || lbp) {
 		sdeb_meta_write_lock(sip);
 		meta_data_locked = true;
 	}
@@ -5384,7 +5387,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 	if (ret)
 		goto out;
 
-	if (unmap && scsi_debug_lbp()) {
+	if (unmap && lbp) {
 		unmap_region(sip, lba, num);
 		goto out;
 	}
@@ -5414,7 +5417,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 		block = do_div(lbaa, sdebug_store_sectors);
 		memmove(fsp + (block * lb_size), fs1p, lb_size);
 	}
-	if (scsi_debug_lbp())
+	if (lbp)
 		map_region(sip, lba, num);
 	/* If ZBC zone then bump its write pointer */
 	if (sdebug_dev_is_zoned(devip))
-- 
2.55.0

[PATCH v4 05/10] scsi: scsi_debug: Report the residual of a write

[Niklas Cassel]

A command transfers the number of logical blocks that it asks for,
bounded by the data buffer that the initiator provided. When the buffer
is larger than that, the bytes beyond are not transferred, and the
difference is a residual that the initiator is entitled to be told
about.

resp_read_dt0() reports it, as does resp_write_tape(), but the write
paths for a disk do not: scsi_get_resid() keeps the zero that
scsi_debug_queuecommand() initialised it with, so a write with an
oversized buffer completes with GOOD status and a residual of zero, as
though the whole buffer had been consumed. A READ of the same length
into the same buffer reports the residual correctly, so the two
directions disagree.

Report it in resp_write_dt0(), resp_write_scat() and
resp_atomic_write().

resp_write_scat() needs a different expression from the other two. Its
data-out buffer holds the parameter list header and the LBA range
descriptors as well as the data, and sg_off walks over all of it, from
the offset at which the data begins to the end of the last range that
was written, so what the command consumed is sg_off and not the number
of bytes that the last range transferred.

It also needs a guard that the other two do not. sg_off counts what the
command asked for rather than what was transferred: lbdof is not
validated against the length of the buffer, and a range is counted in
full even when do_device_access() copied less of it, so sg_off can
exceed the buffer. The other two subtract the number of bytes that were
copied, which the buffer bounds.

Assisted-by: LLM
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
      zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2

An eight logical block WRITE(16) with a 5000 byte buffer reports
resid=904, having transferred 4096. A READ(16) of the same length into
the same buffer reported that before this patch and the WRITE reported
0. A buffer of exactly 4096 bytes reports 0, as does a 512 byte buffer,
which is consumed in its entirety. WRITE ATOMIC (16) behaves like the
WRITE, on a device that is not zoned.

WRITE SCATTERED (16) with one LBA range descriptor of eight blocks and a
5632 byte buffer reports resid=1024, having consumed 512 bytes of
parameter list and 4096 bytes of data. The same command with a 1024 byte
buffer completes and reports resid=0: sg_off reaches 4608, past the end
of the buffer, and without the guard the residual would have been
reported as 4294963712.
---
 drivers/scsi/scsi_debug.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index c68dba6dbbdd..170d5c944b61 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5166,6 +5166,8 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 			    "%s: write: cdb indicated=%u, IO sent=%d bytes\n",
 			    my_name, num * sdebug_sector_size, ret);
 
+	scsi_set_resid(scp, scsi_bufflen(scp) - ret);
+
 	if (unlikely((sdebug_opts & SDEBUG_OPT_RECOV_DIF_DIX) &&
 		     atomic_read(&sdeb_inject_pending))) {
 		if (sdebug_opts & SDEBUG_OPT_RECOVERED_ERR) {
@@ -5354,6 +5356,13 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 		sg_off += num_by;
 		cum_lb += num;
 	}
+	/*
+	 * sg_off counts what the command asked for, which can exceed the
+	 * buffer: lbdof is not validated against it, and a range is counted
+	 * in full even if do_device_access() copied less.
+	 */
+	if (scsi_bufflen(scp) > sg_off)
+		scsi_set_resid(scp, scsi_bufflen(scp) - sg_off);
 	ret = 0;
 err_out_unlock:
 	sdeb_meta_write_unlock(sip);
@@ -6210,6 +6219,8 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 		return DID_ERROR << 16;
 	if (unlikely(ret != len * sdebug_sector_size))
 		return DID_ERROR << 16;
+
+	scsi_set_resid(scp, scsi_bufflen(scp) - ret);
 	return 0;
 }
 
-- 
2.55.0

[PATCH v4 06/10] scsi: scsi_debug: Enforce physical block alignment of zoned writes

[Niklas Cassel]

ZBC-3 r06 (T10/BSR INCITS 579), 4.5.3.3.2 Write access pattern
requirements for sequential write required zones, states:

  The device server terminates with CHECK CONDITION status, with the
  sense key set to ILLEGAL REQUEST, and the additional sense code set
  to UNALIGNED WRITE COMMAND a write command, other than an entire
  medium write same command, that specifies:
    a) the starting LBA in a sequential write required zone set to a
       value that is not equal to the write pointer for that sequential
       write required zone; or
    b) an ending LBA that is not equal to the last logical block within
       a physical block (see SBC-5).

That is why sd_zbc_read_zones() sets the zone_write_granularity queue
limit to the physical block size of a host-managed device, exposing the
constraint to user space.

check_zbc_access_params() implements condition a) but not condition b):
it verifies that a write to a sequential write required zone starts at
the write pointer of the zone, but never validates the ending LBA. As a
consequence, when scsi_debug emulates a host-managed device whose
physical block size is larger than its logical block size, for instance
with zbc=managed sector_size=512 physblk_exp=3, a write of a single
logical block at the write pointer of a sequential zone is accepted and
advances the write pointer by one logical block. The write pointer is
then no longer a multiple of the zone_write_granularity reported for the
device, so nothing can write at it at the granularity that was
advertised, and the zone can only be used again after being reset.

Implement condition b) as well, with the same sense data as the write
pointer check, as the standard gives both conditions the same sense key
and additional sense code. The exclusion of an entire medium write same
command needs no special case: such a command spans the whole medium, so
it is already terminated with WRITE BOUNDARY VIOLATION by the preceding
check.

The check is placed in check_zbc_access_params(), which every command
that advances a zone write pointer reaches first: WRITE, WRITE SCATTERED
and WRITE SAME. Reads return earlier in the function and are unaffected.

Sequential write preferred zones, which are emulated for host-aware
devices with zbc=aware, are left alone: writes to them are not required
to be sequential, and Linux does not restrict the write granularity of
host-aware devices.

With the default physblk_exp=0, the physical block size equals the
logical block size and the new check is a no-op.

Assisted-by: LLM
Fixes: f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
      zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2

Before this patch, a one logical block WRITE(16) at the write pointer of
an empty sequential write required zone completes with GOOD status and
leaves the write pointer at LBA 0x18001, which is not a multiple of the
4096 byte zone_write_granularity reported for the device. After it, the
same command is terminated with ILLEGAL REQUEST / UNALIGNED WRITE
COMMAND and the zone is left EMPTY, while an aligned eight block write
is still accepted.

Compared to the version reviewed in v1, the only change is that the new
mk_sense_buffer() call uses the combined UNALIGNED_WRITE_COMMAND sense
code, as this is based on 7.4/scsi-staging.
---
 drivers/scsi/scsi_debug.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 170d5c944b61..a4db282ac971 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3980,6 +3980,16 @@ static int check_zbc_access_params(struct scsi_cmnd *scp,
 					UNALIGNED_WRITE_COMMAND);
 			return check_condition_result;
 		}
+		/*
+		 * Writes must end on a physical block boundary, that is, the
+		 * transfer length must be a multiple of the physical block
+		 * size.
+		 */
+		if (!IS_ALIGNED(lba + num, 1U << sdebug_physblk_exp)) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					UNALIGNED_WRITE_COMMAND);
+			return check_condition_result;
+		}
 	}
 
 	/* Handle implicit open of closed and empty zones */
-- 
2.55.0

[PATCH v4 07/10] scsi: scsi_debug: Do not write a partial physical block to a zoned device

[Niklas Cassel]

do_device_access() copies one logical block at a time and stops at the
first short copy, so when the data-out buffer is smaller than the
transfer length of the command it can write part of a physical block.
An initiator can arrange that with SG_IO.

A write that does not end on a physical block boundary is perfectly
acceptable to a device that is not zoned, and to a conventional zone,
where the device reads, modifies and writes the physical block that the
write falls in. ZBC-3 r06 (T10/BSR INCITS 579), 4.5.3.3.2, does require
a write to a sequential write required zone to end on a physical block
boundary, though, so a partly written physical block is not a state that
such a zone can be left in.

Stop at the last whole physical block that the buffer holds, for a
sequential write required zone only. The bytes that are left over are
not written, and are reported to the initiator as part of the residual.

With the default physblk_exp=0 the physical block size equals the
logical block size and this changes nothing. Nothing changes either when
the buffer holds all of the data that the command asks for, so a command
that transfers fewer logical blocks than a physical block is unaffected
wherever it is legal.

Assisted-by: LLM
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
      zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2

issuing a sixteen logical block WRITE(16) through SG_IO with a buffer of
twelve blocks. To a sequential write required zone it now transfers
eight blocks, one whole physical block, and reports the remaining 2048
bytes as the residual; before this patch it transferred twelve. To a
conventional zone it still transfers all twelve and reports no residual.

With physblk_exp=0 an eight block write with a buffer of one block still
transfers one block, wherever it is addressed.
---
 drivers/scsi/scsi_debug.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index a4db282ac971..837077a4e740 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4273,6 +4273,8 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp,
 	u64 block;
 	enum dma_data_direction dir;
 	struct scsi_data_buffer *sdb = &scp->sdb;
+	struct scsi_device *sdp = scp->device;
+	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
 	u8 *fsp;
 	int i, total = 0;
 
@@ -4300,6 +4302,24 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp,
 
 	fsp = sip->storep;
 
+	/*
+	 * A write to a sequential write required zone has to end on a physical
+	 * block boundary, so if the data-out buffer does not hold all of the
+	 * data that the command asks for, write up to the last whole physical
+	 * block that it does hold. What is left over is reported as part of
+	 * the residual.
+	 */
+	if (do_write && sdebug_dev_is_zoned(devip)) {
+		struct sdeb_zone_state *zsp = zbc_zone(devip, lba);
+
+		if (zsp->z_type == ZBC_ZTYPE_SWR) {
+			u32 avail = (sdb->length - sg_skip) / sdebug_sector_size;
+
+			if (avail < num)
+				num = round_down(avail, 1U << sdebug_physblk_exp);
+		}
+	}
+
 	block = do_div(lba, sdebug_store_sectors);
 
 	/* Only allow 1x atomic write or multiple non-atomic writes at any given time */
-- 
2.55.0

[PATCH v4 08/10] scsi: scsi_debug: Advance the write pointer over the data written

[Niklas Cassel]

resp_write_dt0() and resp_write_scat() advance the write pointer of a
sequential write required zone by the transfer length of the command
before looking at what do_device_access() returned. It does not always
move that much data: it returns -1 when the data direction of the
command does not match the operation, and a short byte count when the
data-out buffer is smaller than the transfer length, both of which an
initiator can produce with SG_IO. The first terminates the command, the
second completes it with GOOD status and a residual.

The zone state then describes more data than is on the medium, and a
write at the position where the data really ends is terminated with
UNALIGNED WRITE COMMAND, because the write pointer has moved beyond it.
The zone has to be reset before it can be written to again.

Advance the write pointer over the data that was written instead. As
do_device_access() does not write a partial physical block to a
sequential write required zone, what it reports for such a zone is a
whole number of physical blocks, so the write pointer is left on a
physical block boundary, which is where a write can end.

resp_write_same() does not need the same treatment, as it writes with
memmove() and cannot fail part way through.

Assisted-by: LLM
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Fixes: f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
      zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2

issuing WRITE(16) through SG_IO with a data-out buffer shorter than the
transfer length of the command. A sixteen block write with a buffer of
twelve blocks leaves the write pointer of an empty sequential write
required zone eight blocks on, which is where the data it wrote ends,
and a following eight block write continues from there. An eight block
write with a buffer of one block writes nothing and leaves the write
pointer where it was.

Before this patch both advanced the write pointer by the full transfer
length, and a write at the end of the data that had actually been
written was then terminated with UNALIGNED WRITE COMMAND.
---
 drivers/scsi/scsi_debug.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 837077a4e740..cdea21b570c4 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5182,9 +5182,9 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	if (unlikely(lbp))
 		map_region(sip, lba, num);
 
-	/* If ZBC zone then bump its write pointer */
-	if (sdebug_dev_is_zoned(devip))
-		zbc_inc_wp(devip, 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 / sdebug_sector_size);
 	if (meta_data_locked)
 		sdeb_meta_write_unlock(sip);
 
@@ -5348,9 +5348,9 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 		 * writes behaviour as possible.
 		 */
 		ret = do_device_access(sip, scp, sg_off, lba, num, group, true, true);
-		/* If ZBC zone then bump its write pointer */
-		if (sdebug_dev_is_zoned(devip))
-			zbc_inc_wp(devip, 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 / sdebug_sector_size);
 		if (unlikely(scsi_debug_lbp()))
 			map_region(sip, lba, num);
 		if (unlikely(-1 == ret)) {
-- 
2.55.0

[PATCH v4 09/10] scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

[Niklas Cassel]

When logical block provisioning is enabled, a command that writes user
data marks the region that it wrote in the provisioning map, so that
GET LBA STATUS reports the region as mapped. resp_write_dt0(),
resp_write_scat() and resp_write_same() all call map_region() for that.

resp_atomic_write() does not, so a WRITE ATOMIC (16) leaves the
provisioning map untouched, and GET LBA STATUS keeps reporting the
region as deallocated after it has been written. map_state(), which
GET LBA STATUS uses, is the only reader of the map, so that is the whole
of the effect.

Call map_region() the way resp_write_dt0() does, and take the zone
metadata write lock across the access as it does when logical block
provisioning is enabled. That lock is what serialises the provisioning
map against resp_unmap(), which holds it while unmap_region() clears map
bits and zeroes the data that they cover. resp_unmap() does nothing
unless logical block provisioning is enabled, so the lock is only needed
in that case.

Assisted-by: LLM
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug sector_size=512 physblk_exp=3 dev_size_mb=128 \
      atomic_wr=1 lbpu=1

GET LBA STATUS reports a region that has never been written as
deallocated, and reports it as mapped after a WRITE ATOMIC (16) of eight
blocks. Only an ordinary WRITE did so before this patch.
---
 drivers/scsi/scsi_debug.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index cdea21b570c4..5243401701f9 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6200,6 +6200,8 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 	u8 *cmd = scp->cmnd;
 	u16 boundary, len;
 	u64 lba, lba_tmp;
+	bool meta_data_locked = false;
+	bool lbp = scsi_debug_lbp();
 	int ret;
 
 	if (!scsi_debug_atomic_write()) {
@@ -6244,7 +6246,18 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 		}
 	}
 
+	if (lbp) {
+		sdeb_meta_write_lock(sip);
+		meta_data_locked = true;
+	}
+
 	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
+	if (unlikely(lbp))
+		map_region(sip, lba, len);
+
+	if (meta_data_locked)
+		sdeb_meta_write_unlock(sip);
+
 	if (unlikely(ret == -1))
 		return DID_ERROR << 16;
 	if (unlikely(ret != len * sdebug_sector_size))
-- 
2.55.0

[PATCH v4 10/10] scsi: scsi_debug: Validate the access parameters of WRITE ATOMIC (16)

[Niklas Cassel]

resp_atomic_write() validates the fields that are specific to an atomic
write, the alignment and granularity of the transfer, the atomic
boundary and the maximum transfer length, but it never validates the
range that the command addresses. Every other command that writes user
data calls check_device_access_params() first, which rejects a transfer
that ends beyond the capacity of the device, one whose length exceeds
the size of the store, and any write to a write protected device.

As a consequence a WRITE ATOMIC (16) past the end of the device is not
terminated with LOGICAL BLOCK ADDRESS OUT OF RANGE. do_device_access()
reduces the LBA modulo the size of the store, so the command writes
somewhere else on the medium instead. A WRITE ATOMIC (16) also writes to
a device whose wp module parameter is set, which every other write
refuses with DATA PROTECT.

Call check_device_access_params(). The zone checks that it ends with are
unreachable, as atomic writes and ZBC emulation are mutually exclusive.

Assisted-by: LLM
Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug sector_size=512 physblk_exp=3 dev_size_mb=128 \
      atomic_wr=1

The device has a capacity of 262144 logical blocks. A WRITE ATOMIC (16)
of eight blocks at LBA 262140 is now terminated with ILLEGAL REQUEST /
LOGICAL BLOCK ADDRESS OUT OF RANGE; before this patch it completed with
GOOD status, having written eight blocks at LBA 0 instead.

With the wp module parameter set to 1, a WRITE ATOMIC (16) is now
terminated with DATA PROTECT / LOGICAL UNIT SOFTWARE WRITE PROTECTED,
which is what an ordinary WRITE(16) has always returned; before this
patch it completed with GOOD status and wrote the data.
---
 drivers/scsi/scsi_debug.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 5243401701f9..5ae1241e4d8b 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6246,6 +6246,10 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 		}
 	}
 
+	ret = check_device_access_params(scp, lba, len, true);
+	if (ret)
+		return ret;
+
 	if (lbp) {
 		sdeb_meta_write_lock(sip);
 		meta_data_locked = true;
-- 
2.55.0

Re: [PATCH v4 02/10] scsi: scsi_debug: Make atomic writes and ZBC emulation mutually exclusive

[John Garry]

On 9/18/26 07:29, Niklas Cassel wrote:
> There are no SMR drives that support atomic writes. Emulating a device
> that supports both is emulating something that a user cannot encounter,
> rather than the drives that exist, and it means maintaining the zone
> handling of a command that no zoned device implements.
> 
> Nothing in the standards forbids the combination. For what would be by
> far the most common case, a host managed SMR drive whose logical block
> size equals its physical block size, there is no problem in the two
> features being combined: the drive vendor only has to give atomic
> writes an alignment that makes sense with regards to ZBC, for instance
> equal to the physical block size. This is therefore a choice not to
> complicate scsi_debug, rather than a restriction that the standards
> impose.
> 
> Refuse the combination at initialisation. If SMR drives that support
> atomic writes ever become a thing, this can be changed back.
> 
> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Assisted-by: LLM
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: John Garry <john.garry@linux.dev>

> ---
> Tested by loading the module with each combination:
> 
>    zbc=managed atomic_wr=1    refused, -EINVAL
>    zbc=aware   atomic_wr=1    refused, -EINVAL
>    zbc=managed                loads
>    atomic_wr=1                loads
> ---
>   drivers/scsi/scsi_debug.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index c664966e95cb..8e45a57ae406 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -8739,6 +8739,11 @@ static int __init scsi_debug_init(void)
>   		return -EINVAL;
>   	}
>   
> +	if (sdeb_zbc_in_use && sdebug_atomic_wr) {
> +		pr_err("atomic_wr is not supported by a zoned device\n");
> +		return -EINVAL;
> +	}
> +
>   	if (sdebug_dev_size_mb == DEF_DEV_SIZE_PRE_INIT)
>   		sdebug_dev_size_mb = DEF_DEV_SIZE_MB;
>   	if (sdebug_dev_size_mb < 1)

Re: [PATCH v4 09/10] scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

[John Garry]

On 9/18/26 07:29, Niklas Cassel wrote:
> When logical block provisioning is enabled, a command that writes user
> data marks the region that it wrote in the provisioning map, so that
> GET LBA STATUS reports the region as mapped. resp_write_dt0(),
> resp_write_scat() and resp_write_same() all call map_region() for that.
> 
> resp_atomic_write() does not, so a WRITE ATOMIC (16) leaves the
> provisioning map untouched, and GET LBA STATUS keeps reporting the
> region as deallocated after it has been written. map_state(), which
> GET LBA STATUS uses, is the only reader of the map, so that is the whole
> of the effect.
> 
> Call map_region() the way resp_write_dt0() does, and take the zone
> metadata write lock across the access as it does when logical block
> provisioning is enabled. That lock is what serialises the provisioning
> map against resp_unmap(), which holds it while unmap_region() clears map
> bits and zeroes the data that they cover. resp_unmap() does nothing
> unless logical block provisioning is enabled, so the lock is only needed
> in that case.
> 
> Assisted-by: LLM
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Tested with:
> 
>    modprobe scsi_debug sector_size=512 physblk_exp=3 dev_size_mb=128 \
>        atomic_wr=1 lbpu=1
> 
> GET LBA STATUS reports a region that has never been written as
> deallocated, and reports it as mapped after a WRITE ATOMIC (16) of eight
> blocks. Only an ordinary WRITE did so before this patch.
> ---
>   drivers/scsi/scsi_debug.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index cdea21b570c4..5243401701f9 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -6200,6 +6200,8 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
>   	u8 *cmd = scp->cmnd;
>   	u16 boundary, len;
>   	u64 lba, lba_tmp;
> +	bool meta_data_locked = false;
> +	bool lbp = scsi_debug_lbp();
>   	int ret;
>   
>   	if (!scsi_debug_atomic_write()) {
> @@ -6244,7 +6246,18 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
>   		}
>   	}
>   
> +	if (lbp) {
> +		sdeb_meta_write_lock(sip);
> +		meta_data_locked = true;
> +	}
> +
>   	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
> +	if (unlikely(lbp))
 > +		map_region(sip, lba, len);> +
> +	if (meta_data_locked)

Do we really need meta_data_locked variable? why not:
	if (lbp)
		sdeb_meta_write_unlock(sip);

> +		sdeb_meta_write_unlock(sip);
> +
>   	if (unlikely(ret == -1))
>   		return DID_ERROR << 16;
>   	if (unlikely(ret != len * sdebug_sector_size))

Re: [PATCH v4 10/10] scsi: scsi_debug: Validate the access parameters of WRITE ATOMIC (16)

[John Garry]

On 9/18/26 07:29, Niklas Cassel wrote:
> resp_atomic_write() validates the fields that are specific to an atomic
> write, the alignment and granularity of the transfer, the atomic
> boundary and the maximum transfer length, but it never validates the
> range that the command addresses. Every other command that writes user
> data calls check_device_access_params() first, which rejects a transfer
> that ends beyond the capacity of the device, one whose length exceeds
> the size of the store, and any write to a write protected device.
> 
> As a consequence a WRITE ATOMIC (16) past the end of the device is not
> terminated with LOGICAL BLOCK ADDRESS OUT OF RANGE. do_device_access()
> reduces the LBA modulo the size of the store, so the command writes
> somewhere else on the medium instead. A WRITE ATOMIC (16) also writes to
> a device whose wp module parameter is set, which every other write
> refuses with DATA PROTECT.
> 
> Call check_device_access_params(). The zone checks that it ends with are
> unreachable, as atomic writes and ZBC emulation are mutually exclusive.

These are quite verbose commit messages ... LLM-generated, by chance?

> 
> Assisted-by: LLM
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: John Garry <john.garry@linux.dev>

> ---
> Tested with:
> 
>    modprobe scsi_debug sector_size=512 physblk_exp=3 dev_size_mb=128 \
>        atomic_wr=1
> 
> The device has a capacity of 262144 logical blocks. A WRITE ATOMIC (16)
> of eight blocks at LBA 262140 is now terminated with ILLEGAL REQUEST /
> LOGICAL BLOCK ADDRESS OUT OF RANGE; before this patch it completed with
> GOOD status, having written eight blocks at LBA 0 instead.
> 
> With the wp module parameter set to 1, a WRITE ATOMIC (16) is now
> terminated with DATA PROTECT / LOGICAL UNIT SOFTWARE WRITE PROTECTED,
> which is what an ordinary WRITE(16) has always returned; before this
> patch it completed with GOOD status and wrote the data.
> ---
>   drivers/scsi/scsi_debug.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 5243401701f9..5ae1241e4d8b 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -6246,6 +6246,10 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
>   		}
>   	}
>   
> +	ret = check_device_access_params(scp, lba, len, true);
> +	if (ret)
> +		return ret;
> +
>   	if (lbp) {
>   		sdeb_meta_write_lock(sip);
>   		meta_data_locked = true;

Re: [PATCH v4 10/10] scsi: scsi_debug: Validate the access parameters of WRITE ATOMIC (16)

[Niklas Cassel]

On Fri, Sep 18, 2026 at 08:33:07AM +0100, John Garry wrote:
> On 9/18/26 07:29, Niklas Cassel wrote:
> > resp_atomic_write() validates the fields that are specific to an atomic
> > write, the alignment and granularity of the transfer, the atomic
> > boundary and the maximum transfer length, but it never validates the
> > range that the command addresses. Every other command that writes user
> > data calls check_device_access_params() first, which rejects a transfer
> > that ends beyond the capacity of the device, one whose length exceeds
> > the size of the store, and any write to a write protected device.
> > 
> > As a consequence a WRITE ATOMIC (16) past the end of the device is not
> > terminated with LOGICAL BLOCK ADDRESS OUT OF RANGE. do_device_access()
> > reduces the LBA modulo the size of the store, so the command writes
> > somewhere else on the medium instead. A WRITE ATOMIC (16) also writes to
> > a device whose wp module parameter is set, which every other write
> > refuses with DATA PROTECT.
> > 
> > Call check_device_access_params(). The zone checks that it ends with are
> > unreachable, as atomic writes and ZBC emulation are mutually exclusive.
> 
> These are quite verbose commit messages ... LLM-generated, by chance?

Yes, hence the Assisted-by tag just a few lines further down:

> 
> > 
> > Assisted-by: LLM
> > Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>

The commit message does point out two actual problems resulting from the
missing check_device_access_params() call in resp_atomic_write() (which
exists in all other resp_write_*() functions):

- Fails to repect the write protect module parameter, so resp_atomic_write()
  fails to generate the proper sense data in this case.

- Fails to check if the command will write past that device capacity, so
  resp_atomic_write() fails to generate the proper sense data in this case.

The generated sense data differs in the two cases.

I suppose we could drop:

  As a consequence a WRITE ATOMIC (16) past the end of the device is not
  terminated with LOGICAL BLOCK ADDRESS OUT OF RANGE. do_device_access()
  reduces the LBA modulo the size of the store, so the command writes
  somewhere else on the medium instead. A WRITE ATOMIC (16) also writes to
  a device whose wp module parameter is set, which every other write
  refuses with DATA PROTECT.


From the commit message, as I suppose it might be a bit overly verbose to
mention the exact sense data that each missing check would generate.
If someone cares about that, they can just look at the mk_sense_buffer()
calls in check_device_access_params().


Kind regards,
Niklas

Re: [PATCH v4 09/10] scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

[Niklas Cassel]

On Fri, Sep 18, 2026 at 08:29:15AM +0100, John Garry wrote:
> On 9/18/26 07:29, Niklas Cassel wrote:
> > When logical block provisioning is enabled, a command that writes user
> > data marks the region that it wrote in the provisioning map, so that
> > GET LBA STATUS reports the region as mapped. resp_write_dt0(),
> > resp_write_scat() and resp_write_same() all call map_region() for that.
> > 
> > resp_atomic_write() does not, so a WRITE ATOMIC (16) leaves the
> > provisioning map untouched, and GET LBA STATUS keeps reporting the
> > region as deallocated after it has been written. map_state(), which
> > GET LBA STATUS uses, is the only reader of the map, so that is the whole
> > of the effect.
> > 
> > Call map_region() the way resp_write_dt0() does, and take the zone
> > metadata write lock across the access as it does when logical block
> > provisioning is enabled. That lock is what serialises the provisioning
> > map against resp_unmap(), which holds it while unmap_region() clears map
> > bits and zeroes the data that they cover. resp_unmap() does nothing
> > unless logical block provisioning is enabled, so the lock is only needed
> > in that case.
> > 
> > Assisted-by: LLM
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> > Tested with:
> > 
> >    modprobe scsi_debug sector_size=512 physblk_exp=3 dev_size_mb=128 \
> >        atomic_wr=1 lbpu=1
> > 
> > GET LBA STATUS reports a region that has never been written as
> > deallocated, and reports it as mapped after a WRITE ATOMIC (16) of eight
> > blocks. Only an ordinary WRITE did so before this patch.
> > ---
> >   drivers/scsi/scsi_debug.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index cdea21b570c4..5243401701f9 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -6200,6 +6200,8 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
> >   	u8 *cmd = scp->cmnd;
> >   	u16 boundary, len;
> >   	u64 lba, lba_tmp;
> > +	bool meta_data_locked = false;
> > +	bool lbp = scsi_debug_lbp();
> >   	int ret;
> >   	if (!scsi_debug_atomic_write()) {
> > @@ -6244,7 +6246,18 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
> >   		}
> >   	}
> > +	if (lbp) {
> > +		sdeb_meta_write_lock(sip);
> > +		meta_data_locked = true;
> > +	}
> > +
> >   	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
> > +	if (unlikely(lbp))
> > +		map_region(sip, lba, len);> +
> > +	if (meta_data_locked)
> 
> Do we really need meta_data_locked variable? why not:
> 	if (lbp)
> 		sdeb_meta_write_unlock(sip);
> 

Yes, that would work for resp_atomic_write() as it does not support zones.

The code was cargo-culted from resp_write_dt0(), which does support zones.

Will fix in a v5. Will wait until Monday before I respin so that other
people (including Damien) get a chance to review v4.


Kind regards,
Niklas

Re: [PATCH v4 10/10] scsi: scsi_debug: Validate the access parameters of WRITE ATOMIC (16)

[John Garry]

On 9/18/26 08:53, Niklas Cassel wrote:
>>> Call check_device_access_params(). The zone checks that it ends with are
>>> unreachable, as atomic writes and ZBC emulation are mutually exclusive.
>> These are quite verbose commit messages ... LLM-generated, by chance?
> Yes, hence the Assisted-by tag just a few lines further down:

I didn't know which part was :)

> 
>>> Assisted-by: LLM
>>> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
>>> Signed-off-by: Niklas Cassel<cassel@kernel.org>
> The commit message does point out two actual problems resulting from the
> missing check_device_access_params() call in resp_atomic_write() (which
> exists in all other resp_write_*() functions):
> 
> - Fails to repect the write protect module parameter, so resp_atomic_write()
>    fails to generate the proper sense data in this case.
> 
> - Fails to check if the command will write past that device capacity, so
>    resp_atomic_write() fails to generate the proper sense data in this case.
> 
> The generated sense data differs in the two cases.
> 
> I suppose we could drop:
> 
>    As a consequence a WRITE ATOMIC (16) past the end of the device is not
>    terminated with LOGICAL BLOCK ADDRESS OUT OF RANGE. do_device_access()
>    reduces the LBA modulo the size of the store, so the command writes
>    somewhere else on the medium instead. A WRITE ATOMIC (16) also writes to
>    a device whose wp module parameter is set, which every other write
>    refuses with DATA PROTECT.
> 
> 
>  From the commit message, as I suppose it might be a bit overly verbose to
> mention the exact sense data that each missing check would generate.
> If someone cares about that, they can just look at the mk_sense_buffer()
> calls in check_device_access_params().

I don't really care too much. I personally just find these LLM-generated 
commit messages laborious to read.

Re: [PATCH v4 10/10] scsi: scsi_debug: Validate the access parameters of WRITE ATOMIC (16)

[Niklas Cassel]

On Fri, Sep 18, 2026 at 09:19:30AM +0100, John Garry wrote:
> On 9/18/26 08:53, Niklas Cassel wrote:
> > > > Call check_device_access_params(). The zone checks that it ends with are
> > > > unreachable, as atomic writes and ZBC emulation are mutually exclusive.
> > > These are quite verbose commit messages ... LLM-generated, by chance?
> > Yes, hence the Assisted-by tag just a few lines further down:
> 
> I didn't know which part was :)
> 
> > 
> > > > Assisted-by: LLM
> > > > Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> > > > Signed-off-by: Niklas Cassel<cassel@kernel.org>
> > The commit message does point out two actual problems resulting from the
> > missing check_device_access_params() call in resp_atomic_write() (which
> > exists in all other resp_write_*() functions):
> > 
> > - Fails to repect the write protect module parameter, so resp_atomic_write()
> >    fails to generate the proper sense data in this case.
> > 
> > - Fails to check if the command will write past that device capacity, so
> >    resp_atomic_write() fails to generate the proper sense data in this case.
> > 
> > The generated sense data differs in the two cases.
> > 
> > I suppose we could drop:
> > 
> >    As a consequence a WRITE ATOMIC (16) past the end of the device is not
> >    terminated with LOGICAL BLOCK ADDRESS OUT OF RANGE. do_device_access()
> >    reduces the LBA modulo the size of the store, so the command writes
> >    somewhere else on the medium instead. A WRITE ATOMIC (16) also writes to
> >    a device whose wp module parameter is set, which every other write
> >    refuses with DATA PROTECT.
> > 
> > 
> >  From the commit message, as I suppose it might be a bit overly verbose to
> > mention the exact sense data that each missing check would generate.
> > If someone cares about that, they can just look at the mk_sense_buffer()
> > calls in check_device_access_params().
> 
> I don't really care too much. I personally just find these LLM-generated
> commit messages laborious to read.

I do also often find LLM-generated commit messages very verbose.

But at the same time, I often find commit messages written by (most) humans
way too sparse.

Linus himself is a big fan of very verbose commit messages:
https://lore.kernel.org/all/20150314075357.GA8319@gmail.com/

But of course, there is a difference between very verbose commit messages
written by a human, and very verbose commit messages written by an LLM.

I doubt that LLM-generated commit messages are going away (and for non-native
speakers, I very much think that they are an improvement to what we were used
to), but as AI models get better and better, hopefully, they will eventually
learn how to write more concise commit messages by default.

I can imagine that it is already possible with an AI kernel skill.md that
instructs the agent to write commit messages more concise than their default.


Kind regards,
Niklas

Re: [PATCH v4 01/10] scsi: scsi_debug: Refuse a zoned device with a non-zero lowest aligned LBA

[Damien Le Moal]

On 2026/09/18 13:29, Niklas Cassel wrote:
> The LOWEST ALIGNED LOGICAL BLOCK ADDRESS field indicates the LBA of the
> first logical block that is located at the beginning of a physical
> block, see SBC-6 r02 (T10/BSR INCITS 587), 5.20.2. scsi_debug reports
> the lowest_aligned module parameter in that field of the READ CAPACITY
> (16) parameter data, but nothing else in the driver acts on it: the data
> it stores begins at LBA 0 and has no physical block structure behind it.
> 
> That does not matter for a device that is not zoned, where a write need
> not end on a physical block boundary at all. It does matter for a zoned
> device: ZBC requires a write to a sequential write required zone to end
> on a physical block boundary, and a zone begins at a multiple of the
> zone size, so a non-zero lowest aligned LBA puts the beginning of every
> zone in the middle of a physical block. A host that writes in units of
> the zone write granularity that it was given can then never satisfy the
> requirement, and such a zone cannot be written at all.
> 
> Refuse the combination rather than emulate a device that cannot be used.
> 
> Assisted-by: LLM
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 02/10] scsi: scsi_debug: Make atomic writes and ZBC emulation mutually exclusive

[Damien Le Moal]

On 2026/09/18 13:29, Niklas Cassel wrote:
> There are no SMR drives that support atomic writes. Emulating a device
> that supports both is emulating something that a user cannot encounter,
> rather than the drives that exist, and it means maintaining the zone
> handling of a command that no zoned device implements.
> 
> Nothing in the standards forbids the combination. For what would be by
> far the most common case, a host managed SMR drive whose logical block
> size equals its physical block size, there is no problem in the two
> features being combined: the drive vendor only has to give atomic
> writes an alignment that makes sense with regards to ZBC, for instance
> equal to the physical block size. This is therefore a choice not to
> complicate scsi_debug, rather than a restriction that the standards
> impose.
> 
> Refuse the combination at initialisation. If SMR drives that support
> atomic writes ever become a thing, this can be changed back.
> 
> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Assisted-by: LLM
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 03/10] scsi: scsi_debug: Take the zone metadata lock before the data lock

[Damien Le Moal]

On 2026/09/18 13:29, Niklas Cassel wrote:
> resp_comp_write() takes the data lock and then the zone metadata lock.
> Every other command that takes both takes them the other way round:
> resp_write_dt0(), resp_write_scat(), resp_write_same() and
> resp_write_tape() take the metadata lock and then call
> do_device_access(), which takes the data lock.
> 
> Two commands that take the same two locks in opposite orders can
> deadlock against each other, so a COMPARE AND WRITE and any other write
> to the same store can hang one another.
> 
> Take the locks in the same order as everywhere else, and release them in
> the reverse of that order.
> 
> Correct the comment above map_region() while here. The provisioning map
> is covered by the metadata lock rather than by the data lock, which is
> why resp_unmap() takes only the metadata lock while unmap_region()
> clears map bits.
> 
> Assisted-by: LLM
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 04/10] scsi: scsi_debug: Evaluate scsi_debug_lbp() only once

[Damien Le Moal]

On 2026/09/18 13:29, Niklas Cassel wrote:
> corrupt_lbas(), resp_write_dt0() and resp_write_same() call
> scsi_debug_lbp() to decide whether to take the zone metadata lock, and
> then call it again to decide whether to read or write the provisioning
> map that the lock protects.
> 
> The result is not a constant. scsi_debug_lbp() is false while the
> fake_rw module parameter is set, and fake_rw can be written at any time,
> both as a module parameter and through its driver attribute in sysfs.
> The two calls can therefore disagree, and the later one can decide to
> touch the provisioning map after the earlier one decided not to take the
> lock that protects it.
> 
> Call it once and use the result throughout.
> 
> Assisted-by: LLM
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 07/10] scsi: scsi_debug: Do not write a partial physical block to a zoned device

[Damien Le Moal]

On 2026/09/18 13:29, Niklas Cassel wrote:
> do_device_access() copies one logical block at a time and stops at the
> first short copy, so when the data-out buffer is smaller than the
> transfer length of the command it can write part of a physical block.
> An initiator can arrange that with SG_IO.
> 
> A write that does not end on a physical block boundary is perfectly
> acceptable to a device that is not zoned, and to a conventional zone,
> where the device reads, modifies and writes the physical block that the
> write falls in. ZBC-3 r06 (T10/BSR INCITS 579), 4.5.3.3.2, does require
> a write to a sequential write required zone to end on a physical block
> boundary, though, so a partly written physical block is not a state that
> such a zone can be left in.
> 
> Stop at the last whole physical block that the buffer holds, for a
> sequential write required zone only. The bytes that are left over are
> not written, and are reported to the initiator as part of the residual.
> 
> With the default physblk_exp=0 the physical block size equals the
> logical block size and this changes nothing. Nothing changes either when
> the buffer holds all of the data that the command asks for, so a command
> that transfers fewer logical blocks than a physical block is unaffected
> wherever it is legal.
> 
> Assisted-by: LLM
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

A bit shift would be nicer than a division... But nevertheless, looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 10/10] scsi: scsi_debug: Validate the access parameters of WRITE ATOMIC (16)

[Damien Le Moal]

On 2026/09/18 13:29, Niklas Cassel wrote:
> resp_atomic_write() validates the fields that are specific to an atomic
> write, the alignment and granularity of the transfer, the atomic
> boundary and the maximum transfer length, but it never validates the
> range that the command addresses. Every other command that writes user
> data calls check_device_access_params() first, which rejects a transfer
> that ends beyond the capacity of the device, one whose length exceeds
> the size of the store, and any write to a write protected device.
> 
> As a consequence a WRITE ATOMIC (16) past the end of the device is not
> terminated with LOGICAL BLOCK ADDRESS OUT OF RANGE. do_device_access()
> reduces the LBA modulo the size of the store, so the command writes
> somewhere else on the medium instead. A WRITE ATOMIC (16) also writes to
> a device whose wp module parameter is set, which every other write
> refuses with DATA PROTECT.
> 
> Call check_device_access_params(). The zone checks that it ends with are
> unreachable, as atomic writes and ZBC emulation are mutually exclusive.
> 
> Assisted-by: LLM
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 07/10] scsi: scsi_debug: Do not write a partial physical block to a zoned device

[Niklas Cassel]

On Fri, Sep 18, 2026 at 04:31:23PM +0700, Damien Le Moal wrote:
> On 2026/09/18 13:29, Niklas Cassel wrote:
> > do_device_access() copies one logical block at a time and stops at the
> > first short copy, so when the data-out buffer is smaller than the
> > transfer length of the command it can write part of a physical block.
> > An initiator can arrange that with SG_IO.
> > 
> > A write that does not end on a physical block boundary is perfectly
> > acceptable to a device that is not zoned, and to a conventional zone,
> > where the device reads, modifies and writes the physical block that the
> > write falls in. ZBC-3 r06 (T10/BSR INCITS 579), 4.5.3.3.2, does require
> > a write to a sequential write required zone to end on a physical block
> > boundary, though, so a partly written physical block is not a state that
> > such a zone can be left in.
> > 
> > Stop at the last whole physical block that the buffer holds, for a
> > sequential write required zone only. The bytes that are left over are
> > not written, and are reported to the initiator as part of the residual.
> > 
> > With the default physblk_exp=0 the physical block size equals the
> > logical block size and this changes nothing. Nothing changes either when
> > the buffer holds all of the data that the command asks for, so a command
> > that transfers fewer logical blocks than a physical block is unaffected
> > wherever it is legal.
> > 
> > Assisted-by: LLM
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> A bit shift would be nicer than a division... But nevertheless, looks good.
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

Sure, will apply the following change for this patch:

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 5ae1241e4d8b..bbd675ca47f2 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4313,7 +4313,8 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp,
                struct sdeb_zone_state *zsp = zbc_zone(devip, lba);

                if (zsp->z_type == ZBC_ZTYPE_SWR) {
-                       u32 avail = (sdb->length - sg_skip) / sdebug_sector_size;
+                       u32 avail = (sdb->length - sg_skip)
+                               >> ilog2(sdebug_sector_size);

                        if (avail < num)
                                num = round_down(avail, 1U << sdebug_physblk_exp);


And the following change for patch "scsi: scsi_debug: Advance the write
pointer over the data written":

@@ -5184,7 +5185,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 
        /* 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 / sdebug_sector_size);
+               zbc_inc_wp(devip, lba, ret >> ilog2(sdebug_sector_size));
        if (meta_data_locked)
                sdeb_meta_write_unlock(sip);
 
@@ -5350,7 +5351,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
                ret = do_device_access(sip, scp, sg_off, lba, num, group, true, true);
                /* 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 / sdebug_sector_size);
+                       zbc_inc_wp(devip, lba,
+                                  ret >> ilog2(sdebug_sector_size));
                if (unlikely(scsi_debug_lbp()))
                        map_region(sip, lba, num);
                if (unlikely(-1 == ret)) {


when respinning.


Kind regards,
Niklas