On 08/08/2018 02:31 PM, Greg Edwards wrote: > When T10 PI is enabled on a backing device for the iblock backstore, the > PI SGL for the entire command is attached to the first bio only. This > works fine if the command is covered by a single bio, but results in > integrity verification errors for the other bios in a multi-bio command. >
Did you hit this with a older distro kernel? It looks like iblock_get_bio will alloc a bio that has enough vecs for the entire cmd (bi_max_vecs will equal sgl_nents). So it is not clear to me how does the bio_add_page call ever return a value other than sg->length, and we end up doing another iblock_get_bio call? > Split the PI SGL across the bios in the command, so each bip contains > the protection information for the data in the bio. In a multi-bio > command, store where we left off in the PI SGL so we know where to start > with the next bio. > > Signed-off-by: Greg Edwards <[email protected]> > --- > This patch depends on commit 359f642700f2 ("block: move > bio_integrity_{intervals,bytes} into blkdev.h") in Jens' block for-next > branch. > > drivers/target/target_core_iblock.c | 60 +++++++++++++++++++++-------- > 1 file changed, 44 insertions(+), 16 deletions(-) > > diff --git a/drivers/target/target_core_iblock.c > b/drivers/target/target_core_iblock.c > index ce1321a5cb7b..d3ab83282f61 100644 > --- a/drivers/target/target_core_iblock.c > +++ b/drivers/target/target_core_iblock.c > @@ -635,7 +635,8 @@ static ssize_t iblock_show_configfs_dev_params(struct > se_device *dev, char *b) > } > > static int > -iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio) > +iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio, > + struct scatterlist **sgl, unsigned int *skip) > { > struct se_device *dev = cmd->se_dev; > struct blk_integrity *bi; > @@ -643,6 +644,7 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio) > struct iblock_dev *ib_dev = IBLOCK_DEV(dev); > struct scatterlist *sg; > int i, rc; > + unsigned int size, nr_pages, len, off; > > bi = bdev_get_integrity(ib_dev->ibd_bd); > if (!bi) { > @@ -650,32 +652,52 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio) > return -ENODEV; > } > > - bip = bio_integrity_alloc(bio, GFP_NOIO, cmd->t_prot_nents); > + nr_pages = min_t(unsigned int, cmd->t_prot_nents, BIO_MAX_PAGES); > + bip = bio_integrity_alloc(bio, GFP_NOIO, nr_pages); > if (IS_ERR(bip)) { > pr_err("Unable to allocate bio_integrity_payload\n"); > return PTR_ERR(bip); > } > > - bip->bip_iter.bi_size = (cmd->data_length / dev->dev_attrib.block_size) > * > - dev->prot_length; > - bip->bip_iter.bi_sector = bio->bi_iter.bi_sector; > + bip->bip_iter.bi_size = bio_integrity_bytes(bi, bio_sectors(bio)); > + bip_set_seed(bip, bio->bi_iter.bi_sector); > > pr_debug("IBLOCK BIP Size: %u Sector: %llu\n", bip->bip_iter.bi_size, > (unsigned long long)bip->bip_iter.bi_sector); > > - for_each_sg(cmd->t_prot_sg, sg, cmd->t_prot_nents, i) { > + size = bip->bip_iter.bi_size; > + for_each_sg(*sgl, sg, nr_pages, i) { > > - rc = bio_integrity_add_page(bio, sg_page(sg), sg->length, > - sg->offset); > - if (rc != sg->length) { > + len = sg->length - *skip; > + off = sg->offset + *skip; > + > + if (*skip) > + *skip = 0; > + > + if (len > size) { > + len = size; > + *skip = len; > + } > + > + rc = bio_integrity_add_page(bio, sg_page(sg), len, off); > + if (rc != len) { > pr_err("bio_integrity_add_page() failed; %d\n", rc); > return -ENOMEM; > } > > pr_debug("Added bio integrity page: %p length: %d offset; %d\n", > - sg_page(sg), sg->length, sg->offset); > + sg_page(sg), len, off); > + > + size -= len; > + if (!size) > + break; > } > > + if (*skip == 0) > + *sgl = sg_next(sg); > + else > + *sgl = sg; > + > return 0; > } > > @@ -686,12 +708,12 @@ iblock_execute_rw(struct se_cmd *cmd, struct > scatterlist *sgl, u32 sgl_nents, > struct se_device *dev = cmd->se_dev; > sector_t block_lba = target_to_linux_sector(dev, cmd->t_task_lba); > struct iblock_req *ibr; > - struct bio *bio, *bio_start; > + struct bio *bio; > struct bio_list list; > - struct scatterlist *sg; > + struct scatterlist *sg, *sg_prot = cmd->t_prot_sg; > u32 sg_num = sgl_nents; > - unsigned bio_cnt; > - int i, op, op_flags = 0; > + unsigned int bio_cnt, skip = 0; > + int i, rc, op, op_flags = 0; > > if (data_direction == DMA_TO_DEVICE) { > struct iblock_dev *ib_dev = IBLOCK_DEV(dev); > @@ -726,7 +748,6 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist > *sgl, u32 sgl_nents, > if (!bio) > goto fail_free_ibr; > > - bio_start = bio; > bio_list_init(&list); > bio_list_add(&list, bio); > > @@ -741,6 +762,13 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist > *sgl, u32 sgl_nents, > */ > while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset) > != sg->length) { > + if (cmd->prot_type && dev->dev_attrib.pi_prot_type) { > + rc = iblock_alloc_bip(cmd, bio, &sg_prot, > + &skip); > + if (rc) > + goto fail_put_bios; > + } > + > if (bio_cnt >= IBLOCK_MAX_BIO_PER_TASK) { > iblock_submit_bios(&list); > bio_cnt = 0; > @@ -762,7 +790,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist > *sgl, u32 sgl_nents, > } > > if (cmd->prot_type && dev->dev_attrib.pi_prot_type) { > - int rc = iblock_alloc_bip(cmd, bio_start); > + rc = iblock_alloc_bip(cmd, bio, &sg_prot, &skip); > if (rc) > goto fail_put_bios; > } >
