On 08/30/2018 12:25 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 can result in
> ref tag errors in the client for the other bios in a multi-bio command,
> e.g.
>
> [ 47.631236] sda: ref tag error at location 2048 (rcvd 0)
> [ 47.637658] sda: ref tag error at location 4096 (rcvd 0)
> [ 47.644228] sda: ref tag error at location 6144 (rcvd 0)
>
> The command will be split into multiple bios if the number of data SG
> elements exceeds BIO_MAX_PAGES (see iblock_get_bio()).
>
> The bios may later be split again in the block layer on the host after
> iblock_submit_bios(), depending on the queue limits of the backing
> device. The block and SCSI layers will pass through the whole PI SGL
> down to the LLDD however that first bio is split up, but the LLDD may
> only use the portion that corresponds to the data length (depends on the
> LLDD, tested with scsi_debug).
>
> Split the PI SGL across the bios in the command, so each bio's
> bio_integrity_payload contains the protection information for the data
> in the bio. Use an sg_mapping_iter to keep track of where we are in PI
> SGL, so we know where to start with the next bio.
>
> Signed-off-by: Greg Edwards <gedwards@xxxxxxx>
> ---
> Changes from v2:
> * add back min(cmd->t_prot_nents, BIO_MAX_PAGES) for bio_integrity_alloc()
> from v1
>
> Changes from v1:
> * expand commit message
> * use an sg_mapping_iter to track where we are in the PI SGL
>
>
> drivers/target/target_core_iblock.c | 54 ++++++++++++++++++++---------
> 1 file changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index ce1321a5cb7b..db78b1c808e8 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -635,14 +635,15 @@ 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 sg_mapping_iter *miter)
> {
> struct se_device *dev = cmd->se_dev;
> struct blk_integrity *bi;
> struct bio_integrity_payload *bip;
> struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
> - struct scatterlist *sg;
> - int i, rc;
> + int rc;
> + size_t resid, len;
>
> bi = bdev_get_integrity(ib_dev->ibd_bd);
> if (!bi) {
> @@ -650,31 +651,39 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio)
> return -ENODEV;
> }
>
> - bip = bio_integrity_alloc(bio, GFP_NOIO, cmd->t_prot_nents);
> + bip = bio_integrity_alloc(bio, GFP_NOIO,
> + min_t(unsigned int, cmd->t_prot_nents, BIO_MAX_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) {
> + resid = bip->bip_iter.bi_size;
> + while (resid > 0 && sg_miter_next(miter)) {
>
> - rc = bio_integrity_add_page(bio, sg_page(sg), sg->length,
> - sg->offset);
> - if (rc != sg->length) {
> + len = min_t(size_t, miter->length, resid);
> + rc = bio_integrity_add_page(bio, miter->page, len,
> + offset_in_page(miter->addr));
> + if (rc != len) {
> pr_err("bio_integrity_add_page() failed; %d\n", rc);
> + sg_miter_stop(miter);
> return -ENOMEM;
> }
>
> - pr_debug("Added bio integrity page: %p length: %d offset; %d\n",
> - sg_page(sg), sg->length, sg->offset);
> + pr_debug("Added bio integrity page: %p length: %lu offset: %lu\n",
> + miter->page, len, offset_in_page(miter->addr));
> +
> + resid -= len;
> + if (len < miter->length)
> + miter->consumed -= miter->length - len;
> }
> + sg_miter_stop(miter);
>
> return 0;
> }
> @@ -686,12 +695,13 @@ 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;
> u32 sg_num = sgl_nents;
> unsigned bio_cnt;
> - int i, op, op_flags = 0;
> + int i, rc, op, op_flags = 0;
> + struct sg_mapping_iter prot_miter;
>
> if (data_direction == DMA_TO_DEVICE) {
> struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
> @@ -726,13 +736,17 @@ 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);
>
> refcount_set(&ibr->pending, 2);
> bio_cnt = 1;
>
> + if (cmd->prot_type && dev->dev_attrib.pi_prot_type)
> + sg_miter_start(&prot_miter, cmd->t_prot_sg, cmd->t_prot_nents,
> + op == REQ_OP_READ ? SG_MITER_FROM_SG :
> + SG_MITER_TO_SG);
> +
> for_each_sg(sgl, sg, sgl_nents, i) {
> /*
> * XXX: if the length the device accepts is shorter than the
> @@ -741,6 +755,12 @@ 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, &prot_miter);
> + if (rc)
> + goto fail_put_bios;
> + }
> +
> if (bio_cnt >= IBLOCK_MAX_BIO_PER_TASK) {
> iblock_submit_bios(&list);
> bio_cnt = 0;
> @@ -762,7 +782,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, &prot_miter);
> if (rc)
> goto fail_put_bios;
> }
>
Seems ok to me.
Reviewed-by: Mike Christie <mchristi@xxxxxxxxxx>
