FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 05-29-2012, 02:10 AM
Kent Overstreet
 
Default block: Kill bi_destructor

On Mon, May 28, 2012 at 10:36:08AM +0900, Tejun Heo wrote:
> On Fri, May 25, 2012 at 01:25:28PM -0700, Kent Overstreet wrote:
> > Now that we've got generic code for freeing bios allocated from bio
> > pools, this isn't needed anymore.
> >
> > This also changes the semantics of bio_free() a bit - it now also frees
> > bios allocated by bio_kmalloc(). It's also no longer exported, as
> > without bi_destructor there should be no need for it to be called
> > anywhere else.
>
> I like this patch but I'd *really* like to see the patch description
> giving some background and explains *why* this is a good change.

Heh. Well, in my view deleting stuff is good by default, and if you can
delete things without user visible effects and without making the code
more complicated...

So I guess given the simplicity of this particular patch in the series
I'm not sure what there is to justify here. Any suggestions on what
would make sense to put in...?

>
> > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> > index 6b7daf3..b6ddbf1 100644
> > --- a/include/linux/blk_types.h
> > +++ b/include/linux/blk_types.h
> > @@ -74,11 +74,8 @@ 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;
>
> Maybe explain that %NULL bi_pool indicates kmalloc backed allocation?

Yeah, I'll update that comment.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-29-2012, 02:20 AM
Tejun Heo
 
Default block: Kill bi_destructor

Hello, Kent.

On Mon, May 28, 2012 at 10:10:42PM -0400, Kent Overstreet wrote:
> Heh. Well, in my view deleting stuff is good by default, and if you can
> delete things without user visible effects and without making the code
> more complicated...
>
> So I guess given the simplicity of this particular patch in the series
> I'm not sure what there is to justify here. Any suggestions on what
> would make sense to put in...?

So, what I would prefer to see is the justification of the whole
changes somewhere in the patch stream, so that patches can do,n

patch 1: update X to not use bi_destructor which will soon be removed.
patch 2: ditto for Y
...
patch N: bi_destructor is stupid because blah blah and after preceding
changes is no longer used. Kill it.

so that when one sees a patch in the series and wonders why the change
is being made, [s]he can follow the patch stream and understand.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-24-2012, 08:11 PM
Kent Overstreet
 
Default block: Kill bi_destructor

Now that we've got generic code for freeing bios allocated from bio
pools, this isn't needed anymore.

This also changes the semantics of bio_free() a bit - it now also frees
bios allocated by bio_kmalloc(). It's also no longer exported, as
without bi_destructor there should be no need for it to be called
anywhere else.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
Documentation/block/biodoc.txt | 5 -----
block/blk-core.c | 2 +-
drivers/block/drbd/drbd_main.c | 13 +------------
drivers/target/target_core_iblock.c | 8 --------
fs/bio.c | 32 +++++++++++++++++---------------
include/linux/bio.h | 2 +-
include/linux/blk_types.h | 3 ---
7 files changed, 20 insertions(+), 45 deletions(-)

diff --git a/Documentation/block/biodoc.txt b/Documentation/block/biodoc.txt
index e418dc0..8df5e8e 100644
--- a/Documentation/block/biodoc.txt
+++ b/Documentation/block/biodoc.txt
@@ -465,7 +465,6 @@ struct bio {
bio_end_io_t *bi_end_io; /* bi_end_io (bio) */
atomic_t bi_cnt; /* pin count: free when it hits zero */
void *bi_private;
- bio_destructor_t *bi_destructor; /* bi_destructor (bio) */
};

With this multipage bio design:
@@ -647,10 +646,6 @@ for a non-clone bio. There are the 6 pools setup for different size biovecs,
so bio_alloc(gfp_mask, nr_iovecs) will allocate a vec_list of the
given size from these slabs.

