| Message ID | 20171219134517.17296-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(+)