| Message ID | 20201023205723.17880-1-ddiss@suse.de |
|---|---|
| Headers | show |
| Series | scsi: target: COMPARE AND WRITE miscompare sense | expand |
Am 23.10.20 um 22:57 schrieb David Disseldorp: > In preparation for finding and returning the miscompare offset. > > Signed-off-by: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_sbc.c | 117 ++++++++++++++++++------------- > 1 file changed, 67 insertions(+), 50 deletions(-) > > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c > index 5f77dd95f1b9..79216d0355e7 100644 > --- a/drivers/target/target_core_sbc.c > +++ b/drivers/target/target_core_sbc.c > @@ -434,20 +434,77 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success, > return ret; > } > > +/* > + * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare return > + * TCM_MISCOMPARE_VERIFY. > + */ > +static sense_reason_t > +compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents, > + struct scatterlist *cmp_sgl, unsigned int cmp_nents, > + unsigned int cmp_len) > +{ > + unsigned char *buf = NULL; > + struct scatterlist *sg; > + sense_reason_t ret; > + unsigned int offset; > + size_t rc; > + int i; > + > + buf = kzalloc(cmp_len, GFP_KERNEL); > + if (!buf) { > + pr_err("Unable to allocate compare_and_write buf\n"); > + ret = TCM_OUT_OF_RESOURCES; > + goto out; > + } > + > + rc = sg_copy_to_buffer(cmp_sgl, cmp_nents, buf, cmp_len); > + if (!rc) { > + pr_err("sg_copy_to_buffer() failed for compare_and_write\n"); > + ret = TCM_OUT_OF_RESOURCES; > + goto out; > + } > + /* > + * Compare SCSI READ payload against verify payload > + */ > + offset = 0; > + for_each_sg(read_sgl, sg, read_nents, i) { > + unsigned int len = min(sg->length, cmp_len); > + unsigned char *addr = kmap_atomic(sg_page(sg)); > + > + if (memcmp(addr, buf + offset, len)) { > + pr_warn("Detected MISCOMPARE for addr: %p buf: %p\n", > + addr, buf + offset); > + kunmap_atomic(addr); > + ret = TCM_MISCOMPARE_VERIFY; > + goto out; > + } > + kunmap_atomic(addr); > + > + offset += len; > + cmp_len -= len; > + if (!cmp_len) > + break; > + } > + pr_debug("COMPARE AND WRITE read data matches compare data\n"); > + ret = TCM_NO_SENSE; > +out: > + kfree(buf); > + return ret; > +} > + Since you are going to split out a new helper, did you consider to re-write helper's code to avoid the intermediate buffer? Douglas Gilbert currently tries to add new functions to lib/scatterlist.c One of them is sgl_compare_sgl, which directly compares content of two sg lists: https://patchwork.kernel.org/project/linux-block/patch/20201019191928.77845-4-dgilbert@interlog.com/ This code - based on the sg_miter_* calls - works without intermediate buffer. Maybe your helper could use similar code or you could even call Douglas' helper, if he can enhance it to (optionally) return the miscompare offset. > static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool success, > int *post_ret) > { > struct se_device *dev = cmd->se_dev; > struct sg_table write_tbl = { }; > - struct scatterlist *write_sg, *sg; > - unsigned char *buf = NULL, *addr; > + struct scatterlist *write_sg; > struct sg_mapping_iter m; > - unsigned int offset = 0, len; > + unsigned int len; > unsigned int nlbas = cmd->t_task_nolb; > unsigned int block_size = dev->dev_attrib.block_size; > unsigned int compare_len = (nlbas * block_size); > sense_reason_t ret = TCM_NO_SENSE; > - int rc, i; > + int i; > > /* > * Handle early failure in transport_generic_request_failure(), > @@ -473,12 +530,13 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes > goto out; > } > > - buf = kzalloc(cmd->data_length, GFP_KERNEL); > - if (!buf) { > - pr_err("Unable to allocate compare_and_write buf\n"); > - ret = TCM_OUT_OF_RESOURCES; > + ret = compare_and_write_do_cmp(cmd->t_bidi_data_sg, > + cmd->t_bidi_data_nents, > + cmd->t_data_sg, > + cmd->t_data_nents, > + compare_len); > + if (ret) > goto out; > - } > > if (sg_alloc_table(&write_tbl, cmd->t_data_nents, GFP_KERNEL) < 0) { > pr_err("Unable to allocate compare_and_write sg\n"); > @@ -486,41 +544,6 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes > goto out; > } > write_sg = write_tbl.sgl; > - /* > - * Setup verify and write data payloads from total NumberLBAs. > - */ > - rc = sg_copy_to_buffer(cmd->t_data_sg, cmd->t_data_nents, buf, > - cmd->data_length); > - if (!rc) { > - pr_err("sg_copy_to_buffer() failed for compare_and_write\n"); > - ret = TCM_OUT_OF_RESOURCES; > - goto out; > - } > - /* > - * Compare against SCSI READ payload against verify payload > - */ > - for_each_sg(cmd->t_bidi_data_sg, sg, cmd->t_bidi_data_nents, i) { > - addr = (unsigned char *)kmap_atomic(sg_page(sg)); > - if (!addr) { > - ret = TCM_OUT_OF_RESOURCES; > - goto out; > - } > - > - len = min(sg->length, compare_len); > - > - if (memcmp(addr, buf + offset, len)) { > - pr_warn("Detected MISCOMPARE for addr: %p buf: %p\n", > - addr, buf + offset); > - kunmap_atomic(addr); > - goto miscompare; > - } > - kunmap_atomic(addr); > - > - offset += len; > - compare_len -= len; > - if (!compare_len) > - break; > - } > > i = 0; > len = cmd->t_task_nolb * block_size; > @@ -568,13 +591,8 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes > > __target_execute_cmd(cmd, false); > > - kfree(buf); > return ret; > > -miscompare: > - pr_warn("Target/%s: Send MISCOMPARE check condition and sense\n", > - dev->transport->name); > - ret = TCM_MISCOMPARE_VERIFY; > out: > /* > * In the MISCOMPARE or failure case, unlock ->caw_sem obtained in > @@ -582,7 +600,6 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes > */ > up(&dev->caw_sem); > sg_free_table(&write_tbl); > - kfree(buf); > return ret; > } > >
Hi Bodo, On Mon, 26 Oct 2020 17:44:50 +0100, Bodo Stroesser wrote: > Am 23.10.20 um 22:57 schrieb David Disseldorp: > > In preparation for finding and returning the miscompare offset. > > > > Signed-off-by: David Disseldorp <ddiss@suse.de> > > --- > > drivers/target/target_core_sbc.c | 117 ++++++++++++++++++------------- > > 1 file changed, 67 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c > > index 5f77dd95f1b9..79216d0355e7 100644 > > --- a/drivers/target/target_core_sbc.c > > +++ b/drivers/target/target_core_sbc.c > > @@ -434,20 +434,77 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success, > > return ret; > > } > > > > +/* > > + * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare return > > + * TCM_MISCOMPARE_VERIFY. > > + */ > > +static sense_reason_t > > +compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents, > > + struct scatterlist *cmp_sgl, unsigned int cmp_nents, > > + unsigned int cmp_len) > > +{ > > + unsigned char *buf = NULL; > > + struct scatterlist *sg; > > + sense_reason_t ret; > > + unsigned int offset; > > + size_t rc; > > + int i; > > + > > + buf = kzalloc(cmp_len, GFP_KERNEL); > > + if (!buf) { > > + pr_err("Unable to allocate compare_and_write buf\n"); > > + ret = TCM_OUT_OF_RESOURCES; > > + goto out; > > + } > > + > > + rc = sg_copy_to_buffer(cmp_sgl, cmp_nents, buf, cmp_len); > > + if (!rc) { > > + pr_err("sg_copy_to_buffer() failed for compare_and_write\n"); > > + ret = TCM_OUT_OF_RESOURCES; > > + goto out; > > + } > > + /* > > + * Compare SCSI READ payload against verify payload > > + */ > > + offset = 0; > > + for_each_sg(read_sgl, sg, read_nents, i) { > > + unsigned int len = min(sg->length, cmp_len); > > + unsigned char *addr = kmap_atomic(sg_page(sg)); > > + > > + if (memcmp(addr, buf + offset, len)) { > > + pr_warn("Detected MISCOMPARE for addr: %p buf: %p\n", > > + addr, buf + offset); > > + kunmap_atomic(addr); > > + ret = TCM_MISCOMPARE_VERIFY; > > + goto out; > > + } > > + kunmap_atomic(addr); > > + > > + offset += len; > > + cmp_len -= len; > > + if (!cmp_len) > > + break; > > + } > > + pr_debug("COMPARE AND WRITE read data matches compare data\n"); > > + ret = TCM_NO_SENSE; > > +out: > > + kfree(buf); > > + return ret; > > +} > > + > > Since you are going to split out a new helper, did you consider to re-write helper's code to avoid the intermediate buffer? > > Douglas Gilbert currently tries to add new functions to lib/scatterlist.c > One of them is sgl_compare_sgl, which directly compares content of two sg lists: > https://patchwork.kernel.org/project/linux-block/patch/20201019191928.77845-4-dgilbert@interlog.com/ > > This code - based on the sg_miter_* calls - works without intermediate buffer. > Maybe your helper could use similar code or you could even call Douglas' helper, if he can enhance it to > (optionally) return the miscompare offset. Interesting, thanks for the heads-up. Dropping the intermediate compare buffer would be good, but I think this optimization should be left for a separate patchset, at least until sgl_compare_sgl() lands in mainline. LIO currently only supports 1-block compare-and-write requests, so the performance benefits would likely only be modest. @Douglas: would you be open to returning the miscompare offset from sgl_compare_sgl()? Cheers, David