-The bi_destructor() routine takes into account the possibility of the bio
-having originated from a different source (see later discussions on
-n/w to block transfers and kvec_cb)
-
The bio_get() routine may be used to hold an extra reference on a bio prior
to i/o submission, if the bio fields are likely to be accessed after the
i/o is issued (since the bio may otherwise get freed in case i/o completion
diff --git a/block/blk-core.c b/block/blk-core.c
index 93eb3e4..e9058c2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2794,7 +2794,7 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,

free_and_out:
if (bio)
- bio_free(bio, bs);
+ bio_free(bio);
blk_rq_unprep_clone(rq);

return -ENOMEM;
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 920ede2..19bf632 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -161,23 +161,12 @@ static const struct block_device_operations drbd_ops = {
.release = drbd_release,
};

-static void bio_destructor_drbd(struct bio *bio)
-{
- bio_free(bio, drbd_md_io_bio_set);
-}
-
struct bio *bio_alloc_drbd(gfp_t gfp_mask)
{
- struct bio *bio;
-
if (!drbd_md_io_bio_set)
return bio_alloc(gfp_mask, 1);

- bio = bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
- if (!bio)
- return NULL;
- bio->bi_destructor = bio_destructor_drbd;
- return bio;
+ return bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
}

#ifdef __CHECKER__
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index be65582..9338d0602 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -467,15 +467,7 @@ 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;
diff --git a/fs/bio.c b/fs/bio.c
index 252e253..a301071 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -56,6 +56,9 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
*/
struct bio_set *fs_bio_set;

+/* Only used as a sentinal value */
+static struct bio_set bio_kmalloc_pool;
+
/*
* Our slab pool management
*/
@@ -232,10 +235,21 @@ fallback:
return bvl;
}

-void bio_free(struct bio *bio, struct bio_set *bs)
+void bio_free(struct bio *bio)
{
+ struct bio_set *bs = bio->bi_pool;
void *p;

+ BUG_ON(!bs);
+
+ if (bs == &bio_kmalloc_pool) {
+ /* Bio was allocated by bio_kmalloc() */
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, fs_bio_set);
+ kfree(bio);
+ return;
+ }
+
if (bio_has_allocated_vec(bio))
bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));

@@ -347,13 +361,6 @@ struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
}
EXPORT_SYMBOL(bio_alloc);

-static void bio_kmalloc_destructor(struct bio *bio)
-{
- if (bio_integrity(bio))
- bio_integrity_free(bio, fs_bio_set);
- kfree(bio);
-}
-
/**
* bio_kmalloc - allocate a bio for I/O using kmalloc()
* @gfp_mask: the GFP_ mask given to the slab allocator
@@ -380,7 +387,7 @@ struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
bio->bi_flags |= BIO_POOL_NONE << BIO_POOL_OFFSET;
bio->bi_max_vecs = nr_iovecs;
bio->bi_io_vec = bio->bi_inline_vecs;
- bio->bi_destructor = bio_kmalloc_destructor;
+ bio->bi_pool = &bio_kmalloc_pool;

return bio;
}
@@ -418,12 +425,7 @@ void bio_put(struct bio *bio)
*/
if (atomic_dec_and_test(&bio->bi_cnt)) {
bio_disassociate_task(bio);
- bio->bi_next = NULL;
-
- if (bio->bi_pool)
- bio_free(bio, bio->bi_pool);
- else
- bio->bi_destructor(bio);
+ bio_free(bio);
}
}
EXPORT_SYMBOL(bio_put);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index ba60319..393c87e 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -216,7 +216,7 @@ extern struct bio *bio_alloc(gfp_t, unsigned int);
extern struct bio *bio_kmalloc(gfp_t, unsigned int);
extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
extern void bio_put(struct bio *);
-extern void bio_free(struct bio *, struct bio_set *);
+extern void bio_free(struct bio *);

