Re: [PATCH 3.16 106/217] sd: disable discard_zeroes_data for UNMAP

[Ben Hutchings]

On Wed, 2016-04-27 at 17:43 -0300, Rafael David Tinoco wrote:
> It seems that changing discard method from UNMAP to WRITE SAME(16)
> without using NDOB bit (as first described in sbc3r35b.pdf) can cause
> performance problems on big discards (since data-out buffer will be
> checked for every WRITE SAME command). I think this is happening after
> this commit, since NDOB bit wasn't implemented with this change
> (afaik, iirc).

Is that an objection, or just a comment?

I only picked this commit for backporting because it was referenced by
later fixes (commits 397737223c59,Âf4327a95dd08) and I read the commit
message as saying that it fixes data corruption (sd claims to be
writing zeroes but the whole area might not read back as zeroes). ÂIs
my understanding correct?

Ben.

> From the spec:
> """
> To ensure that subsequent read operations return all zeros in a
> logical block, use the WRITE SAME (16)
> command with the NDOB bit set to one. If the UNMAP bit is set to one,
> then the device server may unmap the logical blocks specified by the
> WRITE SAME (16)
> """
> 
> And there were some problems with this change (specifically QEMU SCSI
> WRITE SAME implementation). So the change (commit e461338b6cd4) was
> made to guarantee that if LBPRZ=0, after VPD 0xB2, UNMAP is still
> picked. WRITESAME(16) is picked only if LBPRZ=1. This last commit
> violated spec in favor of a WRITE SAME "optout" approach for QEMU.
> 
> I wonder if this should be taken to previous versions ...
> 
> -Rafael Tinoco
> 
> On Tue, Apr 26, 2016 at 8:02 PM, Ben Hutchings <ben@xxxxxxxxxxxxxxx> wrote:
> > 
> > 3.16.35-rc1 review patch.ÂÂIf anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: "Martin K. Petersen" <martin.petersen@xxxxxxxxxx>
> > 
> > commit 7985090aa0201fa7760583f9f8e6ba41a8d4c392 upstream.
> > 
> > The T10 SBC UNMAP command does not provide any hard guarantees that
> > blocks will return zeroes on a subsequent READ. This is due to the fact
> > that the device server is free to silently ignore all or parts of the
> > request.
> > 
> > The only way to ensure that a block consistently returns zeroes after
> > being unmapped is to use WRITE SAME with the UNMAP bit set. Should the
> > device be unable to unmap one or more blocks described by the command it
> > is required to manually write zeroes to them.
> > 
> > Until now we have preferred UNMAP over the WRITE SAME variants to
> > accommodate thinly provisioned devices that predated the final SBC-3
> > spec. This patch changes the heuristic so that we favor WRITE SAME(16)
> > or (10) over UNMAP if these commands are marked as supported in the
> > Logical Block Provisioning VPD page.
> > 
> > The patch also disables discard_zeroes_data for devices operating in
> > UNMAP mode.
> > 
> > Signed-off-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
> > Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > ---
> > Âdrivers/scsi/sd.c | 10 ++++++----
> > Â1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -627,7 +627,7 @@ static void sd_config_discard(struct scs
> > ÂÂÂÂÂÂÂÂunsigned int logical_block_size = sdkp->device->sector_size;
> > ÂÂÂÂÂÂÂÂunsigned int max_blocks = 0;
> > 
> > -ÂÂÂÂÂÂÂq->limits.discard_zeroes_data = sdkp->lbprz;
> > +ÂÂÂÂÂÂÂq->limits.discard_zeroes_data = 0;
> > ÂÂÂÂÂÂÂÂq->limits.discard_alignment = sdkp->unmap_alignment *
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlogical_block_size;
> > ÂÂÂÂÂÂÂÂq->limits.discard_granularity =
> > @@ -651,11 +651,13 @@ static void sd_config_discard(struct scs
> > ÂÂÂÂÂÂÂÂcase SD_LBP_WS16:
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmax_blocks = min_not_zero(sdkp->max_ws_blocks,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(u32)SD_MAX_WS16_BLOCKS);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂq->limits.discard_zeroes_data = sdkp->lbprz;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > 
> > ÂÂÂÂÂÂÂÂcase SD_LBP_WS10:
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmax_blocks = min_not_zero(sdkp->max_ws_blocks,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(u32)SD_MAX_WS10_BLOCKS);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂq->limits.discard_zeroes_data = sdkp->lbprz;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > 
> > ÂÂÂÂÂÂÂÂcase SD_LBP_ZERO:
> > @@ -2572,12 +2574,12 @@ static void sd_read_block_limits(struct
> > 
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ} else {ÂÂÂÂÂÂÂÂ/* LBP VPD page tells us what to use */
> > 
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (sdkp->lbpu && sdkp->max_unmap_blocks)
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsd_config_discard(sdkp, SD_LBP_UNMAP);
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse if (sdkp->lbpws)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (sdkp->lbpws)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsd_config_discard(sdkp, SD_LBP_WS16);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse if (sdkp->lbpws10)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsd_config_discard(sdkp, SD_LBP_WS10);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse if (sdkp->lbpu && sdkp->max_unmap_blocks)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsd_config_discard(sdkp, SD_LBP_UNMAP);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsd_config_discard(sdkp, SD_LBP_DISABLE);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > 
> 
> 
-- 
Ben Hutchings
All extremists should be taken out and shot.

Attachment: signature.asc
Description: This is a digitally signed message part