Hello,
On Wed, Aug 22, 2012 at 10:04:03AM -0700, Kent Overstreet wrote:
> Previously, bio_kmalloc() and bio_alloc_bioset() behaved slightly
> different because there was some almost-duplicated code - this fixes
> that issue.
What were those slight differences? Why is it safe to change the
behaviors to match each other?
> struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
> {
> + unsigned front_pad;
> + unsigned inline_vecs;
> unsigned long idx = BIO_POOL_NONE;
> struct bio_vec *bvl = NULL;
> struct bio *bio;
> void *p;
>
> - p = mempool_alloc(bs->bio_pool, gfp_mask);
> + if (nr_iovecs > UIO_MAXIOV)
> + return NULL;
This test used to only happen for bio_kmalloc(). If I follow the code
I can see that UIO_MAXIOV is larger than BIOVEC_MAX_IDX, so this
doesn't really affect the capability of bioset allocs; however, given
that bioset allocation already checks against BIOVEC_MAX_IDX, I don't
see why this test is now also applied to them.
> + if (bs == BIO_KMALLOC_POOL) {
> + p = kmalloc(sizeof(struct bio) +
> + nr_iovecs * sizeof(struct bio_vec),
> + gfp_mask);
> + front_pad = 0;
> + inline_vecs = nr_iovecs;
> + } else {
> + p = mempool_alloc(bs->bio_pool, gfp_mask);
> + front_pad = bs->front_pad;
> + inline_vecs = BIO_INLINE_VECS;
> + }
> +
> if (unlikely(!p))
> return NULL;
> - bio = p + bs->front_pad;
>
> + bio = p + front_pad;
> bio_init(bio);
> - bio->bi_pool = bs;
>
> - if (unlikely(!nr_iovecs))
> - goto out_set;
> -
> - if (nr_iovecs <= BIO_INLINE_VECS) {
> - bvl = bio->bi_inline_vecs;
> - nr_iovecs = BIO_INLINE_VECS;
> - } else {
> + if (nr_iovecs > inline_vecs) {
> bvl = bvec_alloc_bs(gfp_mask, nr_iovecs, &idx, bs);
> if (unlikely(!bvl))
> goto err_free;
> -
> - nr_iovecs = bvec_nr_vecs(idx);
> + } else if (nr_iovecs) {
> + bvl = bio->bi_inline_vecs;
> }
> -out_set:
> +
> + bio->bi_pool = bs;
> bio->bi_flags |= idx << BIO_POOL_OFFSET;
> bio->bi_max_vecs = nr_iovecs;
> bio->bi_io_vec = bvl;
> @@ -331,63 +340,6 @@ err_free:
> }
> EXPORT_SYMBOL(bio_alloc_bioset);
> +extern struct bio_set *fs_bio_set;
> +
> +static inline struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
> +{
> + return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
> +}
> +
> +#define BIO_KMALLOC_POOL NULL
> +
> +static inline struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
> +{
> + return bio_alloc_bioset(gfp_mask, nr_iovecs, BIO_KMALLOC_POOL);
> +}
And we lose /** comments on two exported functions and
bio_alloc_bioset() comment doesn't explain that it now also handles
NULL bioset case. Now that they share common implementation, you can
update bio_alloc_bioset() and refer to it from its wrappers but please
don't drop them like this.
Thanks.
--
tejun
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