extern void bio_endio(struct bio *, int);
struct request_queue;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 40411e2..fa45a12 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -84,11 +84,8 @@ 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 */
-
/*
* We can inline a number of vecs at the end of the bio, to avoid
* double allocations for a small number of bio_vecs. This member
--
1.7.7.3

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-25-2012, 11:39 AM
Boaz Harrosh
 
Default block: Kill bi_destructor

On 07/24/2012 11:11 PM, Kent Overstreet wrote:

> Now that we've got generic code for freeing bios allocated from bio
> pools, this isn't needed anymore.
>
> This also changes the semantics of bio_free() a bit - it now also frees
> bios allocated by bio_kmalloc(). It's also no longer exported, as
> without bi_destructor there should be no need for it to be called
> anywhere else.
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>

<snip>

> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index be65582..9338d0602 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -467,15 +467,7 @@ 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;


Merge conflict allmodconfig compilation please?

> diff --git a/fs/bio.c b/fs/bio.c
> index 252e253..a301071 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -56,6 +56,9 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
> */
> struct bio_set *fs_bio_set;
>
> +/* Only used as a sentinal value */
> +static struct bio_set bio_kmalloc_pool;
> +


Id rather if you use a define like:
#define BIO_KMALLOC_POOL ((void *)~0)

So any code access actually crashes in the bug case where
this leaks out. (And there is no actual unused storage allocated)

BTW I like this reuse of the bi_pool member as a flag as well.

Thanks
Boaz

> /*
> * Our slab pool management
> */
> @@ -232,10 +235,21 @@ fallback:
> return bvl;
> }
>
> -void bio_free(struct bio *bio, struct bio_set *bs)
> +void bio_free(struct bio *bio)
> {
> + struct bio_set *bs = bio->bi_pool;
> void *p;
>
> + BUG_ON(!bs);
> +
> + if (bs == &bio_kmalloc_pool) {
> + /* Bio was allocated by bio_kmalloc() */
> + if (bio_integrity(bio))
> + bio_integrity_free(bio, fs_bio_set);
> + kfree(bio);
> + return;
> + }
> +
> if (bio_has_allocated_vec(bio))
> bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
>
> @@ -347,13 +361,6 @@ struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
> }
> EXPORT_SYMBOL(bio_alloc);
>
> -static void bio_kmalloc_destructor(struct bio *bio)
> -{
> - if (bio_integrity(bio))
> - bio_integrity_free(bio, fs_bio_set);
> - kfree(bio);
> -}
> -
> /**
> * bio_kmalloc - allocate a bio for I/O using kmalloc()
> * @gfp_mask: the GFP_ mask given to the slab allocator
> @@ -380,7 +387,7 @@ struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
> bio->bi_flags |= BIO_POOL_NONE << BIO_POOL_OFFSET;
> bio->bi_max_vecs = nr_iovecs;
> bio->bi_io_vec = bio->bi_inline_vecs;
> - bio->bi_destructor = bio_kmalloc_destructor;
> + bio->bi_pool = &bio_kmalloc_pool;
>
> return bio;
> }
> @@ -418,12 +425,7 @@ void bio_put(struct bio *bio)
> */
> if (atomic_dec_and_test(&bio->bi_cnt)) {
> bio_disassociate_task(bio);
> - bio->bi_next = NULL;
> -
> - if (bio->bi_pool)
> - bio_free(bio, bio->bi_pool);
> - else
> - bio->bi_destructor(bio);
> + bio_free(bio);
> }
> }
> EXPORT_SYMBOL(bio_put);
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index ba60319..393c87e 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -216,7 +216,7 @@ extern struct bio *bio_alloc(gfp_t, unsigned int);
> extern struct bio *bio_kmalloc(gfp_t, unsigned int);
> extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
> extern void bio_put(struct bio *);
> -extern void bio_free(struct bio *, struct bio_set *);
> +extern void bio_free(struct bio *);
>
> extern void bio_endio(struct bio *, int);
> struct request_queue;
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 40411e2..fa45a12 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -84,11 +84,8 @@ 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 */
> -
> /*
> * We can inline a number of vecs at the end of the bio, to avoid
> * double allocations for a small number of bio_vecs. This member


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-25-2012, 11:15 PM
Kent Overstreet
 
Default block: Kill bi_destructor

On Wed, Jul 25, 2012 at 02:39:57PM +0300, Boaz Harrosh wrote:
> On 07/24/2012 11:11 PM, Kent Overstreet wrote:
>
> > Now that we've got generic code for freeing bios allocated from bio
> > pools, this isn't needed anymore.
> >
> > This also changes the semantics of bio_free() a bit - it now also frees
> > bios allocated by bio_kmalloc(). It's also no longer exported, as
> > without bi_destructor there should be no need for it to be called
> > anywhere else.
> >
> > Signed-off-by: Kent Overstreet <koverstreet@google.com>
>
> <snip>
>
> > diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> > index be65582..9338d0602 100644
> > --- a/drivers/target/target_core_iblock.c
> > +++ b/drivers/target/target_core_iblock.c
> > @@ -467,15 +467,7 @@ 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;
>
>
> Merge conflict allmodconfig compilation please?
>
> > diff --git a/fs/bio.c b/fs/bio.c
> > index 252e253..a301071 100644
> > --- a/fs/bio.c
> > +++ b/fs/bio.c
> > @@ -56,6 +56,9 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
> > */
> > struct bio_set *fs_bio_set;
> >
> > +/* Only used as a sentinal value */
> > +static struct bio_set bio_kmalloc_pool;
> > +
>
>
> Id rather if you use a define like:
> #define BIO_KMALLOC_POOL ((void *)~0)
>
> So any code access actually crashes in the bug case where
> this leaks out. (And there is no actual unused storage allocated)

