* [PATCH v2 0/4] scsi: target: COMPARE AND WRITE miscompare sense
@ 2020-10-30 21:39 ` David Disseldorp
0 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi
This patchset adds missing functionality to return the offset of
non-matching read/compare data in the sense INFORMATION field on
COMPARE AND WRITE miscompare.
The functionality can be tested using the libiscsi
CompareAndWrite.MiscompareSense test proposed via:
https://github.com/sahlberg/libiscsi/pull/344
Changes since v2:
- perform bad_sector->sense_info rename in overlooked ib_isert
- drop scatterlist change queued via Jens' tree
- add Mike's reviewed-by tag
- rebase against Martin's 5.11/scsi-queue branch
Changes since v1:
- drop unnecessary WARN_ON()
- fix two checkpatch warnings
- drop single-use nlbas variable
- avoid compare_len recalculation
Cheers, David
drivers/infiniband/ulp/isert/ib_isert.c | 4 +-
drivers/target/target_core_sbc.c | 137 ++++++++++++++----------
drivers/target/target_core_transport.c | 33 +++---
include/target/target_core_base.h | 2 +-
4 files changed, 103 insertions(+), 73 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/4] scsi: target: rename struct sense_info to sense_detail
2020-10-30 21:39 ` David Disseldorp
@ 2020-10-30 21:39 ` David Disseldorp
-1 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
This helps distinguish it from the SCSI sense INFORMATION field.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
---
drivers/target/target_core_transport.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index ff26ab0a5f60..caa3a7b34826 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3094,14 +3094,14 @@ bool transport_wait_for_tasks(struct se_cmd *cmd)
}
EXPORT_SYMBOL(transport_wait_for_tasks);
-struct sense_info {
+struct sense_detail {
u8 key;
u8 asc;
u8 ascq;
bool add_sector_info;
};
-static const struct sense_info sense_info_table[] = {
+static const struct sense_detail sense_detail_table[] = {
[TCM_NO_SENSE] = {
.key = NOT_READY
},
@@ -3261,39 +3261,39 @@ static const struct sense_info sense_info_table[] = {
*/
static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
{
- const struct sense_info *si;
+ const struct sense_detail *sd;
u8 *buffer = cmd->sense_buffer;
int r = (__force int)reason;
u8 key, asc, ascq;
bool desc_format = target_sense_desc_format(cmd->se_dev);
- if (r < ARRAY_SIZE(sense_info_table) && sense_info_table[r].key)
- si = &sense_info_table[r];
+ if (r < ARRAY_SIZE(sense_detail_table) && sense_detail_table[r].key)
+ sd = &sense_detail_table[r];
else
- si = &sense_info_table[(__force int)
+ sd = &sense_detail_table[(__force int)
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
- key = si->key;
+ key = sd->key;
if (reason == TCM_CHECK_CONDITION_UNIT_ATTENTION) {
if (!core_scsi3_ua_for_check_condition(cmd, &key, &asc,
&ascq)) {
cmd->scsi_status = SAM_STAT_BUSY;
return;
}
- } else if (si->asc == 0) {
+ } else if (sd->asc == 0) {
WARN_ON_ONCE(cmd->scsi_asc == 0);
asc = cmd->scsi_asc;
ascq = cmd->scsi_ascq;
} else {
- asc = si->asc;
- ascq = si->ascq;
+ asc = sd->asc;
+ ascq = sd->ascq;
}
cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
scsi_build_sense_buffer(desc_format, buffer, key, asc, ascq);
- if (si->add_sector_info)
+ if (sd->add_sector_info)
WARN_ON_ONCE(scsi_set_sense_information(buffer,
cmd->scsi_sense_length,
cmd->bad_sector) < 0);
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 1/4] scsi: target: rename struct sense_info to sense_detail
@ 2020-10-30 21:39 ` David Disseldorp
0 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
This helps distinguish it from the SCSI sense INFORMATION field.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
---
drivers/target/target_core_transport.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index ff26ab0a5f60..caa3a7b34826 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3094,14 +3094,14 @@ bool transport_wait_for_tasks(struct se_cmd *cmd)
}
EXPORT_SYMBOL(transport_wait_for_tasks);
-struct sense_info {
+struct sense_detail {
u8 key;
u8 asc;
u8 ascq;
bool add_sector_info;
};
-static const struct sense_info sense_info_table[] = {
+static const struct sense_detail sense_detail_table[] = {
[TCM_NO_SENSE] = {
.key = NOT_READY
},
@@ -3261,39 +3261,39 @@ static const struct sense_info sense_info_table[] = {
*/
static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
{
- const struct sense_info *si;
+ const struct sense_detail *sd;
u8 *buffer = cmd->sense_buffer;
int r = (__force int)reason;
u8 key, asc, ascq;
bool desc_format = target_sense_desc_format(cmd->se_dev);
- if (r < ARRAY_SIZE(sense_info_table) && sense_info_table[r].key)
- si = &sense_info_table[r];
+ if (r < ARRAY_SIZE(sense_detail_table) && sense_detail_table[r].key)
+ sd = &sense_detail_table[r];
else
- si = &sense_info_table[(__force int)
+ sd = &sense_detail_table[(__force int)
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
- key = si->key;
+ key = sd->key;
if (reason = TCM_CHECK_CONDITION_UNIT_ATTENTION) {
if (!core_scsi3_ua_for_check_condition(cmd, &key, &asc,
&ascq)) {
cmd->scsi_status = SAM_STAT_BUSY;
return;
}
- } else if (si->asc = 0) {
+ } else if (sd->asc = 0) {
WARN_ON_ONCE(cmd->scsi_asc = 0);
asc = cmd->scsi_asc;
ascq = cmd->scsi_ascq;
} else {
- asc = si->asc;
- ascq = si->ascq;
+ asc = sd->asc;
+ ascq = sd->ascq;
}
cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
scsi_build_sense_buffer(desc_format, buffer, key, asc, ascq);
- if (si->add_sector_info)
+ if (sd->add_sector_info)
WARN_ON_ONCE(scsi_set_sense_information(buffer,
cmd->scsi_sense_length,
cmd->bad_sector) < 0);
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/4] scsi: target: rename cmd.bad_sector to cmd.sense_info
2020-10-30 21:39 ` David Disseldorp
@ 2020-10-30 21:39 ` David Disseldorp
-1 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
cmd.bad_sector currently gets packed into the sense INFORMATION field
for TCM_LOGICAL_BLOCK_{GUARD,APP_TAG,REF_TAG}_CHECK_FAILED errors, which
carry an .add_sector_info flag in the sense_detail_table to ensure this.
In preparation for propagating a byte offset on COMPARE AND WRITE
TCM_MISCOMPARE_VERIFY error, rename cmd.bad_sector to cmd.sense_info
and sense_detail.add_sector_info to sense_detail.add_sense_info so
that it better reflects the sense INFORMATION field destination.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
[ddiss: update previously overlooked ib_isert]
---
drivers/infiniband/ulp/isert/ib_isert.c | 4 ++--
drivers/target/target_core_sbc.c | 2 +-
drivers/target/target_core_transport.c | 12 ++++++------
include/target/target_core_base.h | 2 +-
4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 436e17f1d0e5..a3ba0c4d0d39 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -1528,12 +1528,12 @@ isert_check_pi_status(struct se_cmd *se_cmd, struct ib_mr *sig_mr)
}
sec_offset_err = mr_status.sig_err.sig_err_offset;
do_div(sec_offset_err, block_size);
- se_cmd->bad_sector = sec_offset_err + se_cmd->t_task_lba;
+ se_cmd->sense_info = sec_offset_err + se_cmd->t_task_lba;
isert_err("PI error found type %d at sector 0x%llx "
"expected 0x%x vs actual 0x%x\n",
mr_status.sig_err.err_type,
- (unsigned long long)se_cmd->bad_sector,
+ (unsigned long long)se_cmd->sense_info,
mr_status.sig_err.expected,
mr_status.sig_err.actual);
ret = 1;
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 6e8b8d30938f..5f77dd95f1b9 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1439,7 +1439,7 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
if (rc) {
kunmap_atomic(daddr - dsg->offset);
kunmap_atomic(paddr - psg->offset);
- cmd->bad_sector = sector;
+ cmd->sense_info = sector;
return rc;
}
next:
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index caa3a7b34826..c6f45c12d564 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3098,7 +3098,7 @@ struct sense_detail {
u8 key;
u8 asc;
u8 ascq;
- bool add_sector_info;
+ bool add_sense_info;
};
static const struct sense_detail sense_detail_table[] = {
@@ -3201,19 +3201,19 @@ static const struct sense_detail sense_detail_table[] = {
.key = ABORTED_COMMAND,
.asc = 0x10,
.ascq = 0x01, /* LOGICAL BLOCK GUARD CHECK FAILED */
- .add_sector_info = true,
+ .add_sense_info = true,
},
[TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED] = {
.key = ABORTED_COMMAND,
.asc = 0x10,
.ascq = 0x02, /* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */
- .add_sector_info = true,
+ .add_sense_info = true,
},
[TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED] = {
.key = ABORTED_COMMAND,
.asc = 0x10,
.ascq = 0x03, /* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */
- .add_sector_info = true,
+ .add_sense_info = true,
},
[TCM_COPY_TARGET_DEVICE_NOT_REACHABLE] = {
.key = COPY_ABORTED,
@@ -3293,10 +3293,10 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
scsi_build_sense_buffer(desc_format, buffer, key, asc, ascq);
- if (sd->add_sector_info)
+ if (sd->add_sense_info)
WARN_ON_ONCE(scsi_set_sense_information(buffer,
cmd->scsi_sense_length,
- cmd->bad_sector) < 0);
+ cmd->sense_info) < 0);
}
int
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 549947d407cf..7ee2bee46b3a 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -540,7 +540,7 @@ struct se_cmd {
struct scatterlist *t_prot_sg;
unsigned int t_prot_nents;
sense_reason_t pi_err;
- sector_t bad_sector;
+ u64 sense_info;
int cpuid;
};
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 2/4] scsi: target: rename cmd.bad_sector to cmd.sense_info
@ 2020-10-30 21:39 ` David Disseldorp
0 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
cmd.bad_sector currently gets packed into the sense INFORMATION field
for TCM_LOGICAL_BLOCK_{GUARD,APP_TAG,REF_TAG}_CHECK_FAILED errors, which
carry an .add_sector_info flag in the sense_detail_table to ensure this.
In preparation for propagating a byte offset on COMPARE AND WRITE
TCM_MISCOMPARE_VERIFY error, rename cmd.bad_sector to cmd.sense_info
and sense_detail.add_sector_info to sense_detail.add_sense_info so
that it better reflects the sense INFORMATION field destination.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
[ddiss: update previously overlooked ib_isert]
---
drivers/infiniband/ulp/isert/ib_isert.c | 4 ++--
drivers/target/target_core_sbc.c | 2 +-
drivers/target/target_core_transport.c | 12 ++++++------
include/target/target_core_base.h | 2 +-
4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 436e17f1d0e5..a3ba0c4d0d39 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -1528,12 +1528,12 @@ isert_check_pi_status(struct se_cmd *se_cmd, struct ib_mr *sig_mr)
}
sec_offset_err = mr_status.sig_err.sig_err_offset;
do_div(sec_offset_err, block_size);
- se_cmd->bad_sector = sec_offset_err + se_cmd->t_task_lba;
+ se_cmd->sense_info = sec_offset_err + se_cmd->t_task_lba;
isert_err("PI error found type %d at sector 0x%llx "
"expected 0x%x vs actual 0x%x\n",
mr_status.sig_err.err_type,
- (unsigned long long)se_cmd->bad_sector,
+ (unsigned long long)se_cmd->sense_info,
mr_status.sig_err.expected,
mr_status.sig_err.actual);
ret = 1;
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 6e8b8d30938f..5f77dd95f1b9 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1439,7 +1439,7 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
if (rc) {
kunmap_atomic(daddr - dsg->offset);
kunmap_atomic(paddr - psg->offset);
- cmd->bad_sector = sector;
+ cmd->sense_info = sector;
return rc;
}
next:
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index caa3a7b34826..c6f45c12d564 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3098,7 +3098,7 @@ struct sense_detail {
u8 key;
u8 asc;
u8 ascq;
- bool add_sector_info;
+ bool add_sense_info;
};
static const struct sense_detail sense_detail_table[] = {
@@ -3201,19 +3201,19 @@ static const struct sense_detail sense_detail_table[] = {
.key = ABORTED_COMMAND,
.asc = 0x10,
.ascq = 0x01, /* LOGICAL BLOCK GUARD CHECK FAILED */
- .add_sector_info = true,
+ .add_sense_info = true,
},
[TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED] = {
.key = ABORTED_COMMAND,
.asc = 0x10,
.ascq = 0x02, /* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */
- .add_sector_info = true,
+ .add_sense_info = true,
},
[TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED] = {
.key = ABORTED_COMMAND,
.asc = 0x10,
.ascq = 0x03, /* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */
- .add_sector_info = true,
+ .add_sense_info = true,
},
[TCM_COPY_TARGET_DEVICE_NOT_REACHABLE] = {
.key = COPY_ABORTED,
@@ -3293,10 +3293,10 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
scsi_build_sense_buffer(desc_format, buffer, key, asc, ascq);
- if (sd->add_sector_info)
+ if (sd->add_sense_info)
WARN_ON_ONCE(scsi_set_sense_information(buffer,
cmd->scsi_sense_length,
- cmd->bad_sector) < 0);
+ cmd->sense_info) < 0);
}
int
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 549947d407cf..7ee2bee46b3a 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -540,7 +540,7 @@ struct se_cmd {
struct scatterlist *t_prot_sg;
unsigned int t_prot_nents;
sense_reason_t pi_err;
- sector_t bad_sector;
+ u64 sense_info;
int cpuid;
};
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/4] scsi: target: split out COMPARE AND WRITE memcmp into helper
2020-10-30 21:39 ` David Disseldorp
@ 2020-10-30 21:39 ` David Disseldorp
-1 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
In preparation for finding and returning the miscompare offset.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
---
drivers/target/target_core_sbc.c | 121 +++++++++++++++++--------------
1 file changed, 68 insertions(+), 53 deletions(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 5f77dd95f1b9..22d0cbba6ff3 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -434,20 +434,75 @@ 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) {
+ 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 nlbas = cmd->t_task_nolb;
+ unsigned int len;
unsigned int block_size = dev->dev_attrib.block_size;
- unsigned int compare_len = (nlbas * block_size);
+ unsigned int compare_len = (cmd->t_task_nolb * block_size);
sense_reason_t ret = TCM_NO_SENSE;
- int rc, i;
+ int i;
/*
* Handle early failure in transport_generic_request_failure(),
@@ -473,12 +528,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,44 +542,9 @@ 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;
+ len = compare_len;
sg_miter_start(&m, cmd->t_data_sg, cmd->t_data_nents, SG_MITER_TO_SG);
/*
* Currently assumes NoLB=1 and SGLs are PAGE_SIZE..
@@ -568,13 +589,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 +598,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;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 3/4] scsi: target: split out COMPARE AND WRITE memcmp into helper
@ 2020-10-30 21:39 ` David Disseldorp
0 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
In preparation for finding and returning the miscompare offset.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
---
drivers/target/target_core_sbc.c | 121 +++++++++++++++++--------------
1 file changed, 68 insertions(+), 53 deletions(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 5f77dd95f1b9..22d0cbba6ff3 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -434,20 +434,75 @@ 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) {
+ 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 nlbas = cmd->t_task_nolb;
+ unsigned int len;
unsigned int block_size = dev->dev_attrib.block_size;
- unsigned int compare_len = (nlbas * block_size);
+ unsigned int compare_len = (cmd->t_task_nolb * block_size);
sense_reason_t ret = TCM_NO_SENSE;
- int rc, i;
+ int i;
/*
* Handle early failure in transport_generic_request_failure(),
@@ -473,12 +528,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,44 +542,9 @@ 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;
+ len = compare_len;
sg_miter_start(&m, cmd->t_data_sg, cmd->t_data_nents, SG_MITER_TO_SG);
/*
* Currently assumes NoLB=1 and SGLs are PAGE_SIZE..
@@ -568,13 +589,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 +598,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;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 4/4] scsi: target: return COMPARE AND WRITE miscompare offsets
2020-10-30 21:39 ` David Disseldorp
@ 2020-10-30 21:39 ` David Disseldorp
-1 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
SBC-4 r15 5.3 COMPARE AND WRITE command states:
if the compare operation does not indicate a match, then terminate the
command with CHECK CONDITION status with the sense key set to
MISCOMPARE and the additional sense code set to MISCOMPARE DURING
VERIFY OPERATION. In the sense data (see 4.18 and SPC-5) the offset
from the start of the Data-Out Buffer to the first byte of data that
was not equal shall be reported in the INFORMATION field.
This change implements the missing logic to report the miscompare offset
in the sense data INFORMATION field.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
---
drivers/target/target_core_sbc.c | 34 ++++++++++++++++++--------
drivers/target/target_core_transport.c | 1 +
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 22d0cbba6ff3..0fa52a98d6cf 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -435,13 +435,13 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
}
/*
- * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare return
- * TCM_MISCOMPARE_VERIFY.
+ * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare, fill
+ * @miscmp_off and 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 int cmp_len, unsigned int *miscmp_off)
{
unsigned char *buf = NULL;
struct scatterlist *sg;
@@ -467,17 +467,20 @@ compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
*/
offset = 0;
for_each_sg(read_sgl, sg, read_nents, i) {
+ unsigned int 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);
+ for (i = 0; i < len && addr[i] == buf[offset + i]; i++)
+ ;
+ kunmap_atomic(addr);
+ if (i < len) {
+ *miscmp_off = offset + i;
+ pr_warn("Detected MISCOMPARE at offset %u\n",
+ *miscmp_off);
ret = TCM_MISCOMPARE_VERIFY;
goto out;
}
- kunmap_atomic(addr);
offset += len;
cmp_len -= len;
@@ -501,6 +504,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
unsigned int len;
unsigned int block_size = dev->dev_attrib.block_size;
unsigned int compare_len = (cmd->t_task_nolb * block_size);
+ unsigned int miscmp_off = 0;
sense_reason_t ret = TCM_NO_SENSE;
int i;
@@ -532,8 +536,18 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
cmd->t_bidi_data_nents,
cmd->t_data_sg,
cmd->t_data_nents,
- compare_len);
- if (ret)
+ compare_len,
+ &miscmp_off);
+ if (ret == TCM_MISCOMPARE_VERIFY) {
+ /*
+ * SBC-4 r15: 5.3 COMPARE AND WRITE command
+ * In the sense data (see 4.18 and SPC-5) the offset from the
+ * start of the Data-Out Buffer to the first byte of data that
+ * was not equal shall be reported in the INFORMATION field.
+ */
+ cmd->sense_info = miscmp_off;
+ goto out;
+ } else if (ret)
goto out;
if (sg_alloc_table(&write_tbl, cmd->t_data_nents, GFP_KERNEL) < 0) {
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c6f45c12d564..693ed3fe4388 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3196,6 +3196,7 @@ static const struct sense_detail sense_detail_table[] = {
.key = MISCOMPARE,
.asc = 0x1d, /* MISCOMPARE DURING VERIFY OPERATION */
.ascq = 0x00,
+ .add_sense_info = true,
},
[TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED] = {
.key = ABORTED_COMMAND,
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 4/4] scsi: target: return COMPARE AND WRITE miscompare offsets
@ 2020-10-30 21:39 ` David Disseldorp
0 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-30 21:39 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi, David Disseldorp, Mike Christie
SBC-4 r15 5.3 COMPARE AND WRITE command states:
if the compare operation does not indicate a match, then terminate the
command with CHECK CONDITION status with the sense key set to
MISCOMPARE and the additional sense code set to MISCOMPARE DURING
VERIFY OPERATION. In the sense data (see 4.18 and SPC-5) the offset
from the start of the Data-Out Buffer to the first byte of data that
was not equal shall be reported in the INFORMATION field.
This change implements the missing logic to report the miscompare offset
in the sense data INFORMATION field.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <michael.christie@oracle.com>
---
drivers/target/target_core_sbc.c | 34 ++++++++++++++++++--------
drivers/target/target_core_transport.c | 1 +
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 22d0cbba6ff3..0fa52a98d6cf 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -435,13 +435,13 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
}
/*
- * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare return
- * TCM_MISCOMPARE_VERIFY.
+ * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare, fill
+ * @miscmp_off and 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 int cmp_len, unsigned int *miscmp_off)
{
unsigned char *buf = NULL;
struct scatterlist *sg;
@@ -467,17 +467,20 @@ compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
*/
offset = 0;
for_each_sg(read_sgl, sg, read_nents, i) {
+ unsigned int 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);
+ for (i = 0; i < len && addr[i] = buf[offset + i]; i++)
+ ;
+ kunmap_atomic(addr);
+ if (i < len) {
+ *miscmp_off = offset + i;
+ pr_warn("Detected MISCOMPARE at offset %u\n",
+ *miscmp_off);
ret = TCM_MISCOMPARE_VERIFY;
goto out;
}
- kunmap_atomic(addr);
offset += len;
cmp_len -= len;
@@ -501,6 +504,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
unsigned int len;
unsigned int block_size = dev->dev_attrib.block_size;
unsigned int compare_len = (cmd->t_task_nolb * block_size);
+ unsigned int miscmp_off = 0;
sense_reason_t ret = TCM_NO_SENSE;
int i;
@@ -532,8 +536,18 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
cmd->t_bidi_data_nents,
cmd->t_data_sg,
cmd->t_data_nents,
- compare_len);
- if (ret)
+ compare_len,
+ &miscmp_off);
+ if (ret = TCM_MISCOMPARE_VERIFY) {
+ /*
+ * SBC-4 r15: 5.3 COMPARE AND WRITE command
+ * In the sense data (see 4.18 and SPC-5) the offset from the
+ * start of the Data-Out Buffer to the first byte of data that
+ * was not equal shall be reported in the INFORMATION field.
+ */
+ cmd->sense_info = miscmp_off;
+ goto out;
+ } else if (ret)
goto out;
if (sg_alloc_table(&write_tbl, cmd->t_data_nents, GFP_KERNEL) < 0) {
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c6f45c12d564..693ed3fe4388 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3196,6 +3196,7 @@ static const struct sense_detail sense_detail_table[] = {
.key = MISCOMPARE,
.asc = 0x1d, /* MISCOMPARE DURING VERIFY OPERATION */
.ascq = 0x00,
+ .add_sense_info = true,
},
[TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED] = {
.key = ABORTED_COMMAND,
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/4] scsi: target: return COMPARE AND WRITE miscompare offsets
2020-10-30 21:39 ` David Disseldorp
@ 2020-10-31 0:06 ` Douglas Gilbert
-1 siblings, 0 replies; 14+ messages in thread
From: Douglas Gilbert @ 2020-10-31 0:06 UTC (permalink / raw)
To: David Disseldorp, target-devel; +Cc: linux-scsi, Mike Christie
On 2020-10-30 5:39 p.m., David Disseldorp wrote:
> SBC-4 r15 5.3 COMPARE AND WRITE command states:
> if the compare operation does not indicate a match, then terminate the
> command with CHECK CONDITION status with the sense key set to
> MISCOMPARE and the additional sense code set to MISCOMPARE DURING
> VERIFY OPERATION. In the sense data (see 4.18 and SPC-5) the offset
> from the start of the Data-Out Buffer to the first byte of data that
> was not equal shall be reported in the INFORMATION field.
>
> This change implements the missing logic to report the miscompare offset
> in the sense data INFORMATION field.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> Reviewed-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/target/target_core_sbc.c | 34 ++++++++++++++++++--------
> drivers/target/target_core_transport.c | 1 +
> 2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 22d0cbba6ff3..0fa52a98d6cf 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -435,13 +435,13 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
> }
>
> /*
> - * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare return
> - * TCM_MISCOMPARE_VERIFY.
> + * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare, fill
> + * @miscmp_off and 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 int cmp_len, unsigned int *miscmp_off)
> {
> unsigned char *buf = NULL;
> struct scatterlist *sg;
> @@ -467,17 +467,20 @@ compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
> */
> offset = 0;
> for_each_sg(read_sgl, sg, read_nents, i) {
> + unsigned int 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);
> + for (i = 0; i < len && addr[i] == buf[offset + i]; i++)
> + ;
I believe the logic is correct (and scsi_debug doesn't set the INFO field
in its CAW, but should), but I wonder about performance.
If the probability of equality is high (e.g. like it is usually with
VERIFY(BytChk=1) ) and memcmp() is faster than that for-loop (which
could be optimized), then a better strategy might be to always do memcmp()
first and only if it fails go into the byte by byte for-loop to find the
offset of the first miscompare.
IMO this should only be considered, if there is going to be a "v4" of
this patchset.
Doug Gilbert
> + kunmap_atomic(addr);
> + if (i < len) {
> + *miscmp_off = offset + i;
> + pr_warn("Detected MISCOMPARE at offset %u\n",
> + *miscmp_off);
> ret = TCM_MISCOMPARE_VERIFY;
> goto out;
> }
> - kunmap_atomic(addr);
>
> offset += len;
> cmp_len -= len;
> @@ -501,6 +504,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
> unsigned int len;
> unsigned int block_size = dev->dev_attrib.block_size;
> unsigned int compare_len = (cmd->t_task_nolb * block_size);
> + unsigned int miscmp_off = 0;
> sense_reason_t ret = TCM_NO_SENSE;
> int i;
>
> @@ -532,8 +536,18 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
> cmd->t_bidi_data_nents,
> cmd->t_data_sg,
> cmd->t_data_nents,
> - compare_len);
> - if (ret)
> + compare_len,
> + &miscmp_off);
> + if (ret == TCM_MISCOMPARE_VERIFY) {
> + /*
> + * SBC-4 r15: 5.3 COMPARE AND WRITE command
> + * In the sense data (see 4.18 and SPC-5) the offset from the
> + * start of the Data-Out Buffer to the first byte of data that
> + * was not equal shall be reported in the INFORMATION field.
> + */
> + cmd->sense_info = miscmp_off;
> + goto out;
> + } else if (ret)
> goto out;
>
> if (sg_alloc_table(&write_tbl, cmd->t_data_nents, GFP_KERNEL) < 0) {
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index c6f45c12d564..693ed3fe4388 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -3196,6 +3196,7 @@ static const struct sense_detail sense_detail_table[] = {
> .key = MISCOMPARE,
> .asc = 0x1d, /* MISCOMPARE DURING VERIFY OPERATION */
> .ascq = 0x00,
> + .add_sense_info = true,
> },
> [TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED] = {
> .key = ABORTED_COMMAND,
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/4] scsi: target: return COMPARE AND WRITE miscompare offsets
@ 2020-10-31 0:06 ` Douglas Gilbert
0 siblings, 0 replies; 14+ messages in thread
From: Douglas Gilbert @ 2020-10-31 0:06 UTC (permalink / raw)
To: David Disseldorp, target-devel; +Cc: linux-scsi, Mike Christie
On 2020-10-30 5:39 p.m., David Disseldorp wrote:
> SBC-4 r15 5.3 COMPARE AND WRITE command states:
> if the compare operation does not indicate a match, then terminate the
> command with CHECK CONDITION status with the sense key set to
> MISCOMPARE and the additional sense code set to MISCOMPARE DURING
> VERIFY OPERATION. In the sense data (see 4.18 and SPC-5) the offset
> from the start of the Data-Out Buffer to the first byte of data that
> was not equal shall be reported in the INFORMATION field.
>
> This change implements the missing logic to report the miscompare offset
> in the sense data INFORMATION field.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> Reviewed-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/target/target_core_sbc.c | 34 ++++++++++++++++++--------
> drivers/target/target_core_transport.c | 1 +
> 2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 22d0cbba6ff3..0fa52a98d6cf 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -435,13 +435,13 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
> }
>
> /*
> - * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare return
> - * TCM_MISCOMPARE_VERIFY.
> + * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare, fill
> + * @miscmp_off and 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 int cmp_len, unsigned int *miscmp_off)
> {
> unsigned char *buf = NULL;
> struct scatterlist *sg;
> @@ -467,17 +467,20 @@ compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
> */
> offset = 0;
> for_each_sg(read_sgl, sg, read_nents, i) {
> + unsigned int 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);
> + for (i = 0; i < len && addr[i] = buf[offset + i]; i++)
> + ;
I believe the logic is correct (and scsi_debug doesn't set the INFO field
in its CAW, but should), but I wonder about performance.
If the probability of equality is high (e.g. like it is usually with
VERIFY(BytChk=1) ) and memcmp() is faster than that for-loop (which
could be optimized), then a better strategy might be to always do memcmp()
first and only if it fails go into the byte by byte for-loop to find the
offset of the first miscompare.
IMO this should only be considered, if there is going to be a "v4" of
this patchset.
Doug Gilbert
> + kunmap_atomic(addr);
> + if (i < len) {
> + *miscmp_off = offset + i;
> + pr_warn("Detected MISCOMPARE at offset %u\n",
> + *miscmp_off);
> ret = TCM_MISCOMPARE_VERIFY;
> goto out;
> }
> - kunmap_atomic(addr);
>
> offset += len;
> cmp_len -= len;
> @@ -501,6 +504,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
> unsigned int len;
> unsigned int block_size = dev->dev_attrib.block_size;
> unsigned int compare_len = (cmd->t_task_nolb * block_size);
> + unsigned int miscmp_off = 0;
> sense_reason_t ret = TCM_NO_SENSE;
> int i;
>
> @@ -532,8 +536,18 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
> cmd->t_bidi_data_nents,
> cmd->t_data_sg,
> cmd->t_data_nents,
> - compare_len);
> - if (ret)
> + compare_len,
> + &miscmp_off);
> + if (ret = TCM_MISCOMPARE_VERIFY) {
> + /*
> + * SBC-4 r15: 5.3 COMPARE AND WRITE command
> + * In the sense data (see 4.18 and SPC-5) the offset from the
> + * start of the Data-Out Buffer to the first byte of data that
> + * was not equal shall be reported in the INFORMATION field.
> + */
> + cmd->sense_info = miscmp_off;
> + goto out;
> + } else if (ret)
> goto out;
>
> if (sg_alloc_table(&write_tbl, cmd->t_data_nents, GFP_KERNEL) < 0) {
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index c6f45c12d564..693ed3fe4388 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -3196,6 +3196,7 @@ static const struct sense_detail sense_detail_table[] = {
> .key = MISCOMPARE,
> .asc = 0x1d, /* MISCOMPARE DURING VERIFY OPERATION */
> .ascq = 0x00,
> + .add_sense_info = true,
> },
> [TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED] = {
> .key = ABORTED_COMMAND,
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/4] scsi: target: return COMPARE AND WRITE miscompare offsets
2020-10-31 0:06 ` Douglas Gilbert
@ 2020-10-31 10:38 ` David Disseldorp
-1 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-31 10:38 UTC (permalink / raw)
To: Douglas Gilbert
Cc: target-devel, linux-scsi, Mike Christie, Dirk Hohndel (VMware)
On Fri, 30 Oct 2020 20:06:31 -0400, Douglas Gilbert wrote:
> I believe the logic is correct (and scsi_debug doesn't set the INFO field
> in its CAW, but should), but I wonder about performance.
>
> If the probability of equality is high (e.g. like it is usually with
> VERIFY(BytChk=1) ) and memcmp() is faster than that for-loop (which
> could be optimized), then a better strategy might be to always do memcmp()
> first and only if it fails go into the byte by byte for-loop to find the
> offset of the first miscompare.
While adding the INFO return to tgt I noticed that it had the same
memcmp-with-miscompare-for-loop logic that you describe. I'm only aware
of ESXi as a COMPARE AND WRITE consumer and I think probability of
equality is quite high (@Dirk: perhaps you can confirm?).
> IMO this should only be considered, if there is going to be a "v4" of
> this patchset.
I think this optimization wouldn't make the code much more complex, so
I'll do a re-spin with this change. Thanks for the feedback.
Cheers, David
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] scsi: target: return COMPARE AND WRITE miscompare offsets
@ 2020-10-31 10:38 ` David Disseldorp
0 siblings, 0 replies; 14+ messages in thread
From: David Disseldorp @ 2020-10-31 10:38 UTC (permalink / raw)
To: Douglas Gilbert
Cc: target-devel, linux-scsi, Mike Christie, Dirk Hohndel (VMware)
On Fri, 30 Oct 2020 20:06:31 -0400, Douglas Gilbert wrote:
> I believe the logic is correct (and scsi_debug doesn't set the INFO field
> in its CAW, but should), but I wonder about performance.
>
> If the probability of equality is high (e.g. like it is usually with
> VERIFY(BytChk=1) ) and memcmp() is faster than that for-loop (which
> could be optimized), then a better strategy might be to always do memcmp()
> first and only if it fails go into the byte by byte for-loop to find the
> offset of the first miscompare.
While adding the INFO return to tgt I noticed that it had the same
memcmp-with-miscompare-for-loop logic that you describe. I'm only aware
of ESXi as a COMPARE AND WRITE consumer and I think probability of
equality is quite high (@Dirk: perhaps you can confirm?).
> IMO this should only be considered, if there is going to be a "v4" of
> this patchset.
I think this optimization wouldn't make the code much more complex, so
I'll do a re-spin with this change. Thanks for the feedback.
Cheers, David
^ permalink raw reply [flat|nested] 14+ messages in thread