Re: [PATCH v3 01/16] block: Generalized bio pool freeing

[Boaz Harrosh <bharrosh@panasas.com>]

On 05/25/2012 11:25 PM, Kent Overstreet wrote:
<snip>

> diff --git a/fs/bio.c b/fs/bio.c
> index e453924..3667cef 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -269,10 +269,6 @@ EXPORT_SYMBOL(bio_init);
>   *   bio_alloc_bioset will try its own mempool to satisfy the allocation.
>   *   If %__GFP_WAIT is set then we will block on the internal pool waiting
>   *   for a &struct bio to become free.
> - *
> - *   Note that the caller must set ->bi_destructor on successful return
> - *   of a bio, to do the appropriate freeing of the bio once the reference
> - *   count drops to zero.
>   **/
>  struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
>  {
> @@ -287,6 +283,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
>  	bio = p + bs->front_pad;
>  
>  	bio_init(bio);
> +	bio->bi_pool = bs;
>  


I really hate it that people in the Kernel at some point decided
to name bio_set(s) "pool". This is against Kernel style guide.

You don't overload the namespace and you don't call one-thing
another-thing.

For one "pool"s are already another thing. And for-most the struct
is bio_set a pointer to it should be called bio_set or in short
bs.

The original bio_set code kept the "bio_set" and/or "bs" naming
conventions. But later code. Kept breaking it with "pool". In
code and in comments.

But this Patch is the most blasphemous of all because it's
the first instance of block core code that calls it a "pool".
Up to now bio core kept the bio_set naming.

Please change the name to bi_bs so we can have:
+	bio->bi_bs = bs;

For me above code is an immediate alarm in my mind. Rrrr
false alarm. Please don't let my poor old mind Jump
unnecessarily.

>  	if (unlikely(!nr_iovecs))
>  		goto out_set;
> @@ -313,11 +310,6 @@ err_free:
>  }
>  EXPORT_SYMBOL(bio_alloc_bioset);
>  
> -static void bio_fs_destructor(struct bio *bio)
> -{
> -	bio_free(bio, fs_bio_set);
> -}
> -
>  /**
>   *	bio_alloc - allocate a new bio, memory pool backed
>   *	@gfp_mask: allocation mask to use
> @@ -339,12 +331,7 @@ static void bio_fs_destructor(struct bio *bio)
>   */
>  struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
>  {
> -	struct bio *bio = bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
> -
> -	if (bio)
> -		bio->bi_destructor = bio_fs_destructor;
> -
> -	return bio;
> +	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
>  }
>  EXPORT_SYMBOL(bio_alloc);
>  
> @@ -419,7 +406,11 @@ void bio_put(struct bio *bio)
>  	 */
>  	if (atomic_dec_and_test(&bio->bi_cnt)) {
>  		bio->bi_next = NULL;
> -		bio->bi_destructor(bio);
> +
> +		if (bio->bi_pool)
> +			bio_free(bio, bio->bi_pool);
> +		else
> +			bio->bi_destructor(bio);
>  	}
>  }
>  EXPORT_SYMBOL(bio_put);
> @@ -470,12 +461,11 @@ EXPORT_SYMBOL(__bio_clone);
>   */
>  struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
>  {
> -	struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
> +	struct bio *b = bio_alloc(gfp_mask, bio->bi_max_vecs);
>  
>  	if (!b)
>  		return NULL;
>  
> -	b->bi_destructor = bio_fs_destructor;
>  	__bio_clone(b, bio);
>  
>  	if (bio_integrity(bio)) {
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 4053cbd..dc0e399 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -70,6 +70,9 @@ struct bio {
>  	struct bio_integrity_payload *bi_integrity;  /* data integrity */
>  #endif
>  
> +	/* If bi_pool is non NULL, bi_destructor is not called */
> +	struct bio_set		*bi_pool;
> +


Please, Please , Please Don't forget this time. 
	bi_bs.
Pools are something a bit different (though related) in the
Kernel.

>  	bio_destructor_t	*bi_destructor;	/* destructor */
>  
>  	/*


BTW: I would have loved it if some brave sole would go and
     rename struct bio_set => bio_pool. Because when we speak
     about them they are pools. (And very much related to memory pools)

     And actually a set is something else. This here is definitely
     a pool. Because a set is a group of objects that have a relating
     trait, a relationship, a uniting factor. Its when we want to act
     on a group of objects as one unit.

Thanks
Boaz