Hi Jamie,
Apologies for the delayed follow-up. Comments below.
On Thu, 2015-11-19 at 12:30 +0000, Pocas, Jamie wrote:
> Sorry for the ungodly subject length (and this top post, which seems
> appropriate given the circumstances). I have been using subversion a
> lot recently and so I forgot to leave a separate brief line in the git
> commit message. I have already amended locally. Please let me know if
> I should retransmit and start over with a new thread.
> ________________________________________
> From: Jamie Pocas [jamie.pocas@xxxxxxx]
> Sent: Thursday, November 19, 2015 7:09 AM
> To: target-devel@xxxxxxxxxxxxxxx
> Cc: Pocas, Jamie
> Subject: [PATCH] This change sets the LBPRZ flag in EVPD page b2h and
> READ CAPACITY (16) based on a new unmap_zeroes_data device attribute.
> This flag is set automatically for iblock based on underlying block
> device queue's discard_zeroes_data flag.
>
> Signed-off-by: Jamie Pocas <jamie.pocas@xxxxxxx>
> ---
> drivers/target/target_core_configfs.c | 36 +++++++++++++++++++++++++++++++++++
> drivers/target/target_core_device.c | 2 ++
> drivers/target/target_core_iblock.c | 2 ++
> drivers/target/target_core_sbc.c | 10 +++++++++-
> drivers/target/target_core_spc.c | 12 ++++++++++++
> include/target/target_core_base.h | 3 +++
> 6 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 860e840..dfb50ea 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -511,6 +511,7 @@ DEF_TB_DEV_ATTRIB_SHOW(max_unmap_lba_count);
> DEF_TB_DEV_ATTRIB_SHOW(max_unmap_block_desc_count);
> DEF_TB_DEV_ATTRIB_SHOW(unmap_granularity);
> DEF_TB_DEV_ATTRIB_SHOW(unmap_granularity_alignment);
> +DEF_TB_DEV_ATTRIB_SHOW(unmap_zeroes_data);
> DEF_TB_DEV_ATTRIB_SHOW(max_write_same_len);
>
> #define DEF_TB_DEV_ATTRIB_STORE_U32(_name) \
> @@ -866,6 +867,39 @@ static ssize_t store_emulate_rest_reord(struct se_dev_attrib *da,
> return count;
> }
>
> +static ssize_t store_unmap_zeroes_data(struct se_dev_attrib *da,
> + const char *page, size_t count)
> +{
> + bool flag;
> + int ret;
> +
> + ret = strtobool(page, &flag);
> + if (ret < 0)
> + return ret;
> +
> + if (da->da_dev->export_count) {
> + pr_err("dev[%p]: Unable to change SE Device"
> + " unmap_zeroes_data while export_count is %d\n",
> + da->da_dev, da->da_dev->export_count);
> + return -EINVAL;
> + }
> + /*
> + * We expect this value to be non-zero when generic Block Layer
> + * Discard supported is detected iblock_configure_device().
> + */
> + if (flag && !da->max_unmap_block_desc_count) {
> + pr_err("dev[%p]: Thin Provisioning LBPRZ will not be set"
> + "because max_unmap_block_desc_count is zero\n",
> + da->da_dev);
> + return -ENOSYS;
> + }
> + da->unmap_zeroes_data = flag;
> + pr_debug("dev[%p]: SE Device Thin Provisioning LBPRZ bit: %d\n",
> + da->da_dev, flag);
> + return 0;
> +}
> +
> +
> /*
> * Note, this can only be called on unexported SE Device Object.
> */
> @@ -1007,6 +1041,7 @@ TB_DEV_ATTR(target_core, max_unmap_lba_count, S_IRUGO | S_IWUSR);
> TB_DEV_ATTR(target_core, max_unmap_block_desc_count, S_IRUGO | S_IWUSR);
> TB_DEV_ATTR(target_core, unmap_granularity, S_IRUGO | S_IWUSR);
> TB_DEV_ATTR(target_core, unmap_granularity_alignment, S_IRUGO | S_IWUSR);
> +TB_DEV_ATTR(target_core, unmap_zeroes_data, S_IRUGO | S_IWUSR);
> TB_DEV_ATTR(target_core, max_write_same_len, S_IRUGO | S_IWUSR);
>
> CONFIGFS_EATTR_STRUCT(target_core_dev_attrib, se_dev_attrib);
> @@ -1046,6 +1081,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
> &target_core_dev_attrib_max_unmap_block_desc_count.attr,
> &target_core_dev_attrib_unmap_granularity.attr,
> &target_core_dev_attrib_unmap_granularity_alignment.attr,
> + &target_core_dev_attrib_unmap_zeroes_data.attr,
> &target_core_dev_attrib_max_write_same_len.attr,
> NULL,
> };
Note the way that configfs attributes work has changed significantly in
>= v4.4-rc1 code.
<SNIP>
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index f87d4ce..440322d 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -628,6 +628,18 @@ spc_emulate_evpd_b2(struct se_cmd *cmd, unsigned char *buf)
> if (dev->dev_attrib.emulate_tpws != 0)
> buf[5] |= 0x40 | 0x20;
>
> + /*
> + * 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) |
^^^^
I assume is a typo and should be a "||". Fixing up now.
> + (dev->dev_attrib.emulate_tpws != 0)) &&
> + (dev->dev_attrib.unmap_zeroes_data != 0))
> + buf[5] |= 0x04;
> +
> return 0;
> }
>
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 17ae2d6..a6371d4 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -62,6 +62,8 @@
> #define DA_UNMAP_GRANULARITY_DEFAULT 0
> /* Default unmap_granularity_alignment */
> #define DA_UNMAP_GRANULARITY_ALIGNMENT_DEFAULT 0
> +/* Default unmap_zeroes_data */
> +#define DA_UNMAP_ZEROES_DATA_DEFAULT 0
> /* Default max_write_same_len, disabled by default */
> #define DA_MAX_WRITE_SAME_LEN 0
> /* Use a model alias based on the configfs backend device name */
> @@ -651,6 +653,7 @@ struct se_dev_attrib {
> int force_pr_aptpl;
> int is_nonrot;
> int emulate_rest_reord;
> + int unmap_zeroes_data;
> u32 hw_block_size;
> u32 block_size;
> u32 hw_max_sectors;
> --
Ok, separate from these items the changes looks fine.
I've fixed up the configfs bit + typo above, and the updated version has
been applied to target-pending/for-next code here:
https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?h=for-next&id=c8ac1c36a82d181dd5801bf09656a572cdecbaa0
One other thing though, this patch is whitespace broken. You'll want to
consider using git-send-email in the future so TABs aren't lost when
posting patches.
Thank you,
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
