On 3/5/2014 4:52 AM, Nicholas A. Bellinger wrote:
On Sun, 2014-03-02 at 19:51 +0200, Sagi Grimberg wrote:
When copying between device and command protection scatters
we must take into account that device scatters might be offset
and we must not copy outside scatter range. Thus for each cmd prot
scatter we must take the min between cmd prot scatter, dev prot
scatter, and what's left (and loop in case we havn't copied enough
from/to cmd prot scatter).
This issue was found using iSER T10-PI offload over rd_mcp (wasn't
discovered with fileio since file_dev prot sglists are never offset).
Debug example (single t_prot_sg of len 2048):
kernel: sbc_dif_copy_prot: se_cmd=ffff880380aaf970, left=2048, sg_off=3072, sg->length=4096, len=2048
kernel: isert: se_cmd=ffff880380aaf970 PI error found type 0 at sector 0x2600 expected 0x0 vs actual 0x725f, lba=0x2580
Instead of copying 2048 bytes from offset 3072 (copying junk
outside sg limit 4096), we must copy 1024 bytes and continue to
next sg until we complete cmd prot scatter (1024 from next sg
in this case).
Same example:
kernel: sbc_dif_copy_prot: se_cmd=ffff8803ea6aba28, left=2048, offset=3072, sg->length=4096, len=1024
kernel: sg = sg_next(sg)
kernel: sbc_dif_copy_prot: se_cmd=ffff8803ea6aba28, left=1024, offset=0, sg->length=4096, len=1024
Signed-off-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
---
A few comments below..
drivers/target/target_core_sbc.c | 37 +++++++++++++++++++++----------------
1 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 0d1476a..ab006d2 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1137,25 +1137,30 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
left = sectors * dev->prot_length;
for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
-
- len = min(psg->length, left);
- if (offset >= sg->length) {
- sg = sg_next(sg);
- offset = 0;
- }
+ unsigned int psg_len, copied = 0;
paddr = kmap_atomic(sg_page(psg)) + psg->offset;
- addr = kmap_atomic(sg_page(sg)) + sg->offset + offset;
-
- if (read)
- memcpy(paddr, addr, len);
- else
- memcpy(addr, paddr, len);
-
- left -= len;
- offset += len;
+ psg_len = min(left, psg->length - psg->offset);
So it's wrong to consider psg->offset in the length calculation here..
The psg->length should already be taking into account any non zero
psg->offset, without having to do a recalculation.
So, it's perfectly fine to expect:
sg->length = 1024
sg->offset = 3072
which can result in a bogus value with the current patch.
Eg, following SGL would be considered bogus:
sg->length = 4096
sg->offset = 3072
Your probably right, I tested this patch with randread on a non-written
backstore (escape values),
so I probably missed that. I'll fix.
+ while (copied < psg_len) {
+ len = min(psg_len, sg->length - offset);
Tested the fix, I stepped on another bug here: len should consider how
much was copied already.
Should be:
len = min(psg_len, sg->length - offset) - copied;
+ addr = kmap_atomic(sg_page(sg)) + sg->offset + offset;
+
+ if (read)
+ memcpy(paddr, addr, len);
+ else
+ memcpy(addr, paddr, len);
+
AFAICT paddr's location is incorrect on subsequent loops here, as it's
not taking the already copied byte offset into account..
I think this should be:
if (read)
memcpy(paddr + copied, addr, len);
else
memcpy(addr, paddr + copied, len);
Same here, I'll fix.
+ left -= len;
+ offset += len;
+ copied += len;
+
+ if (offset >= sg->length - sg->offset) {
Same issue here wrt to sg->offset as above.
OK.
--nab
tested with several hours of fio "randrw". seems to work now.
More on this,
To tell you the truth I don't see a good reason to put this routine
here, It serves RD and fileIO DIF emulation,
but I think it is better to put this copy specific to rd backstore as
the protection information is stored as a SG-list.
For FileIO I don't see why we need a tmp SG-list to bounce between the
prot_file to cmd->t_prot_sg.
Thoughts?
+ sg = sg_next(sg);
+ offset = 0;
+ }
+ kunmap_atomic(addr);
+ }
kunmap_atomic(paddr);
- kunmap_atomic(addr);
}
}
--
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
--
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
