Re: [PATCH v6 11/13] block: Rework bio_pair_split()

[Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

Hello, Kent.

On Wed, Aug 22, 2012 at 10:04:08AM -0700, Kent Overstreet wrote:
> This changes bio_pair_split() to use the new bio_split() underneath,
> which gets rid of the single page bio limitation. The various callers
> are fixed up for the slightly different struct bio_pair, and to remove
> the unnecessary checks.

This changes an existing API both in its interface and behavior and
there's no detailed explanation on how it's changed and what are the
implications.

> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 1f5b483..63e5852 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -751,14 +751,13 @@ static struct bio *bio_chain_clone(struct bio **old, struct bio **next,
>  
>  			/* split the bio. We'll release it either in the next
>  			   call, or it will have to be released outside */
> -			bp = bio_pair_split(old_chain,
> -					    (len - total) / SECTOR_SIZE);
> +			bp = bio_pair_split(old_chain, (len - total) / SECTOR_SIZE);

Probably belongs to the previous patch which renamed bio_split() to
bio_pair_split()?  Another thing is, is this renaming really
necessary?  If you did,

	* s/bio_split()/bio_pair_split().

	* introduce better and prettier bio_split() which has
          different semantics.

	* replace bio_pair_split() users with bio_split().

the renaming would have made sense, but you renamed an existing API,
intrudced a new one and then changed the renamed old API.  Doesn't
make too much sense to me.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0f31ec4..9fa07c7 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1080,15 +1080,9 @@ static void make_request(struct mddev *mddev, struct bio * bio)
>  		     && (conf->geo.near_copies < conf->geo.raid_disks
>  			 || conf->prev.near_copies < conf->prev.raid_disks))) {
>  		struct bio_pair *bp;
> -		/* Sanity check -- queue functions should prevent this happening */
> -		if (bio->bi_vcnt != 1 ||
> -		    bio->bi_idx != 0)
> -			goto bad_map;
> -		/* This is a one page bio that upper layers
> -		 * refuse to split for us, so we need to split it.
> -		 */
> +
>  		bp = bio_pair_split(bio,
> -				    chunk_sects - (bio->bi_sector & (chunk_sects - 1)) );
> +				    chunk_sects - (bio->bi_sector & (chunk_sects - 1)));

I suppose this one too belongs to the previous rename patch?

> --- a/fs/bio-integrity.c
> +++ b/fs/bio-integrity.c
> @@ -681,50 +681,6 @@ void bio_integrity_trim(struct bio *bio, unsigned int offset,
>  EXPORT_SYMBOL(bio_integrity_trim);
>  
>  /**
> - * bio_integrity_split - Split integrity metadata
> - * @bio:	Protected bio
> - * @bp:		Resulting bio_pair
> - * @sectors:	Offset
> - *
> - * Description: Splits an integrity page into a bio_pair.
> - */
> -void bio_integrity_split(struct bio *bio, struct bio_pair *bp, int sectors)
> -{
> -	struct blk_integrity *bi;
> -	struct bio_integrity_payload *bip = bio->bi_integrity;
> -	unsigned int nr_sectors;
> -
> -	if (bio_integrity(bio) == 0)
> -		return;
> -
> -	bi = bdev_get_integrity(bio->bi_bdev);
> -	BUG_ON(bi == NULL);
> -	BUG_ON(bip->bip_vcnt != 1);
> -
> -	nr_sectors = bio_integrity_hw_sectors(bi, sectors);
> -
> -	bp->bio1.bi_integrity = &bp->bip1;
> -	bp->bio2.bi_integrity = &bp->bip2;
> -
> -	bp->iv1 = bip->bip_vec[0];
> -	bp->iv2 = bip->bip_vec[0];
> -
> -	bp->bip1.bip_vec[0] = bp->iv1;
> -	bp->bip2.bip_vec[0] = bp->iv2;
> -
> -	bp->iv1.bv_len = sectors * bi->tuple_size;
> -	bp->iv2.bv_offset += sectors * bi->tuple_size;
> -	bp->iv2.bv_len -= sectors * bi->tuple_size;
> -
> -	bp->bip1.bip_sector = bio->bi_integrity->bip_sector;
> -	bp->bip2.bip_sector = bio->bi_integrity->bip_sector + nr_sectors;
> -
> -	bp->bip1.bip_vcnt = bp->bip2.bip_vcnt = 1;
> -	bp->bip1.bip_idx = bp->bip2.bip_idx = 0;
> -}
> -EXPORT_SYMBOL(bio_integrity_split);

I complained about this in the last posting and in the previous patch.
Please respond.  Martin, are you okay with these integrity changes?

> -static void bio_pair_end_1(struct bio *bi, int err)
> +static void bio_pair_end(struct bio *bio, int error)
>  {
> -	struct bio_pair *bp = container_of(bi, struct bio_pair, bio1);
> +	struct bio_pair *bp = bio->bi_private;
>  
> -	if (err)
> -		bp->error = err;
> -
> -	bio_pair_release(bp);
> -}
> -
> -static void bio_pair_end_2(struct bio *bi, int err)
> -{
> -	struct bio_pair *bp = container_of(bi, struct bio_pair, bio2);
> -
> -	if (err)
> -		bp->error = err;
> +	if (error)
> +		clear_bit(BIO_UPTODATE, &bp->orig->bi_flags);
>  
>  	bio_pair_release(bp);
>  }

Why is losing error value okay here?

> @@ -1856,8 +1846,7 @@ static int __init init_bio(void)
>  	if (bioset_integrity_create(fs_bio_set, BIO_POOL_SIZE))
>  		panic("bio: can't create integrity pool\n");
>  
> -	bio_split_pool = mempool_create_kmalloc_pool(BIO_SPLIT_ENTRIES,
> -						     sizeof(struct bio_pair));
> +	bio_split_pool = bioset_create(BIO_POOL_SIZE, offsetof(struct bio_pair, split));
>  	if (!bio_split_pool)
>  		panic("bio: can't create split pool\n");

It would be nice to mention that using this from stacking drivers is
inherently broken.  This is something which has been broken before
this patch but still.  bio_split*() should always require separate
biosets.

> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 1c3bb47..3ad3540 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -192,14 +192,13 @@ struct bio_integrity_payload {
>   *   in bio2.bi_private
>   */
>  struct bio_pair {
> -	struct bio			bio1, bio2;
> -	struct bio_vec			bv1, bv2;
> -#if defined(CONFIG_BLK_DEV_INTEGRITY)
> -	struct bio_integrity_payload	bip1, bip2;
> -	struct bio_vec			iv1, iv2;
> -#endif
> -	atomic_t			cnt;
> -	int				error;
> +	atomic_t		cnt;
> +
> +	bio_end_io_t		*bi_end_io;
> +	void			*bi_private;
> +
> +	struct bio		*orig;
> +	struct bio		split;
>  };

So, this is struct is allocated as frontpad, which is a pretty unusual
thing to do.  Please explain and emphasize that ->split should come
last.  Also, given that it's a pair split, it would be nice to somehow
indicate that ->split is the earlier half.  Before this change it was
pretty clear with ->bio1/2.

Thanks.

-- 
tejun