I kind of prefer having a sentinal value that can't be used for anything
else, but it doesn't really matter if there's only ever going to be one
sentinal value.

> BTW I like this reuse of the bi_pool member as a flag as well.

Yeah me too, this way bi_pool always tells you how to free the bio.

diff --git a/fs/bio.c b/fs/bio.c
index c7f3442..ebc7309 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -56,6 +56,8 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
*/
struct bio_set *fs_bio_set;

+#define BIO_KMALLOC_POOL ((void *) ~0)
+
/*
* Our slab pool management
*/
@@ -232,10 +234,21 @@ fallback:
return bvl;
}

-void bio_free(struct bio *bio, struct bio_set *bs)
+void bio_free(struct bio *bio)
{
+ struct bio_set *bs = bio->bi_pool;
void *p;

+ BUG_ON(!bs);
+
+ if (bs == BIO_KMALLOC_POOL) {
+ /* Bio was allocated by bio_kmalloc() */
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, fs_bio_set);
+ kfree(bio);
+ return;
+ }
+
if (bio_has_allocated_vec(bio))
bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));

@@ -346,13 +359,6 @@ struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
}
EXPORT_SYMBOL(bio_alloc);

-static void bio_kmalloc_destructor(struct bio *bio)
-{
- if (bio_integrity(bio))
- bio_integrity_free(bio, fs_bio_set);
- kfree(bio);
-}
-
/**
* bio_kmalloc - allocate a bio for I/O using kmalloc()
* @gfp_mask: the GFP_ mask given to the slab allocator
@@ -379,7 +385,7 @@ struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
bio->bi_flags |= BIO_POOL_NONE << BIO_POOL_OFFSET;
bio->bi_max_vecs = nr_iovecs;
bio->bi_io_vec = bio->bi_inline_vecs;
- bio->bi_destructor = bio_kmalloc_destructor;
+ bio->bi_pool = BIO_KMALLOC_POOL;

return bio;
}
@@ -417,12 +423,7 @@ void bio_put(struct bio *bio)
*/
if (atomic_dec_and_test(&bio->bi_cnt)) {
bio_disassociate_task(bio);
- bio->bi_next = NULL;
-
- if (bio->bi_pool)
- bio_free(bio, bio->bi_pool);
- else
- bio->bi_destructor(bio);
+ bio_free(bio);
}
}
EXPORT_SYMBOL(bio_put);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index ba60319..393c87e 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -216,7 +216,7 @@ extern struct bio *bio_alloc(gfp_t, unsigned int);
extern struct bio *bio_kmalloc(gfp_t, unsigned int);
extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
extern void bio_put(struct bio *);
-extern void bio_free(struct bio *, struct bio_set *);
+extern void bio_free(struct bio *);

