[Drbd-dev] [PATCH 25/27] block: remove the discard_zeroes_data flag

[Nicholas A. Bellinger]

On Mon, 2017-05-01 at 23:43 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2017-05-01 at 20:45 +0000, Bart Van Assche wrote:
> > On Wed, 2017-04-05 at 19:21 +0200, Christoph Hellwig wrote:
> > > Now that we use the proper REQ_OP_WRITE_ZEROES operation everywhere we can
> > > kill this hack.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch at lst.de>
> > > Reviewed-by: Martin K. Petersen <martin.petersen at oracle.com>
> > > Reviewed-by: Hannes Reinecke <hare at suse.com>
> > > [ ... ]
> > > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> > > index c754ae33bf7b..d2f089cfa9ae 100644
> > > --- a/drivers/target/target_core_device.c
> > > +++ b/drivers/target/target_core_device.c
> > > @@ -851,7 +851,7 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
> > >  	attrib->unmap_granularity = q->limits.discard_granularity / block_size;
> > >  	attrib->unmap_granularity_alignment = q->limits.discard_alignment /
> > >  								block_size;
> > > -	attrib->unmap_zeroes_data = q->limits.discard_zeroes_data;
> > > +	attrib->unmap_zeroes_data = 0;
> > >  	return true;
> > >  }
> > >  EXPORT_SYMBOL(target_configure_unmap_from_queue);
> > 
> > Hello Christoph,
> > 
> > Sorry that I hadn't noticed this before but I think that this patch
> > introduces a significant performance regressions for LIO users. Before
> > this patch the LBPRZ flag was reported correctly to initiator systems
> > through the thin provisioning VPD. With this patch applied that flag
> > will always be reported as zero, forcing initiators to submit WRITE
> > commands with zeroed data buffers instead of submitting the SCSI UNMAP
> > command to block devices for which discard_zeroes_data was set. From
> > target_core_spc.c:
> > 
> > /* Thin Provisioning VPD */
> > static sense_reason_t spc_emulate_evpd_b2(struct se_cmd *cmd, unsigned char *buf)
> > {
> > 	[ ... ]
> > 	/*
> > 	 * The unmap_zeroes_data set means that the underlying device supports
> > 	 * REQ_DISCARD and has the discard_zeroes_data bit set. This satisfies
> > 	 * the SBC requirements for LBPRZ, meaning that a subsequent read
> > 	 * will return zeroes after an UNMAP or WRITE SAME (16) to an LBA
> > 	 * See sbc4r36 6.6.4.
> > 	 */
> > 	if (((dev->dev_attrib.emulate_tpu != 0) ||
> > 	     (dev->dev_attrib.emulate_tpws != 0)) &&
> > 	     (dev->dev_attrib.unmap_zeroes_data != 0))
> > 		buf[5] |= 0x04;
> > 	[ ... ]
> > }
> > 
> 
> According to sd_config_discard(), it's SD_LBP_WS16, SD_LBP_WS10 and
> SD_LBP_ZERO that where ever setting unmap_zeros_data = 1 to begin with.
> For UNMAP, q->limits.discard_zeroes_data was never set.
> 
> That said, it's pretty much implied that supporting DISCARD means
> subsequent READs return zeros, so target_configure_unmap_from_queue()
> should be setting attrib->unmap_zeroes_data = 1, or just dropping it
> all-together.
> 
> Post -rc1, I'll push a patch to do the latter.
> 

Or, another options is use bdev_write_zeroes_sectors() to determine when
dev_attrib->unmap_zeroes_data should be set.