| Message ID | 1429972410-7146-4-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/25/2015 5:33 PM, Akinobu Mita wrote: > sbc_dif_generate() and sbc_dif_verify() currently assume that each > SG element for data transfer memory doesn't straddle the block size > boundary. > > However, when using SG_IO ioctl, we can choose the data transfer > memory which doesn't satisfy that alignment requirement. > > In order to handle such cases correctly, this change inverts the outer > loop to iterate data transfer memory and the inner loop to iterate > protection information and enables to calculate CRC for a block which > straddles multiple SG elements. > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > Cc: Tim Chen <tim.c.chen@linux.intel.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: linux-crypto@vger.kernel.org > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > Cc: Sagi Grimberg <sagig@mellanox.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> > Cc: target-devel@vger.kernel.org > Cc: linux-scsi@vger.kernel.org > --- > * Changes from v2: > - Handle odd SG mapping correctly instead of giving up > > drivers/target/target_core_sbc.c | 108 +++++++++++++++++++++++++-------------- > 1 file changed, 69 insertions(+), 39 deletions(-) > > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c > index edba39f..33d2426 100644 > --- a/drivers/target/target_core_sbc.c > +++ b/drivers/target/target_core_sbc.c > @@ -1182,27 +1182,43 @@ sbc_dif_generate(struct se_cmd *cmd) > { > struct se_device *dev = cmd->se_dev; > struct se_dif_v1_tuple *sdt; > - struct scatterlist *dsg, *psg = cmd->t_prot_sg; > + struct scatterlist *dsg = cmd->t_data_sg, *psg; > sector_t sector = cmd->t_task_lba; > void *daddr, *paddr; > int i, j, offset = 0; > + unsigned int block_size = dev->dev_attrib.block_size; > > - for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) { > - daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > + for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) { > paddr = kmap_atomic(sg_page(psg)) + psg->offset; > + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > > - for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) { > + for (j = 0; j < psg->length; > + j += sizeof(struct se_dif_v1_tuple)) { > + __u16 crc = 0; > + unsigned int avail; > > - if (offset >= psg->length) { > - kunmap_atomic(paddr); > - psg = sg_next(psg); > - paddr = kmap_atomic(sg_page(psg)) + psg->offset; > - offset = 0; > + if (offset >= dsg->length) { > + offset -= dsg->length; > + kunmap_atomic(daddr); This unmap is inconsistent. You need to unmap (daddr - dsg->offset). This applies throughout the patch. > + dsg = sg_next(dsg); > + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > } > > - sdt = paddr + offset; > - sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j, > - dev->dev_attrib.block_size)); > + sdt = paddr + j; > + > + avail = min(block_size, dsg->length - offset); > + crc = crc_t10dif(daddr + offset, avail); > + if (avail < block_size) { > + kunmap_atomic(daddr); > + dsg = sg_next(dsg); > + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > + offset = block_size - avail; > + crc = crc_t10dif_update(crc, daddr, offset); > + } else { > + offset += block_size; > + } > + > + sdt->guard_tag = cpu_to_be16(crc); > if (cmd->prot_type == TARGET_DIF_TYPE1_PROT) > sdt->ref_tag = cpu_to_be32(sector & 0xffffffff); > sdt->app_tag = 0; > @@ -1215,26 +1231,23 @@ sbc_dif_generate(struct se_cmd *cmd) > be32_to_cpu(sdt->ref_tag)); > > sector++; > - offset += sizeof(struct se_dif_v1_tuple); > } > > - kunmap_atomic(paddr); > kunmap_atomic(daddr); > + kunmap_atomic(paddr); > } > } > > static sense_reason_t > sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt, > - const void *p, sector_t sector, unsigned int ei_lba) > + __u16 crc, sector_t sector, unsigned int ei_lba) > { > - struct se_device *dev = cmd->se_dev; > - int block_size = dev->dev_attrib.block_size; > __be16 csum; > > if (!(cmd->prot_checks & TARGET_DIF_CHECK_GUARD)) > goto check_ref; > > - csum = cpu_to_be16(crc_t10dif(p, block_size)); > + csum = cpu_to_be16(crc); > > if (sdt->guard_tag != csum) { > pr_err("DIFv1 checksum failed on sector %llu guard tag 0x%04x" > @@ -1316,26 +1329,32 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, > { > struct se_device *dev = cmd->se_dev; > struct se_dif_v1_tuple *sdt; > - struct scatterlist *dsg; > + struct scatterlist *dsg = cmd->t_data_sg; > sector_t sector = start; > void *daddr, *paddr; > - int i, j; > + int i; > sense_reason_t rc; > + int dsg_off = 0; > + unsigned int block_size = dev->dev_attrib.block_size; > > - for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) { > - daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > + for (; psg && sector < start + sectors; psg = sg_next(psg)) { > paddr = kmap_atomic(sg_page(psg)) + psg->offset; > + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > > - for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) { > - > - if (psg_off >= psg->length) { > - kunmap_atomic(paddr - psg->offset); > - psg = sg_next(psg); > - paddr = kmap_atomic(sg_page(psg)) + psg->offset; > - psg_off = 0; > + for (i = psg_off; i < psg->length && > + sector < start + sectors; > + i += sizeof(struct se_dif_v1_tuple)) { > + __u16 crc; > + unsigned int avail; > + > + if (dsg_off >= dsg->length) { > + dsg_off -= dsg->length; > + kunmap_atomic(daddr); > + dsg = sg_next(dsg); > + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > } > > - sdt = paddr + psg_off; > + sdt = paddr + i; > > pr_debug("DIF READ sector: %llu guard_tag: 0x%04x" > " app_tag: 0x%04x ref_tag: %u\n", > @@ -1343,27 +1362,38 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, > sdt->app_tag, be32_to_cpu(sdt->ref_tag)); > > if (sdt->app_tag == cpu_to_be16(0xffff)) { > - sector++; > - psg_off += sizeof(struct se_dif_v1_tuple); > - continue; > + dsg_off += block_size; > + goto next; > + } > + > + avail = min(block_size, dsg->length - dsg_off); > + > + crc = crc_t10dif(daddr + dsg_off, avail); > + if (avail < block_size) { > + kunmap_atomic(daddr); > + dsg = sg_next(dsg); > + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; > + dsg_off = block_size - avail; > + crc = crc_t10dif_update(crc, daddr, dsg_off); > + } else { > + dsg_off += block_size; > } > > - rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector, > - ei_lba); > + rc = sbc_dif_v1_verify(cmd, sdt, crc, sector, ei_lba); > if (rc) { > - kunmap_atomic(paddr - psg->offset); > kunmap_atomic(daddr - dsg->offset); > + kunmap_atomic(paddr - psg->offset); > cmd->bad_sector = sector; > return rc; > } > - > +next: > sector++; > ei_lba++; > - psg_off += sizeof(struct se_dif_v1_tuple); > } > > - kunmap_atomic(paddr - psg->offset); > + psg_off = 0; > kunmap_atomic(daddr - dsg->offset); > + kunmap_atomic(paddr - psg->offset); > } > > return 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-04-26 19:07 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>: > On 4/25/2015 5:33 PM, Akinobu Mita wrote: >> >> sbc_dif_generate() and sbc_dif_verify() currently assume that each >> SG element for data transfer memory doesn't straddle the block size >> boundary. >> >> However, when using SG_IO ioctl, we can choose the data transfer >> memory which doesn't satisfy that alignment requirement. >> >> In order to handle such cases correctly, this change inverts the outer >> loop to iterate data transfer memory and the inner loop to iterate >> protection information and enables to calculate CRC for a block which >> straddles multiple SG elements. >> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> >> Cc: Tim Chen <tim.c.chen@linux.intel.com> >> Cc: Herbert Xu <herbert@gondor.apana.org.au> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: linux-crypto@vger.kernel.org >> Cc: Nicholas Bellinger <nab@linux-iscsi.org> >> Cc: Sagi Grimberg <sagig@mellanox.com> >> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> >> Cc: target-devel@vger.kernel.org >> Cc: linux-scsi@vger.kernel.org >> --- >> * Changes from v2: >> - Handle odd SG mapping correctly instead of giving up >> >> drivers/target/target_core_sbc.c | 108 >> +++++++++++++++++++++++++-------------- >> 1 file changed, 69 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/target/target_core_sbc.c >> b/drivers/target/target_core_sbc.c >> index edba39f..33d2426 100644 >> --- a/drivers/target/target_core_sbc.c >> +++ b/drivers/target/target_core_sbc.c >> @@ -1182,27 +1182,43 @@ sbc_dif_generate(struct se_cmd *cmd) >> { >> struct se_device *dev = cmd->se_dev; >> struct se_dif_v1_tuple *sdt; >> - struct scatterlist *dsg, *psg = cmd->t_prot_sg; >> + struct scatterlist *dsg = cmd->t_data_sg, *psg; >> sector_t sector = cmd->t_task_lba; >> void *daddr, *paddr; >> int i, j, offset = 0; >> + unsigned int block_size = dev->dev_attrib.block_size; >> >> - for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) { >> - daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; >> + for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) { >> paddr = kmap_atomic(sg_page(psg)) + psg->offset; >> + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; >> >> - for (j = 0; j < dsg->length; j += >> dev->dev_attrib.block_size) { >> + for (j = 0; j < psg->length; >> + j += sizeof(struct se_dif_v1_tuple)) { >> + __u16 crc = 0; >> + unsigned int avail; >> >> - if (offset >= psg->length) { >> - kunmap_atomic(paddr); >> - psg = sg_next(psg); >> - paddr = kmap_atomic(sg_page(psg)) + >> psg->offset; >> - offset = 0; >> + if (offset >= dsg->length) { >> + offset -= dsg->length; >> + kunmap_atomic(daddr); > > > This unmap is inconsistent. You need to unmap (daddr - dsg->offset). > > This applies throughout the patch. Thanks for pointing out. I'll fix them all. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index edba39f..33d2426 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1182,27 +1182,43 @@ sbc_dif_generate(struct se_cmd *cmd) { struct se_device *dev = cmd->se_dev; struct se_dif_v1_tuple *sdt; - struct scatterlist *dsg, *psg = cmd->t_prot_sg; + struct scatterlist *dsg = cmd->t_data_sg, *psg; sector_t sector = cmd->t_task_lba; void *daddr, *paddr; int i, j, offset = 0; + unsigned int block_size = dev->dev_attrib.block_size; - for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) { - daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; + for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) { paddr = kmap_atomic(sg_page(psg)) + psg->offset; + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; - for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) { + for (j = 0; j < psg->length; + j += sizeof(struct se_dif_v1_tuple)) { + __u16 crc = 0; + unsigned int avail; - if (offset >= psg->length) { - kunmap_atomic(paddr); - psg = sg_next(psg); - paddr = kmap_atomic(sg_page(psg)) + psg->offset; - offset = 0; + if (offset >= dsg->length) { + offset -= dsg->length; + kunmap_atomic(daddr); + dsg = sg_next(dsg); + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; } - sdt = paddr + offset; - sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j, - dev->dev_attrib.block_size)); + sdt = paddr + j; + + avail = min(block_size, dsg->length - offset); + crc = crc_t10dif(daddr + offset, avail); + if (avail < block_size) { + kunmap_atomic(daddr); + dsg = sg_next(dsg); + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; + offset = block_size - avail; + crc = crc_t10dif_update(crc, daddr, offset); + } else { + offset += block_size; + } + + sdt->guard_tag = cpu_to_be16(crc); if (cmd->prot_type == TARGET_DIF_TYPE1_PROT) sdt->ref_tag = cpu_to_be32(sector & 0xffffffff); sdt->app_tag = 0; @@ -1215,26 +1231,23 @@ sbc_dif_generate(struct se_cmd *cmd) be32_to_cpu(sdt->ref_tag)); sector++; - offset += sizeof(struct se_dif_v1_tuple); } - kunmap_atomic(paddr); kunmap_atomic(daddr); + kunmap_atomic(paddr); } } static sense_reason_t sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt, - const void *p, sector_t sector, unsigned int ei_lba) + __u16 crc, sector_t sector, unsigned int ei_lba) { - struct se_device *dev = cmd->se_dev; - int block_size = dev->dev_attrib.block_size; __be16 csum; if (!(cmd->prot_checks & TARGET_DIF_CHECK_GUARD)) goto check_ref; - csum = cpu_to_be16(crc_t10dif(p, block_size)); + csum = cpu_to_be16(crc); if (sdt->guard_tag != csum) { pr_err("DIFv1 checksum failed on sector %llu guard tag 0x%04x" @@ -1316,26 +1329,32 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, { struct se_device *dev = cmd->se_dev; struct se_dif_v1_tuple *sdt; - struct scatterlist *dsg; + struct scatterlist *dsg = cmd->t_data_sg; sector_t sector = start; void *daddr, *paddr; - int i, j; + int i; sense_reason_t rc; + int dsg_off = 0; + unsigned int block_size = dev->dev_attrib.block_size; - for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) { - daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; + for (; psg && sector < start + sectors; psg = sg_next(psg)) { paddr = kmap_atomic(sg_page(psg)) + psg->offset; + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; - for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) { - - if (psg_off >= psg->length) { - kunmap_atomic(paddr - psg->offset); - psg = sg_next(psg); - paddr = kmap_atomic(sg_page(psg)) + psg->offset; - psg_off = 0; + for (i = psg_off; i < psg->length && + sector < start + sectors; + i += sizeof(struct se_dif_v1_tuple)) { + __u16 crc; + unsigned int avail; + + if (dsg_off >= dsg->length) { + dsg_off -= dsg->length; + kunmap_atomic(daddr); + dsg = sg_next(dsg); + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; } - sdt = paddr + psg_off; + sdt = paddr + i; pr_debug("DIF READ sector: %llu guard_tag: 0x%04x" " app_tag: 0x%04x ref_tag: %u\n", @@ -1343,27 +1362,38 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, sdt->app_tag, be32_to_cpu(sdt->ref_tag)); if (sdt->app_tag == cpu_to_be16(0xffff)) { - sector++; - psg_off += sizeof(struct se_dif_v1_tuple); - continue; + dsg_off += block_size; + goto next; + } + + avail = min(block_size, dsg->length - dsg_off); + + crc = crc_t10dif(daddr + dsg_off, avail); + if (avail < block_size) { + kunmap_atomic(daddr); + dsg = sg_next(dsg); + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; + dsg_off = block_size - avail; + crc = crc_t10dif_update(crc, daddr, dsg_off); + } else { + dsg_off += block_size; } - rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector, - ei_lba); + rc = sbc_dif_v1_verify(cmd, sdt, crc, sector, ei_lba); if (rc) { - kunmap_atomic(paddr - psg->offset); kunmap_atomic(daddr - dsg->offset); + kunmap_atomic(paddr - psg->offset); cmd->bad_sector = sector; return rc; } - +next: sector++; ei_lba++; - psg_off += sizeof(struct se_dif_v1_tuple); } - kunmap_atomic(paddr - psg->offset); + psg_off = 0; kunmap_atomic(daddr - dsg->offset); + kunmap_atomic(paddr - psg->offset); } return 0;
sbc_dif_generate() and sbc_dif_verify() currently assume that each SG element for data transfer memory doesn't straddle the block size boundary. However, when using SG_IO ioctl, we can choose the data transfer memory which doesn't satisfy that alignment requirement. In order to handle such cases correctly, this change inverts the outer loop to iterate data transfer memory and the inner loop to iterate protection information and enables to calculate CRC for a block which straddles multiple SG elements. Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> Cc: Tim Chen <tim.c.chen@linux.intel.com> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: "David S. Miller" <davem@davemloft.net> Cc: linux-crypto@vger.kernel.org Cc: Nicholas Bellinger <nab@linux-iscsi.org> Cc: Sagi Grimberg <sagig@mellanox.com> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: Christoph Hellwig <hch@lst.de> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> Cc: target-devel@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes from v2: - Handle odd SG mapping correctly instead of giving up drivers/target/target_core_sbc.c | 108 +++++++++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 39 deletions(-)