Re: [PATCH v3 3/5] target: handle odd SG mapping for data transfer memory

Sagi Grimberg
2015-04-26T10:07:35Z

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