[RFC PATCH] target: enable discard if supported by underlying block device

[RFC PATCH] target: enable discard if supported by underlying block device

[David Disseldorp]

The initial commit of LIO (v4.0.0-rc6) came with support for SCSI UNMAP
disabled. This was justified in the LIO wiki with:
  Many SATA SSDs have issues handling UNMAP properly, so it is disabled
  per default in LIO.
Since this time, some critical discard bugs have been fixed, e.g.
f3f5da624e0a ("block: Do a full clone when splitting discard bios").

Block layer backed logical units are capable of deferring to the block
layer when determining UNMAP attributes, but this logic currently leaves
the emulate_tpu (UNMAP) and emulate_tpws (WRITE_SAME with UNMAP=1) flags
disabled.
With this change, emulate_tpu and emulate_tpws are set for block layer
backed logical units based on underlying block device discard support.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/target/target_core_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index e8dd6da164b2..65dab7c2460a 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -856,6 +856,8 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
 	attrib->unmap_granularity_alignment = q->limits.discard_alignment /
 								block_size;
 	attrib->unmap_zeroes_data = (q->limits.max_write_zeroes_sectors);
+	attrib->emulate_tpu = 1;
+	attrib->emulate_tpws = 1;
 	return true;
 }
 EXPORT_SYMBOL(target_configure_unmap_from_queue);
-- 
2.13.6

Re: [RFC PATCH] target: enable discard if supported by underlying block device

[David Disseldorp]

Ping, any thoughts on this one? I think it makes sense to enable unmap
in this case.

Cheers, David

On Tue, 19 Dec 2017 14:45:17 +0100, David Disseldorp wrote:

> The initial commit of LIO (v4.0.0-rc6) came with support for SCSI UNMAP
> disabled. This was justified in the LIO wiki with:
>   Many SATA SSDs have issues handling UNMAP properly, so it is disabled
>   per default in LIO.
> Since this time, some critical discard bugs have been fixed, e.g.
> f3f5da624e0a ("block: Do a full clone when splitting discard bios").
> 
> Block layer backed logical units are capable of deferring to the block
> layer when determining UNMAP attributes, but this logic currently leaves
> the emulate_tpu (UNMAP) and emulate_tpws (WRITE_SAME with UNMAP=1) flags
> disabled.
> With this change, emulate_tpu and emulate_tpws are set for block layer
> backed logical units based on underlying block device discard support.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index e8dd6da164b2..65dab7c2460a 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -856,6 +856,8 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
>  	attrib->unmap_granularity_alignment = q->limits.discard_alignment /
>  								block_size;
>  	attrib->unmap_zeroes_data = (q->limits.max_write_zeroes_sectors);
> +	attrib->emulate_tpu = 1;
> +	attrib->emulate_tpws = 1;
>  	return true;
>  }
>  EXPORT_SYMBOL(target_configure_unmap_from_queue);

Re: [RFC PATCH] target: enable discard if supported by underlying block device

[Mike Christie]

On 12/19/2017 07:45 AM, David Disseldorp wrote:
> The initial commit of LIO (v4.0.0-rc6) came with support for SCSI UNMAP
> disabled. This was justified in the LIO wiki with:
>   Many SATA SSDs have issues handling UNMAP properly, so it is disabled
>   per default in LIO.
> Since this time, some critical discard bugs have been fixed, e.g.
> f3f5da624e0a ("block: Do a full clone when splitting discard bios").
> 
> Block layer backed logical units are capable of deferring to the block
> layer when determining UNMAP attributes, but this logic currently leaves
> the emulate_tpu (UNMAP) and emulate_tpws (WRITE_SAME with UNMAP=1) flags
> disabled.

Is there any cases where the user set tpu/tpws to off on purpose to
work around a backing device or initiator side issue? If so, I think the
only concern would be for non-rtslib based apps. If they did the attrib
setting stage after device creation but before the config stage, then
this patch would overwrite those settings.

