diff mbox series

[4/5] scsi: target: split out COMPARE AND WRITE memcmp into helper

Message ID 20201023205723.17880-5-ddiss@suse.de (mailing list archive)
State Superseded
Headers show
Series scsi: target: COMPARE AND WRITE miscompare sense | expand

Commit Message

David Disseldorp Oct. 23, 2020, 8:57 p.m. UTC 
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(-)

Comments

Bodo Stroesser Oct. 26, 2020, 4:44 p.m. UTC | #1 
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;
>   }
>   
>
David Disseldorp Oct. 26, 2020, 5:57 p.m. UTC | #2 
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
diff mbox series

Patch

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;
+}
+
 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;
 }