From: Kent Overstreet <koverstreet@google.com>
To: Tejun Heo <tj@kernel.org>
Cc: linux-bcache@vger.kernel.org, linux-kernel@vger.kernel.org,
dm-devel@redhat.com, vgoyal@redhat.com, mpatocka@redhat.com,
bharrosh@panasas.com, Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH v7 3/9] block: Add bio_reset()
Date: Tue, 28 Aug 2012 15:53:38 -0700 [thread overview]
Message-ID: <20120828225337.GH1048@moria.home.lan> (raw)
In-Reply-To: <20120828221715.GD1048@moria.home.lan>
On Tue, Aug 28, 2012 at 03:17:15PM -0700, Kent Overstreet wrote:
> On Tue, Aug 28, 2012 at 01:31:48PM -0700, Tejun Heo wrote:
> > > + unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
> > > +
> > > + if (bio_integrity(bio))
> > > + bio_integrity_free(bio, bio->bi_pool);
> > > +
> > > + bio_disassociate_task(bio);
> >
> > Is this desirable? Why?
>
> *twitch* I should've thought more when I made that change.
>
> It occurs to me now that we specifically talked about that and decided
> to do it the other way - when I changed that I was just looking at
> bio_free() and decided they needed to be symmetrical.
>
> I still think they should be symmetrical, but if that's true bi_ioc and
> bi_css need to be moved, and also bio_disassociate_task() should be
> getting called from bio_free(), not bio_put().
Though - looking at it again I didn't actually screw anything up when I
made that change, it's just bad style.
Just screwing around a bit with the patch below, but - couple thoughts:
1) I hate duplicated code, and for the stuff in
bio_init()/bio_free()/bio_reset() it's perhaps not worth it, it is the
kind of stuff that gets out of sync.
2) It'd be better to have bio_reset() reset as much as possible, i.e. be
as close to bio_init() as posible. Fewer differences to confuse people.
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index 7c8fe1d..a38bc7d 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -148,6 +148,9 @@ void bio_integrity_free(struct bio *bio, struct bio_set *bs)
BUG_ON(bip == NULL);
+ if (!bs)
+ bs = fs_bio_set;
+
/* A cloned bio doesn't own the integrity metadata */
if (!bio_flagged(bio, BIO_CLONED) && !bio_flagged(bio, BIO_FS_INTEGRITY)
&& bip->bip_buf != NULL)
diff --git a/fs/bio.c b/fs/bio.c
index c938f42..56d8fa2 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -234,39 +234,47 @@ fallback:
return bvl;
}
+static void bio_free_stuff(struct bio *bio)
+{
+ bio_disassociate_task(bio);
+
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, bio->bi_pool);
+}
+
void bio_free(struct bio *bio)
{
struct bio_set *bs = bio->bi_pool;
void *p;
+ bio_free_stuff(bio);
+
if (!bs) {
- /* 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));
+ } else {
+ if (bio_has_allocated_vec(bio))
+ bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
- if (bio_integrity(bio))
- bio_integrity_free(bio, bs);
+ /*
+ * If we have front padding, adjust the bio pointer before freeing
+ */
+ p = bio;
+ if (bs->front_pad)
+ p -= bs->front_pad;
- /*
- * If we have front padding, adjust the bio pointer before freeing
- */
- p = bio;
- if (bs->front_pad)
- p -= bs->front_pad;
+ mempool_free(p, bs->bio_pool);
+ }
+}
- mempool_free(p, bs->bio_pool);
+static void __bio_init(struct bio *bio)
+{
+ memset(bio, 0, BIO_RESET_BYTES);
+ bio->bi_flags = 1 << BIO_UPTODATE;
}
void bio_init(struct bio *bio)
{
- memset(bio, 0, sizeof(*bio));
- bio->bi_flags = 1 << BIO_UPTODATE;
+ __bio_init(bio);
atomic_set(&bio->bi_cnt, 1);
}
EXPORT_SYMBOL(bio_init);
@@ -275,13 +283,10 @@ void bio_reset(struct bio *bio)
{
unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
- if (bio_integrity(bio))
- bio_integrity_free(bio, bio->bi_pool);
+ bio_free_stuff(bio);
+ __bio_init(bio);
- bio_disassociate_task(bio);
-
- memset(bio, 0, BIO_RESET_BYTES);
- bio->bi_flags = flags|(1 << BIO_UPTODATE);
+ bio->bi_flags |= flags;
}
EXPORT_SYMBOL(bio_reset);
@@ -440,10 +445,8 @@ void bio_put(struct bio *bio)
/*
* last put frees it
*/
- if (atomic_dec_and_test(&bio->bi_cnt)) {
- bio_disassociate_task(bio);
+ if (atomic_dec_and_test(&bio->bi_cnt))
bio_free(bio);
- }
}
EXPORT_SYMBOL(bio_put);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index ca63847..28bddc0 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -68,15 +68,6 @@ struct bio {
struct bvec_iter bi_iter;
- /*
- * Everything starting with bi_max_vecs will be preserved by bio_reset()
- */
-
- unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
-
- atomic_t bi_cnt; /* pin count */
-
- struct bio_vec *bi_io_vec; /* the actual vec list */
#ifdef CONFIG_BLK_CGROUP
/*
* Optional ioc and css associated with this bio. Put on bio
@@ -89,6 +80,16 @@ struct bio {
struct bio_integrity_payload *bi_integrity; /* data integrity */
#endif
+ /*
+ * Everything starting with bi_max_vecs will be preserved by bio_reset()
+ */
+
+ unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
+
+ atomic_t bi_cnt; /* pin count */
+
+ struct bio_vec *bi_io_vec; /* the actual vec list */
+
struct bio_set *bi_pool;
/*
next prev parent reply other threads:[~2012-08-28 22:55 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-28 17:37 [PATCH v7 0/9] Block cleanups, deadlock fix Kent Overstreet
2012-08-28 17:37 ` [PATCH v7 1/9] block: Generalized bio pool freeing Kent Overstreet
2012-08-28 17:37 ` [PATCH v7 2/9] dm: Use bioset's front_pad for dm_rq_clone_bio_info Kent Overstreet
2012-08-28 17:37 ` [PATCH v7 3/9] block: Add bio_reset() Kent Overstreet
2012-08-28 20:31 ` Tejun Heo
2012-08-28 22:17 ` Kent Overstreet
2012-08-28 22:53 ` Kent Overstreet [this message]
2012-09-01 2:23 ` Tejun Heo
2012-09-05 20:13 ` Kent Overstreet
2012-08-28 17:37 ` [PATCH v7 4/9] pktcdvd: Switch to bio_kmalloc() Kent Overstreet
2012-08-28 20:32 ` Tejun Heo
2012-08-28 22:24 ` Kent Overstreet
2012-09-04 9:05 ` Jiri Kosina
2012-09-05 19:44 ` Kent Overstreet
2012-08-28 17:37 ` [PATCH v7 5/9] block: Kill bi_destructor Kent Overstreet
2012-08-28 20:36 ` Tejun Heo
2012-08-28 22:07 ` Kent Overstreet
2012-08-28 17:37 ` [PATCH v7 6/9] block: Consolidate bio_alloc_bioset(), bio_kmalloc() Kent Overstreet
2012-08-28 20:41 ` Tejun Heo
2012-08-28 22:03 ` Kent Overstreet
2012-09-01 2:17 ` Tejun Heo
2012-08-28 17:37 ` [PATCH v7 7/9] block: Add bio_clone_bioset(), bio_clone_kmalloc() Kent Overstreet
2012-08-28 20:44 ` Tejun Heo
2012-08-28 22:05 ` Kent Overstreet
2012-09-01 2:19 ` Tejun Heo
2012-08-28 17:37 ` [PATCH v7 8/9] block: Reorder struct bio_set Kent Overstreet
2012-08-28 17:37 ` [PATCH v7 9/9] block: Avoid deadlocks with bio allocation by stacking drivers Kent Overstreet
2012-08-28 20:49 ` Tejun Heo
2012-08-28 22:28 ` Kent Overstreet
2012-08-28 23:01 ` Kent Overstreet
2012-08-29 1:31 ` Vivek Goyal
2012-08-29 3:25 ` Kent Overstreet
2012-08-29 12:57 ` Vivek Goyal
2012-08-29 14:39 ` [dm-devel] " Alasdair G Kergon
2012-08-29 16:26 ` Kent Overstreet
2012-08-29 21:01 ` John Stoffel
2012-08-29 21:08 ` Kent Overstreet
2012-08-28 22:06 ` Vivek Goyal
2012-08-28 22:23 ` Kent Overstreet
2012-08-29 16:24 ` Mikulas Patocka
2012-08-29 16:50 ` Kent Overstreet
2012-08-29 16:57 ` [dm-devel] " Alasdair G Kergon
2012-08-29 17:07 ` Vivek Goyal
2012-08-29 17:13 ` Kent Overstreet
2012-08-29 17:23 ` [dm-devel] " Alasdair G Kergon
2012-08-29 17:32 ` Kent Overstreet
2012-08-30 22:07 ` Vivek Goyal
2012-08-31 1:43 ` Kent Overstreet
2012-08-31 1:55 ` Kent Overstreet
2012-08-31 15:01 ` Vivek Goyal
2012-09-03 1:26 ` Kent Overstreet
2012-09-03 20:41 ` Mikulas Patocka
2012-09-04 3:41 ` Kent Overstreet
2012-09-04 18:55 ` Tejun Heo
2012-09-04 19:01 ` Tejun Heo
2012-09-04 19:43 ` Kent Overstreet
2012-09-04 19:42 ` Kent Overstreet
2012-09-04 21:03 ` Tejun Heo
2012-09-04 19:26 ` Mikulas Patocka
2012-09-04 19:39 ` Vivek Goyal
2012-09-04 19:51 ` [PATCH] dm: Use bioset's front_pad for dm_target_io Kent Overstreet
2012-09-04 21:20 ` Tejun Heo
2012-09-11 19:28 ` [PATCH 2] " Mikulas Patocka
2012-09-11 19:50 ` Kent Overstreet
2012-09-12 22:31 ` Mikulas Patocka
2012-09-14 23:09 ` [dm-devel] " Alasdair G Kergon
2012-09-01 2:13 ` [PATCH v7 9/9] block: Avoid deadlocks with bio allocation by stacking drivers Tejun Heo
2012-09-03 1:34 ` [PATCH v2] " Kent Overstreet
2012-09-04 15:00 ` [PATCH v7 9/9] " Vivek Goyal
2012-09-03 0:49 ` Dave Chinner
2012-09-03 1:17 ` Kent Overstreet
2012-09-04 13:54 ` Vivek Goyal
2012-09-04 18:26 ` Tejun Heo
2012-09-05 3:57 ` Dave Chinner
2012-09-05 4:37 ` Tejun Heo
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=20120828225337.GH1048@moria.home.lan \
--to=koverstreet@google.com \
--cc=axboe@kernel.dk \
--cc=bharrosh@panasas.com \
--cc=dm-devel@redhat.com \
--cc=linux-bcache@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mpatocka@redhat.com \
--cc=tj@kernel.org \
--cc=vgoyal@redhat.com \
/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).