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

[Niklas Cassel <cassel@kernel.org>]

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