| Message ID | 1344290921-25154-12-git-send-email-koverstreet@google.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On Mon, Aug 06, 2012 at 03:08:40PM -0700, Kent Overstreet wrote: > This consolidates some code, and will help in a later patch changing how > bio cloning works. I think it would be better to introduce bio_clone*() functions in a separate patch and convert the users in a different one. > /** > - * bio_clone - clone a bio > + * bio_clone_bioset - clone a bio > * @bio: bio to clone > * @gfp_mask: allocation priority > + * @bs: bio_set to allocate from > * > * Like __bio_clone, only also allocates the returned bio > */ > -struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > +struct bio *bio_clone_bioset(struct bio *bio, gfp_t gfp_mask, > + struct bio_set *bs) > { > - struct bio *b = bio_alloc(gfp_mask, bio->bi_max_vecs); > + struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs); > > if (!b) > return NULL; > @@ -485,7 +487,7 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > if (bio_integrity(bio)) { > int ret; > > - ret = bio_integrity_clone(b, bio, gfp_mask, fs_bio_set); > + ret = bio_integrity_clone(b, bio, gfp_mask, bs); > > if (ret < 0) { > bio_put(b); > @@ -495,6 +497,12 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > > return b; > } > +EXPORT_SYMBOL(bio_clone_bioset); > + > +struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > +{ > + return bio_clone_bioset(bio, gfp_mask, fs_bio_set); > +} So, bio_clone() loses its function comment. Also, does it even make sense to call bio_clone() from fs_bio_set? Let's say it's so, then what's the difference from using _kmalloc variant? Thanks.
On Wed, Aug 08, 2012 at 04:21:20PM -0700, Tejun Heo wrote: > On Mon, Aug 06, 2012 at 03:08:40PM -0700, Kent Overstreet wrote: > > This consolidates some code, and will help in a later patch changing how > > bio cloning works. > > I think it would be better to introduce bio_clone*() functions in a > separate patch and convert the users in a different one. > > > /** > > - * bio_clone - clone a bio > > + * bio_clone_bioset - clone a bio > > * @bio: bio to clone > > * @gfp_mask: allocation priority > > + * @bs: bio_set to allocate from > > * > > * Like __bio_clone, only also allocates the returned bio > > */ > > -struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > > +struct bio *bio_clone_bioset(struct bio *bio, gfp_t gfp_mask, > > + struct bio_set *bs) > > { > > - struct bio *b = bio_alloc(gfp_mask, bio->bi_max_vecs); > > + struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs); > > > > if (!b) > > return NULL; > > @@ -485,7 +487,7 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > > if (bio_integrity(bio)) { > > int ret; > > > > - ret = bio_integrity_clone(b, bio, gfp_mask, fs_bio_set); > > + ret = bio_integrity_clone(b, bio, gfp_mask, bs); > > > > if (ret < 0) { > > bio_put(b); > > @@ -495,6 +497,12 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > > > > return b; > > } > > +EXPORT_SYMBOL(bio_clone_bioset); > > + > > +struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) > > +{ > > + return bio_clone_bioset(bio, gfp_mask, fs_bio_set); > > +} > > So, bio_clone() loses its function comment. Also, does it even make > sense to call bio_clone() from fs_bio_set? I'll re add the function comment if you want, just for a single line wrapper I don't know if it's worth the cost - comments get out of date, and they're more stuff to wade through. > Let's say it's so, then > what's the difference from using _kmalloc variant? bio_kmalloc() fails if nr_iovecs > 1024, bio_alloc_bioset() fails if nr_iovecs > 256 and bio_alloc_bioset() is mempool backed, bio_kmalloc() is not. AFAICT that's it. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Aug 08, 2012 at 07:56:10PM -0700, Kent Overstreet wrote: > > So, bio_clone() loses its function comment. Also, does it even make > > sense to call bio_clone() from fs_bio_set? > > I'll re add the function comment if you want, just for a single line > wrapper I don't know if it's worth the cost - comments get out of date, > and they're more stuff to wade through. People actually look at docbook generated docs. I don't know why but they do. It's a utility function at block layer. Please just add the comment. > > Let's say it's so, then > > what's the difference from using _kmalloc variant? > > bio_kmalloc() fails if nr_iovecs > 1024, bio_alloc_bioset() fails if > nr_iovecs > 256 > > and bio_alloc_bioset() is mempool backed, bio_kmalloc() is not. > > AFAICT that's it. So, the thing is being mempool backed doesn't mean anything if multiple layers use the pool. I *suspect* fs_bio_set is supposed to be used by fs layer - ie. where bios originate. The reason why I wondered about bio_clone() is that bio_clone() is almost always used from stacking drivers and stacking driver tapping into fs reserve is buggy. So, I'm wondering whether cloning from fs_bio_set should be supported at all. Thanks.
On Wed, Aug 08, 2012 at 11:52:51PM -0700, Tejun Heo wrote: > On Wed, Aug 08, 2012 at 07:56:10PM -0700, Kent Overstreet wrote: > > > So, bio_clone() loses its function comment. Also, does it even make > > > sense to call bio_clone() from fs_bio_set? > > > > I'll re add the function comment if you want, just for a single line > > wrapper I don't know if it's worth the cost - comments get out of date, > > and they're more stuff to wade through. > > People actually look at docbook generated docs. I don't know why but > they do. It's a utility function at block layer. Please just add the > comment. Will do then. > > > Let's say it's so, then > > > what's the difference from using _kmalloc variant? > > > > bio_kmalloc() fails if nr_iovecs > 1024, bio_alloc_bioset() fails if > > nr_iovecs > 256 > > > > and bio_alloc_bioset() is mempool backed, bio_kmalloc() is not. > > > > AFAICT that's it. > > So, the thing is being mempool backed doesn't mean anything if > multiple layers use the pool. It's worse than just using kmalloc, because then you've introduced the possibility of deadlock. > I *suspect* fs_bio_set is supposed to > be used by fs layer - ie. where bios originate. The reason why I > wondered about bio_clone() is that bio_clone() is almost always used > from stacking drivers and stacking driver tapping into fs reserve is > buggy. So, I'm wondering whether cloning from fs_bio_set should be > supported at all. That's actually a really good point. I just grepped and there's actually only 3 callers - I thought there'd be more. That should be easy to fix, at least. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/block/blk-core.c b/block/blk-core.c index e9058c2..10a6e08 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -2768,16 +2768,10 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src, blk_rq_init(NULL, rq); __rq_for_each_bio(bio_src, rq_src) { - bio = bio_alloc_bioset(gfp_mask, bio_src->bi_max_vecs, bs); + bio = bio_clone_bioset(bio_src, gfp_mask, bs); if (!bio) goto free_and_out; - __bio_clone(bio, bio_src); - - if (bio_integrity(bio_src) && - bio_integrity_clone(bio, bio_src, gfp_mask, bs)) - goto free_and_out; - if (bio_ctr && bio_ctr(bio, bio_src, data)) goto free_and_out; diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 4014696..3f3c26e 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1101,8 +1101,8 @@ static void __issue_target_request(struct clone_info *ci, struct dm_target *ti, * ci->bio->bi_max_vecs is BIO_INLINE_VECS anyway, for both flush * and discard, so no need for concern about wasted bvec allocations. */ - clone = bio_alloc_bioset(GFP_NOIO, ci->bio->bi_max_vecs, ci->md->bs); - __bio_clone(clone, ci->bio); + clone = bio_clone_bioset(ci->bio, GFP_NOIO, ci->md->bs); + 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 f9d16dc..069c3bc 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -173,28 +173,10 @@ EXPORT_SYMBOL_GPL(bio_alloc_mddev); struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask, struct mddev *mddev) { - struct bio *b; - if (!mddev || !mddev->bio_set) return bio_clone(bio, gfp_mask); - b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, mddev->bio_set); - if (!b) - return NULL; - - __bio_clone(b, bio); - if (bio_integrity(bio)) { - int ret; - - ret = bio_integrity_clone(b, bio, gfp_mask, mddev->bio_set); - - if (ret < 0) { - bio_put(b); - return NULL; - } - } - - return b; + return bio_clone_bioset(bio, gfp_mask, mddev->bio_set); } EXPORT_SYMBOL_GPL(bio_clone_mddev); diff --git a/fs/bio.c b/fs/bio.c index 77b9313..38ad026 100644 --- a/fs/bio.c +++ b/fs/bio.c @@ -467,15 +467,17 @@ void __bio_clone(struct bio *bio, struct bio *bio_src) EXPORT_SYMBOL(__bio_clone); /** - * bio_clone - clone a bio + * bio_clone_bioset - clone a bio * @bio: bio to clone * @gfp_mask: allocation priority + * @bs: bio_set to allocate from * * Like __bio_clone, only also allocates the returned bio */ -struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) +struct bio *bio_clone_bioset(struct bio *bio, gfp_t gfp_mask, + struct bio_set *bs) { - struct bio *b = bio_alloc(gfp_mask, bio->bi_max_vecs); + struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs); if (!b) return NULL; @@ -485,7 +487,7 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) if (bio_integrity(bio)) { int ret; - ret = bio_integrity_clone(b, bio, gfp_mask, fs_bio_set); + ret = bio_integrity_clone(b, bio, gfp_mask, bs); if (ret < 0) { bio_put(b); @@ -495,6 +497,12 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) return b; } +EXPORT_SYMBOL(bio_clone_bioset); + +struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask) +{ + return bio_clone_bioset(bio, gfp_mask, fs_bio_set); +} EXPORT_SYMBOL(bio_clone); struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask) diff --git a/include/linux/bio.h b/include/linux/bio.h index e180f1d..fb90584 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -220,6 +220,7 @@ struct request_queue; extern int bio_phys_segments(struct request_queue *, struct bio *); extern void __bio_clone(struct bio *, struct bio *); +extern struct bio *bio_clone_bioset(struct bio *, gfp_t, struct bio_set *bs); extern struct bio *bio_clone(struct bio *, gfp_t); struct bio *bio_clone_kmalloc(struct bio *, gfp_t);
This consolidates some code, and will help in a later patch changing how bio cloning works. Signed-off-by: Kent Overstreet <koverstreet@google.com> --- block/blk-core.c | 8 +------- drivers/md/dm.c | 4 ++-- drivers/md/md.c | 20 +------------------- fs/bio.c | 16 ++++++++++++---- include/linux/bio.h | 1 + 5 files changed, 17 insertions(+), 32 deletions(-)