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

[John Garry <john.garry@linux.dev>]

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