extern void bio_endio(struct bio *, int);
struct request_queue;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 769799f..4bd8685 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -84,11 +84,8 @@ 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 */
-
/*
* We can inline a number of vecs at the end of the bio, to avoid
* double allocations for a small number of bio_vecs. This member

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-06-2012, 10:08 PM
Kent Overstreet
 
Default block: Kill bi_destructor

Now that we've got generic code for freeing bios allocated from bio
pools, this isn't needed anymore.

This also changes the semantics of bio_free() a bit - it now also frees
bios allocated by bio_kmalloc(). It's also no longer exported, as
without bi_destructor there should be no need for it to be called
anywhere else.

v5: Switch to BIO_KMALLOC_POOL ((void *)~0), per Boaz

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
Documentation/block/biodoc.txt | 5 -----
block/blk-core.c | 2 +-
drivers/block/drbd/drbd_main.c | 13 +------------
fs/bio.c | 31 ++++++++++++++++---------------
include/linux/bio.h | 2 +-
include/linux/blk_types.h | 3 ---
6 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/Documentation/block/biodoc.txt b/Documentation/block/biodoc.txt
index e418dc0..8df5e8e 100644
--- a/Documentation/block/biodoc.txt
+++ b/Documentation/block/biodoc.txt
@@ -465,7 +465,6 @@ struct bio {
bio_end_io_t *bi_end_io; /* bi_end_io (bio) */
atomic_t bi_cnt; /* pin count: free when it hits zero */
void *bi_private;
- bio_destructor_t *bi_destructor; /* bi_destructor (bio) */
};

With this multipage bio design:
@@ -647,10 +646,6 @@ for a non-clone bio. There are the 6 pools setup for different size biovecs,
so bio_alloc(gfp_mask, nr_iovecs) will allocate a vec_list of the
given size from these slabs.

-The bi_destructor() routine takes into account the possibility of the bio
-having originated from a different source (see later discussions on
-n/w to block transfers and kvec_cb)
-
The bio_get() routine may be used to hold an extra reference on a bio prior
to i/o submission, if the bio fields are likely to be accessed after the
i/o is issued (since the bio may otherwise get freed in case i/o completion
diff --git a/block/blk-core.c b/block/blk-core.c
index 93eb3e4..e9058c2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2794,7 +2794,7 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,

free_and_out:
if (bio)
- bio_free(bio, bs);
+ bio_free(bio);
blk_rq_unprep_clone(rq);

return -ENOMEM;
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 920ede2..19bf632 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -161,23 +161,12 @@ static const struct block_device_operations drbd_ops = {
.release = drbd_release,
};

-static void bio_destructor_drbd(struct bio *bio)
-{
- bio_free(bio, drbd_md_io_bio_set);
-}
-
struct bio *bio_alloc_drbd(gfp_t gfp_mask)
{
- struct bio *bio;
-
if (!drbd_md_io_bio_set)
return bio_alloc(gfp_mask, 1);

- bio = bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
- if (!bio)
- return NULL;
- bio->bi_destructor = bio_destructor_drbd;
- return bio;
+ return bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
}

#ifdef __CHECKER__
diff --git a/fs/bio.c b/fs/bio.c
index c7f3442..ebc7309 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -56,6 +56,8 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
*/
struct bio_set *fs_bio_set;

+#define BIO_KMALLOC_POOL ((void *) ~0)
+
/*
* Our slab pool management
*/
@@ -232,10 +234,21 @@ fallback:
return bvl;
}

