From: Kent Overstreet <koverstreet@google.com>
To: Boaz Harrosh <bharrosh@panasas.com>
Cc: linux-bcache@vger.kernel.org, linux-kernel@vger.kernel.org,
dm-devel@redhat.com, tj@kernel.org, axboe@kernel.dk,
agk@redhat.com, neilb@suse.de, drbd-dev@lists.linbit.com,
vgoyal@redhat.com, mpatocka@redhat.com, sage@newdream.net,
yehuda@hq.newdream.net
Subject: Re: [PATCH v4 05/12] block: Kill bi_destructor
Date: Wed, 25 Jul 2012 16:15:15 -0700 [thread overview]
Message-ID: <20120725231515.GC8673@moria.home.lan> (raw)
In-Reply-To: <500FDB0D.4070605@panasas.com>
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
next prev parent reply other threads:[~2012-07-25 23:15 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-24 20:11 [PATCH v4 00/12] Block cleanups Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 01/12] block: Generalized bio pool freeing Kent Overstreet
2012-07-25 11:06 ` Boaz Harrosh
2012-07-25 23:38 ` Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 02/12] dm: Use bioset's front_pad for dm_rq_clone_bio_info Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 03/12] block: Add bio_reset() Kent Overstreet
2012-07-25 11:19 ` Boaz Harrosh
2012-07-25 22:56 ` Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 04/12] pktcdvd: Switch to bio_kmalloc() Kent Overstreet
2012-07-25 11:29 ` Boaz Harrosh
2012-07-25 23:01 ` Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 05/12] block: Kill bi_destructor Kent Overstreet
2012-07-25 11:39 ` Boaz Harrosh
2012-07-25 23:15 ` Kent Overstreet [this message]
2012-07-24 20:11 ` [PATCH v4 06/12] block: Add an explicit bio flag for bios that own their bvec Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 07/12] block: Rename bio_split() -> bio_pair_split() Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 08/12] block: Introduce new bio_split() Kent Overstreet
2012-07-25 11:55 ` Boaz Harrosh
2012-07-25 23:26 ` Kent Overstreet
2012-07-27 0:50 ` [PATCH] A possible deadlock with stacked devices (was: [PATCH v4 08/12] block: Introduce new bio_split()) Mikulas Patocka
2012-08-15 23:07 ` Kent Overstreet
2012-08-29 16:08 ` Mikulas Patocka
2012-07-24 20:11 ` [PATCH v4 09/12] block: Rework bio_pair_split() Kent Overstreet
2012-07-25 12:03 ` Boaz Harrosh
2012-07-24 20:11 ` [PATCH v4 10/12] block: Add bio_clone_kmalloc() Kent Overstreet
2012-07-25 12:05 ` Boaz Harrosh
2012-07-24 20:11 ` [PATCH v4 11/12] block: Add bio_clone_bioset() Kent Overstreet
2012-07-24 20:11 ` [PATCH v4 12/12] block: Only clone bio vecs that are in use Kent Overstreet
2012-08-07 3:17 ` Muthu Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120725231515.GC8673@moria.home.lan \
--to=koverstreet@google.com \
--cc=agk@redhat.com \
--cc=axboe@kernel.dk \
--cc=bharrosh@panasas.com \
--cc=dm-devel@redhat.com \
--cc=drbd-dev@lists.linbit.com \
--cc=linux-bcache@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mpatocka@redhat.com \
--cc=neilb@suse.de \
--cc=sage@newdream.net \
--cc=tj@kernel.org \
--cc=vgoyal@redhat.com \
--cc=yehuda@hq.newdream.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).