Re: [PATCH v4 01/12] block: Generalized bio pool freeing
On Wed, Jul 25, 2012 at 02:06:23PM +0300, Boaz Harrosh wrote:
> On 07/24/2012 11:11 PM, Kent Overstreet wrote:
>
> > With the old code, when you allocate a bio from a bio pool you have to
> > implement your own destructor that knows how to find the bio pool the
> > bio was originally allocated from.
> >
> > This adds a new field to struct bio (bi_pool) and changes
> > bio_alloc_bioset() to use it. This makes various bio destructors
> > unnecessary, so they're then deleted.
> >
> > Signed-off-by: Kent Overstreet
>
>
>
> > diff --git a/drivers/target/target_core_iblock.c
> > b/drivers/target/target_core_iblock.c
> > index fd47950..be65582 100644
> > --- a/drivers/target/target_core_iblock.c
> > +++ b/drivers/target/target_core_iblock.c
> > @@ -447,14 +447,6 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
> > kfree(ibr);
> > }
> >
> > -static void iblock_bio_destructor(struct bio *bio)
> > -{
> > - struct se_cmd *cmd = bio->bi_private;
> > - struct iblock_dev *ib_dev = cmd->se_dev->dev_ptr;
> > -
> > - bio_free(bio, ib_dev->ibd_bio_set);
> > -}
> > -
> > static struct bio *
> > iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32 sg_num)
> > {
> > @@ -475,8 +467,15 @@ iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32
> > sg_num)
> > }
> >
> > bio->bi_bdev = ib_dev->ibd_bd;
> > +<<< HEAD
> > bio->bi_private = cmd;
> > bio->bi_destructor = iblock_bio_destructor;
> > +||| merged common ancestors
> > + bio->bi_private = task;
> > + bio->bi_destructor = iblock_bio_destructor;
> > +===
> > + bio->bi_private = task;
> > +>>> block: Generalized bio pool freeing
> > bio->bi_end_io = &iblock_bio_done;
> > bio->bi_sector = lba;
> > return bio;
>
>
> You left out a rebase merge conflict. Did you allmodconfig compile
> these patches?
Argh, clearly not. And I even fixed that rebase merge conflict at one
point, that's distressing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 01/12] block: Generalized bio pool freeing
On 07/24/2012 11:11 PM, Kent Overstreet wrote:
> With the old code, when you allocate a bio from a bio pool you have to
> implement your own destructor that knows how to find the bio pool the
> bio was originally allocated from.
>
> This adds a new field to struct bio (bi_pool) and changes
> bio_alloc_bioset() to use it. This makes various bio destructors
> unnecessary, so they're then deleted.
>
> Signed-off-by: Kent Overstreet
> diff --git a/drivers/target/target_core_iblock.c
> b/drivers/target/target_core_iblock.c
> index fd47950..be65582 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -447,14 +447,6 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
> kfree(ibr);
> }
>
> -static void iblock_bio_destructor(struct bio *bio)
> -{
> - struct se_cmd *cmd = bio->bi_private;
> - struct iblock_dev *ib_dev = cmd->se_dev->dev_ptr;
> -
> - bio_free(bio, ib_dev->ibd_bio_set);
> -}
> -
> static struct bio *
> iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32 sg_num)
> {
> @@ -475,8 +467,15 @@ iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32
> sg_num)
> }
>
> bio->bi_bdev = ib_dev->ibd_bd;
> +<<< HEAD
> bio->bi_private = cmd;
> bio->bi_destructor = iblock_bio_destructor;
> +||| merged common ancestors
> + bio->bi_private = task;
> + bio->bi_destructor = iblock_bio_destructor;
> +===
> + bio->bi_private = task;
> +>>> block: Generalized bio pool freeing
> bio->bi_end_io = &iblock_bio_done;
> bio->bi_sector = lba;
> return bio;
You left out a rebase merge conflict. Did you allmodconfig compile
these patches?
Boaz
> diff --git a/fs/bio.c b/fs/bio.c
> index 73922ab..1c6c8b7 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -271,10 +271,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)
> {
> @@ -289,6 +285,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;
>
> if (unlikely(!nr_iovecs))
> goto out_set;
> @@ -315,11 +312,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
> @@ -341,12 +333,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);
>
> @@ -422,7 +409,11 @@ void bio_put(struct bio *bio)
> if (atomic_dec_and_test(&bio->bi_cnt)) {
> bio_disassociate_task(bio);
> 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);
> @@ -473,12 +464,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 0edb65d..293547e 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -80,6 +80,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;
> +
> bio_destructor_t*bi_destructor; /* destructor */
>
> /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 01/12] block: Generalized bio pool freeing
With the old code, when you allocate a bio from a bio pool you have to
implement your own destructor that knows how to find the bio pool the
bio was originally allocated from.
This adds a new field to struct bio (bi_pool) and changes
bio_alloc_bioset() to use it. This makes various bio destructors
unnecessary, so they're then deleted.
Signed-off-by: Kent Overstreet
---
drivers/md/dm-crypt.c |9 -
drivers/md/dm-io.c | 11 ---
drivers/md/dm.c | 20
drivers/md/md.c | 28
drivers/target/target_core_iblock.c | 15 +++
fs/bio.c| 26 --
include/linux/blk_types.h |3 +++
7 files changed, 22 insertions(+), 90 deletions(-)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3f06df5..40716d8 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -808,14 +808,6 @@ static int crypt_convert(struct crypt_config *cc,
return 0;
}
-static void dm_crypt_bio_destructor(struct bio *bio)
-{
- struct dm_crypt_io *io = bio->bi_private;
- struct crypt_config *cc = io->target->private;
-
- bio_free(bio, cc->bs);
-}
-
/*
* Generate a new unfragmented bio with the given size
* This should never violate the device limitations
@@ -985,7 +977,6 @@ static void clone_init(struct dm_crypt_io *io, struct bio
*clone)
clone->bi_end_io = crypt_endio;
clone->bi_bdev= cc->dev->bdev;
clone->bi_rw = io->base_bio->bi_rw;
- clone->bi_destructor = dm_crypt_bio_destructor;
}
static int kcryptd_io_read(struct dm_crypt_io *io, gfp_t gfp)
diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
index ea5dd28..1c46f97 100644
--- a/drivers/md/dm-io.c
+++ b/drivers/md/dm-io.c
@@ -249,16 +249,6 @@ static void vm_dp_init(struct dpages *dp, void *data)
dp->context_ptr = data;
}
-static void dm_bio_destructor(struct bio *bio)
-{
- unsigned region;
- struct io *io;
-
- retrieve_io_and_region_from_bio(bio, &io, ®ion);
-
- bio_free(bio, io->client->bios);
-}
-
/*
* Functions for getting the pages from kernel memory.
*/
@@ -317,7 +307,6 @@ static void do_region(int rw, unsigned region, struct
dm_io_region *where,
bio->bi_sector = where->sector + (where->count - remaining);
bio->bi_bdev = where->bdev;
bio->bi_end_io = endio;
- bio->bi_destructor = dm_bio_destructor;
store_io_and_region_in_bio(bio, io, region);
if (rw & REQ_DISCARD) {
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index e24143c..40b7735 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -681,11 +681,6 @@ static void clone_endio(struct bio *bio, int error)
}
}
- /*
-* Store md for cleanup instead of tio which is about to get freed.
-*/
- bio->bi_private = md->bs;
-
free_tio(md, tio);
bio_put(bio);
dec_pending(io, error);
@@ -1013,11 +1008,6 @@ static void __map_bio(struct dm_target *ti, struct bio
*clone,
/* error the io and bail out, or requeue it if needed */
md = tio->io->md;
dec_pending(tio->io, r);
- /*
-* Store bio_set for cleanup.
-*/
- clone->bi_end_io = NULL;
- clone->bi_private = md->bs;
bio_put(clone);
free_tio(md, tio);
} else if (r) {
@@ -1036,13 +1026,6 @@ struct clone_info {
unsigned short idx;
};
-static void dm_bio_destructor(struct bio *bio)
-{
- struct bio_set *bs = bio->bi_private;
-
- bio_free(bio, bs);
-}
-
/*
* Creates a little bio that just does part of a bvec.
*/
@@ -1054,7 +1037,6 @@ static struct bio *split_bvec(struct bio *bio, sector_t
sector,
struct bio_vec *bv = bio->bi_io_vec + idx;
clone = bio_alloc_bioset(GFP_NOIO, 1, bs);
- clone->bi_destructor = dm_bio_destructor;
*clone->bi_io_vec = *bv;
clone->bi_sector = sector;
@@ -1086,7 +1068,6 @@ static struct bio *clone_bio(struct bio *bio, sector_t
sector,
clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
__bio_clone(clone, bio);
- clone->bi_destructor = dm_bio_destructor;
clone->bi_sector = sector;
clone->bi_idx = idx;
clone->bi_vcnt = idx + bv_count;
@@ -1131,7 +1112,6 @@ static void __issue_target_request(struct clone_info *ci,
struct dm_target *ti,
*/
clone = bio_alloc_bioset(GFP_NOIO, ci->bio->bi_max_vecs, ci->md->bs);
__bio_clone(clone, ci->bio);
- clone->bi_destructor = dm_bio_destructor;
if (len) {
clone->bi_sector = ci->sector;
clone->bi_size = to_bytes(len);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d5ab449
