| Message ID | 20200218180546.21313-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: target: fix unmap_zeroes_data boolean initialisation | expand |
On 2/18/20 10:05 AM, David Disseldorp wrote: > The LIO unmap_zeroes_data device attribute is mapped to the LBPRZ flag in > the READ CAPACITY (16) and Thin Provisioning VPD INQUIRY responses. > It's exposed via configfs, where any write value is correctly validated > via strtobool(), defaults to 0. However, when initialised via > target_configure_unmap_from_queue() it takes the value of the device's > max_write_zeroes_sectors queue limit, which is non-boolean. > > Fixes: 2237498f0b5c ("target/iblock: Convert WRITE_SAME to blkdev_issue_zeroout") > Signed-off-by: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_device.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c > index 2d19f0e332b0..2c7ba2f7e13c 100644 > --- a/drivers/target/target_core_device.c > +++ b/drivers/target/target_core_device.c > @@ -829,7 +829,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.max_write_zeroes_sectors); > + attrib->unmap_zeroes_data = !!(q->limits.max_write_zeroes_sectors); > return true; > } > EXPORT_SYMBOL(target_configure_unmap_from_queue); Hi David, How about changing the datatype of unmap_zeroes_data from 'int' into 'bool'? I think that change would have the same effect as this patch and additionally would make it clear that 'true' and 'false' are the only allowed variables for that struct member. Thanks, Bart.
Thanks for the feedback, Bart... On Tue, 18 Feb 2020 10:18:51 -0800, Bart Van Assche wrote: > On 2/18/20 10:05 AM, David Disseldorp wrote: > > --- a/drivers/target/target_core_device.c > > +++ b/drivers/target/target_core_device.c > > @@ -829,7 +829,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.max_write_zeroes_sectors); > > + attrib->unmap_zeroes_data = !!(q->limits.max_write_zeroes_sectors); > > return true; > > } > > EXPORT_SYMBOL(target_configure_unmap_from_queue); > > Hi David, > > How about changing the datatype of unmap_zeroes_data from 'int' into > 'bool'? I think that change would have the same effect as this patch and > additionally would make it clear that 'true' and 'false' are the only > allowed variables for that struct member. Yes, that'd also be an option, although my preference would be to change the type *and* carry the above hunk for readability. There are plenty of other configfs attrs which are validated via strtobool() and stored in an int. I guess it makes sense to also change them as a follow up. There's also still a question of how we deal with fixing configfs parsing tools which may have obtained an incorrect (> 1) unmap_zeroes_data value and expect to be able to write it back - should we relax the strtobool() check in unmap_zeroes_data_store() to handle mapping from >1 to true, or just leave it up to them to deal with? I'm leaning towards the latter. Cheers, David
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 2d19f0e332b0..2c7ba2f7e13c 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -829,7 +829,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.max_write_zeroes_sectors); + attrib->unmap_zeroes_data = !!(q->limits.max_write_zeroes_sectors); return true; } EXPORT_SYMBOL(target_configure_unmap_from_queue);
The LIO unmap_zeroes_data device attribute is mapped to the LBPRZ flag in the READ CAPACITY (16) and Thin Provisioning VPD INQUIRY responses. It's exposed via configfs, where any write value is correctly validated via strtobool(), defaults to 0. However, when initialised via target_configure_unmap_from_queue() it takes the value of the device's max_write_zeroes_sectors queue limit, which is non-boolean. Fixes: 2237498f0b5c ("target/iblock: Convert WRITE_SAME to blkdev_issue_zeroout") Signed-off-by: David Disseldorp <ddiss@suse.de> --- drivers/target/target_core_device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)