For rtslib based apps, if the user had set tpu/tpws values to off on
purpose, it looks like the user requested values will still be used
since the attrib setting stage is done after the configure.

If we do not support non rtslib based apps then the patch is nice for
users and looks ok.

Reviewed-by: Mike Christie <mchristi@redhat.com>

Re: [RFC PATCH] target: enable discard if supported by underlying block device

[David Disseldorp]

Thanks for the feedback, Mike.

On Wed, 3 Jan 2018 12:33:32 -0600, Mike Christie wrote:

> > Block layer backed logical units are capable of deferring to the block
> > layer when determining UNMAP attributes, but this logic currently leaves
> > the emulate_tpu (UNMAP) and emulate_tpws (WRITE_SAME with UNMAP=1) flags
> > disabled.  
> 
> Is there any cases where the user set tpu/tpws to off on purpose to
> work around a backing device or initiator side issue? If so, I think the
> only concern would be for non-rtslib based apps. If they did the attrib
> setting stage after device creation but before the config stage, then
> this patch would overwrite those settings.

I'm not aware of any such cases, but yes, good point. If this is a
concern, then another option would be to change the DA_EMULATE_TPU/TPWS
defaults unconditionally (for all backstores), but I that'd lead to
other consequences (e.g. on FSes that don't support PUNCH_HOLE).

> For rtslib based apps, if the user had set tpu/tpws values to off on
> purpose, it looks like the user requested values will still be used
> since the attrib setting stage is done after the configure.
> 
> If we do not support non rtslib based apps then the patch is nice for
> users and looks ok.

FWIW, I frequently use LIO via configfs directly (without rtslib), so
would like to ensure continued compatibility.

> Reviewed-by: Mike Christie <mchristi@redhat.com>

Cheers, David

Re: [RFC PATCH] target: enable discard if supported by underlying block device

[Nicholas A. Bellinger]

Hi David & MNC,

On Wed, 2018-01-03 at 23:27 +0100, David Disseldorp wrote:
> Thanks for the feedback, Mike.
> 
> On Wed, 3 Jan 2018 12:33:32 -0600, Mike Christie wrote:
> 
> > > Block layer backed logical units are capable of deferring to the block
> > > layer when determining UNMAP attributes, but this logic currently leaves
> > > the emulate_tpu (UNMAP) and emulate_tpws (WRITE_SAME with UNMAP=1) flags
> > > disabled.  
> > 
> > Is there any cases where the user set tpu/tpws to off on purpose to
> > work around a backing device or initiator side issue? If so, I think the
> > only concern would be for non-rtslib based apps. If they did the attrib
> > setting stage after device creation but before the config stage, then
> > this patch would overwrite those settings.
> 
> I'm not aware of any such cases, but yes, good point. If this is a
> concern, then another option would be to change the DA_EMULATE_TPU/TPWS
> defaults unconditionally (for all backstores), but I that'd lead to
> other consequences (e.g. on FSes that don't support PUNCH_HOLE).
> 

I'm reluctant to enable tpu/tpws by default, even when underlying
device claims to support it.  Primarily because of two issues:

First for IBLOCK, blkdev_issue_discard() does synchronous bio
completion.  As-is, this causes iscsi-target RX kthreads to block for
extended periods of time resulting in NOPs triggering connection
reinstatement, et al.

In practice though, out-of-tree bio based backend drivers which actually
enable UNMAP + WRITE_SAME w/ UNMAP=1 always end up adding some
submit_bio() + async completion mechanism for production usage.

Second, even in my experience with async REQ_OP_DISCARD completion using
fast struct block_devices, it's still not very hard for a host (like
ESX/VMFS for example) to run into SCSI timeouts with heavy small block
I/O + UNMAP workloads.

So enabling this by default for all cases still makes me nervous,
without any async REQ_OP_DISCARD completion in place for upstream.