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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxx/
>
> 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
