Thread (7 messages) 7 messages, 3 authors, 2018-06-16

Re: why does __split_and_process_bio use bio_clone_bioset?

From: Mike Snitzer <hidden>
Date: 2018-06-14 20:08:19
Also in: dm-devel
Subsystem: device-mapper (lvm), the rest · Maintainers: Alasdair Kergon, Mike Snitzer, Mikulas Patocka, Benjamin Marzinski, Linus Torvalds

On Thu, Jun 14 2018 at  2:12P -0400,
Mike Snitzer [off-list ref] wrote:
On Thu, Jun 14 2018 at  4:19am -0400,
Christoph Hellwig [off-list ref] wrote:
quoted
Hi Neil,

In commit 18a25da8 ("dm: ensure bio submission follows a depth-first
tree walk") you've added a call to bio_clone_bioset to
__split_and_process_bio.  Unlike all other bio splitting code this
actually allocates a new bio_vec array instead of just splitting the bio
and the iterator.  I can't actually find a good reason for that either
in a cursory review of the code, the commit or the comments.

Do you remember why this can't just use bio_clone_fast?
Your question caused me to revisit this code and it is suspect for a
couple reasons:

1) I'm also not seeing why we need bio_clone_bioset()
The patch below seems to work fine (given quick testing).. It also has a
side-effect of not breaking integrity support (which commit 18a25da8
appears to do because it isn't accounting for any of the integrity stuff
bio_split, or dm.c:clone_bio, does).

FYI, my other concerns in my my previous reply were unfounded and due to
misreading the existing code.

Neil, please still feel free to have a look at this to see if you can
recall why you used bio_clone_bioset().

If in the end you agree that the following patch is fine please let me
know and I'll get a proper fix staged.

Thanks,
Mike
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 20a8d63..dfb4783 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1582,10 +1582,9 @@ static blk_qc_t __split_and_process_bio(struct mapped_device *md,
 				 * the usage of io->orig_bio in dm_remap_zone_report()
 				 * won't be affected by this reassignment.
 				 */
-				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
-								 &md->queue->bio_split);
+				struct bio *b = bio_split(bio, bio_sectors(bio) - ci.sector_count,
+							  GFP_NOIO, &md->queue->bio_split);
 				ci.io->orig_bio = b;
-				bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9);
 				bio_chain(b, bio);
 				ret = generic_make_request(bio);
 				break;
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help