-void bio_free(struct bio *bio, struct bio_set *bs)
+void bio_free(struct bio *bio)
{
+ struct bio_set *bs = bio->bi_pool;
void *p;

+ BUG_ON(!bs);
+
+ if (bs == BIO_KMALLOC_POOL) {
+ /* Bio was allocated by bio_kmalloc() */
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, fs_bio_set);
+ kfree(bio);
+ return;
+ }
+
if (bio_has_allocated_vec(bio))
bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));

@@ -346,13 +359,6 @@ struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
}
EXPORT_SYMBOL(bio_alloc);

-static void bio_kmalloc_destructor(struct bio *bio)
-{
- if (bio_integrity(bio))
- bio_integrity_free(bio, fs_bio_set);
- kfree(bio);
-}
-
/**
* bio_kmalloc - allocate a bio for I/O using kmalloc()
* @gfp_mask: the GFP_ mask given to the slab allocator
@@ -379,7 +385,7 @@ struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
bio->bi_flags |= BIO_POOL_NONE << BIO_POOL_OFFSET;
bio->bi_max_vecs = nr_iovecs;
bio->bi_io_vec = bio->bi_inline_vecs;
- bio->bi_destructor = bio_kmalloc_destructor;
+ bio->bi_pool = BIO_KMALLOC_POOL;

return bio;
}
@@ -417,12 +423,7 @@ void bio_put(struct bio *bio)
*/
if (atomic_dec_and_test(&bio->bi_cnt)) {
bio_disassociate_task(bio);
- bio->bi_next = NULL;
-
- if (bio->bi_pool)
- bio_free(bio, bio->bi_pool);
- else
- bio->bi_destructor(bio);
+ bio_free(bio);
}
}
EXPORT_SYMBOL(bio_put);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index ba60319..393c87e 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -216,7 +216,7 @@ extern struct bio *bio_alloc(gfp_t, unsigned int);
extern struct bio *bio_kmalloc(gfp_t, unsigned int);
extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
extern void bio_put(struct bio *);
-extern void bio_free(struct bio *, struct bio_set *);
+extern void bio_free(struct bio *);

extern void bio_endio(struct bio *, int);
struct request_queue;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 401c573..d10c2e49 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -84,11 +84,8 @@ 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 */
-
/*
* We can inline a number of vecs at the end of the bio, to avoid
* double allocations for a small number of bio_vecs. This member
--
1.7.7.3

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-07-2012, 03:19 AM
Mike Snitzer
 
Default block: Kill bi_destructor

On Mon, Aug 06 2012 at 6:08pm -0400,
Kent Overstreet <koverstreet@google.com> wrote:

> Now that we've got generic code for freeing bios allocated from bio
> pools, this isn't needed anymore.
>
> This also changes the semantics of bio_free() a bit - it now also frees
> bios allocated by bio_kmalloc(). It's also no longer exported, as
> without bi_destructor there should be no need for it to be called
> anywhere else.

Seems you forgot to remove bio_free's EXPORT_SYMBOL

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-08-2012, 10:22 PM
Tejun Heo
 
Default block: Kill bi_destructor

Hello,

On Mon, Aug 06, 2012 at 03:08:34PM -0700, Kent Overstreet wrote:
> Now that we've got generic code for freeing bios allocated from bio
> pools, this isn't needed anymore.
>
> This also changes the semantics of bio_free() a bit - it now also frees
> bios allocated by bio_kmalloc(). It's also no longer exported, as
> without bi_destructor there should be no need for it to be called
> anywhere else.
>
> v5: Switch to BIO_KMALLOC_POOL ((void *)~0), per Boaz
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> ---
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index 920ede2..19bf632 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -161,23 +161,12 @@ static const struct block_device_operations drbd_ops = {
> .release = drbd_release,
> };
>
> -static void bio_destructor_drbd(struct bio *bio)
> -{
> - bio_free(bio, drbd_md_io_bio_set);
> -}
> -
> struct bio *bio_alloc_drbd(gfp_t gfp_mask)
> {
> - struct bio *bio;
> -
> if (!drbd_md_io_bio_set)
> return bio_alloc(gfp_mask, 1);
>
> - bio = bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
> - if (!bio)
> - return NULL;
> - bio->bi_destructor = bio_destructor_drbd;
> - return bio;
> + return bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
> }

Does this chunk belong to this patch?

> @@ -56,6 +56,8 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
> */
> struct bio_set *fs_bio_set;
>
> +#define BIO_KMALLOC_POOL ((void *) ~0)

