Re: [PATCH v3] target/iblock: split T10 PI SGL across command bios

[Mike Christie <mchristi@redhat.com>]

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@ddn.com>
> ---
> 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@redhat.com>