What's wrong with good ol' NULL?

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-09-2012, 12:14 AM
Kent Overstreet
 
Default block: Kill bi_destructor

On Mon, Aug 06, 2012 at 11:19:21PM -0400, Mike Snitzer wrote:
> On Mon, Aug 06 2012 at 6:08pm -0400,
> Kent Overstreet <koverstreet@google.com> wrote:
>
> > Now that we've got generic code for freeing bios allocated from bio
> > pools, this isn't needed anymore.
> >
> > This also changes the semantics of bio_free() a bit - it now also frees
> > bios allocated by bio_kmalloc(). It's also no longer exported, as
> > without bi_destructor there should be no need for it to be called
> > anywhere else.
>
> Seems you forgot to remove bio_free's EXPORT_SYMBOL

Whoops - thanks, fixed.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-09-2012, 12:21 AM
Kent Overstreet
 
Default block: Kill bi_destructor

On Wed, Aug 08, 2012 at 03:22:23PM -0700, Tejun Heo wrote:
> Hello,
>
> On Mon, Aug 06, 2012 at 03:08:34PM -0700, Kent Overstreet wrote:
> > Now that we've got generic code for freeing bios allocated from bio
> > pools, this isn't needed anymore.
> >
> > This also changes the semantics of bio_free() a bit - it now also frees
> > bios allocated by bio_kmalloc(). It's also no longer exported, as
> > without bi_destructor there should be no need for it to be called
> > anywhere else.
> >
> > v5: Switch to BIO_KMALLOC_POOL ((void *)~0), per Boaz
> >
> > Signed-off-by: Kent Overstreet <koverstreet@google.com>
> > ---
> > diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> > index 920ede2..19bf632 100644
> > --- a/drivers/block/drbd/drbd_main.c
> > +++ b/drivers/block/drbd/drbd_main.c
> > @@ -161,23 +161,12 @@ static const struct block_device_operations drbd_ops = {
> > .release = drbd_release,
> > };
> >
> > -static void bio_destructor_drbd(struct bio *bio)
> > -{
> > - bio_free(bio, drbd_md_io_bio_set);
> > -}
> > -
> > struct bio *bio_alloc_drbd(gfp_t gfp_mask)
> > {
> > - struct bio *bio;
> > -
> > if (!drbd_md_io_bio_set)
> > return bio_alloc(gfp_mask, 1);
> >
> > - bio = bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
> > - if (!bio)
> > - return NULL;
> > - bio->bi_destructor = bio_destructor_drbd;
> > - return bio;
> > + return bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
> > }
>
> Does this chunk belong to this patch?

Hrm, that should've been in the first patch. Will move it.

>
> > @@ -56,6 +56,8 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
> > */
> > struct bio_set *fs_bio_set;
> >
> > +#define BIO_KMALLOC_POOL ((void *) ~0)
>
> What's wrong with good ol' NULL?

If it's NULL, we can't distinguish between bios where that field wasn't
set (i.e. bios that were statically allocated somewhere) from bios that
were allocated by bio_kmalloc().

It's just there to make debugging easier - if bi_cnt goes to 0 on a bio
where it shouldn't we'll catch it at the BUG_ON() in bio_free() instead
of kfreeing a bad pointer.

>
> Thanks.
>
> --
> tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 10:26 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org