Skip to contentSkip to conversation

kill bio->bi_set

PATCH In review Jens Axboe 2 participants 5 messages Jul 21, 2005 – Jul 26, 2005

Subsystems (3)
VFS Block layer Device mapper
lore
Share & tools
lore

Jens Axboe

author
#0 lore raw

PATCH 1/1 kill bio->bi_set

Hi,

Dunno why I didn't notice before, but ->bi_set is totally unnecessary bloat of struct bio. Just define a proper destructor for the bio and it already knows what bio_set it belongs too.

Signed-off-by Jens Axboe <[email protected]>
drivers/md/dm-io.c +6 -0
@@ -239,6 +239,11 @@ static void vm_dp_init(struct dpages *dp
 	dp->context_ptr = data;
 }
 
+static void dm_bio_destructor(struct bio *bio)
+{
+	bio_free(bio, _bios);
+}
+
 /*-----------------------------------------------------------------
  * IO routines that accept a list of pages.
  *---------------------------------------------------------------*/

@@ -263,6 +268,7 @@ static void do_region(int rw, unsigned i
 		bio->bi_bdev = where->bdev;
 		bio->bi_end_io = endio;
 		bio->bi_private = io;
+		bio->bi_destructor = dm_bio_destructor;
 		bio_set_region(bio, region);
 
 		/*
fs/bio.c +18 -10
@@ -104,18 +104,22 @@ static inline struct bio_vec *bvec_alloc
 	return bvl;
 }
 
-/*
- * default destructor for a bio allocated with bio_alloc_bioset()
- */
-static void bio_destructor(struct bio *bio)
+void bio_free(struct bio *bio, struct bio_set *bio_set)
 {
 	const int pool_idx = BIO_POOL_IDX(bio);
-	struct bio_set *bs = bio->bi_set;
 
 	BIO_BUG_ON(pool_idx >= BIOVEC_NR_POOLS);
 
-	mempool_free(bio->bi_io_vec, bs->bvec_pools[pool_idx]);
-	mempool_free(bio, bs->bio_pool);
+	mempool_free(bio->bi_io_vec, fs_bio_set->bvec_pools[pool_idx]);
+	mempool_free(bio, fs_bio_set->bio_pool);
+}
+
+/*
+ * default destructor for a bio allocated with bio_alloc_bioset()
+ */
+static void bio_fs_destructor(struct bio *bio)
+{
+	bio_free(bio, fs_bio_set);
 }
 
 inline void bio_init(struct bio *bio)

@@ -171,8 +175,6 @@ struct bio *bio_alloc_bioset(unsigned in
 			bio->bi_max_vecs = bvec_slabs[idx].nr_vecs;
 		}
 		bio->bi_io_vec = bvl;
-		bio->bi_destructor = bio_destructor;
-		bio->bi_set = bs;
 	}
 out:
 	return bio;

@@ -180,7 +182,12 @@ out:
 
 struct bio *bio_alloc(unsigned int __nocast gfp_mask, int nr_iovecs)
 {
-	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
+	struct bio *bio = bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
+
+	if (bio)
+		bio->bi_destructor = bio_fs_destructor;
+
+	return bio;
 }
 
 void zero_fill_bio(struct bio *bio)

@@ -1078,6 +1085,7 @@ subsys_initcall(init_bio);
 
 EXPORT_SYMBOL(bio_alloc);
 EXPORT_SYMBOL(bio_put);
+EXPORT_SYMBOL(bio_free);
 EXPORT_SYMBOL(bio_endio);
 EXPORT_SYMBOL(bio_init);
 EXPORT_SYMBOL(__bio_clone);
include/linux/bio.h +1 -1
@@ -111,7 +111,6 @@ struct bio {
 	void			*bi_private;
 
 	bio_destructor_t	*bi_destructor;	/* destructor */
-	struct bio_set		*bi_set;	/* memory pools set */
 };
 
 /*

@@ -280,6 +279,7 @@ extern void bioset_free(struct bio_set *
 extern struct bio *bio_alloc(unsigned int __nocast, int);
 extern struct bio *bio_alloc_bioset(unsigned int __nocast, int, struct bio_set *);
 extern void bio_put(struct bio *);
+extern void bio_free(struct bio *, struct bio_set *);
 
 extern void bio_endio(struct bio *, unsigned int, int);
 struct request_queue;
Signature · 1 line
Jens Axboe
#1 lore raw
Quoted 3 lines · Jens Axboe

Dunno why I didn't notice before, but ->bi_set is totally unnecessary bloat of struct bio. Just define a proper destructor for the bio and it already knows what bio_set it belongs too.

This causes crashes on my computer.

fs/bio.c static inline struct bio_vec *bvec_alloc · line 115

Quoted 12 lines
fs/bio.c +4 -3
+void bio_free(struct bio *bio, struct bio_set *bio_set)
 {
 	const int pool_idx = BIO_POOL_IDX(bio);
-	struct bio_set *bs = bio->bi_set;
 
 	BIO_BUG_ON(pool_idx >= BIOVEC_NR_POOLS);
 
-	mempool_free(bio->bi_io_vec, bs->bvec_pools[pool_idx]);
-	mempool_free(bio, bs->bio_pool);
+	mempool_free(bio->bi_io_vec, fs_bio_set->bvec_pools[pool_idx]);
+	mempool_free(bio, fs_bio_set->bio_pool);
+}

This function uses fs_bio_set instead of the function parameter bio_set.

fs/bio.c struct bio *bio_alloc_bioset(unsigned in · line 178

Quoted 7 lines
fs/bio.c +0 -2
@@ -171,8 +175,6 @@ struct bio *bio_alloc_bioset(unsigned in
 			bio->bi_max_vecs = bvec_slabs[idx].nr_vecs;
 		}
 		bio->bi_io_vec = bvl;
-		bio->bi_destructor = bio_destructor;
-		bio->bi_set = bs;
 	}

This change means that all code that calls bio_alloc_bioset() must now set bi_destructor, but this is forgotten in bio_clone() in bio.c and in split_bvec() in dm.c.

Signature · 2 lines
Peter Osterlund - [email protected]
http://web.telia.com/~u89404340
author
#2 lore raw
Quoted 7 lines · Peter Osterlund
Quoted 3 lines · Jens Axboe

Dunno why I didn't notice before, but ->bi_set is totally unnecessary bloat of struct bio. Just define a proper destructor for the bio and it already knows what bio_set it belongs too.

This causes crashes on my computer.

Did I neglect to mention it was untested? :)

Quoted 7 lines
Quoted 7 lines
fs/bio.c +1 -1
+void bio_free(struct bio *bio, struct bio_set *bio_set)
 {
 	const int pool_idx = BIO_POOL_IDX(bio);
-	struct bio_set *bs = bio->bi_set;
 
 	BIO_BUG_ON(pool_idx >= BIOVEC_NR_POOLS);
Quoted 8 lines

fs/bio.c static inline struct bio_vec *bvec_alloc · line 115

Quoted 5 lines
fs/bio.c +3 -2
-	mempool_free(bio->bi_io_vec, bs->bvec_pools[pool_idx]);
-	mempool_free(bio, bs->bio_pool);
+	mempool_free(bio->bi_io_vec, fs_bio_set->bvec_pools[pool_idx]);
+	mempool_free(bio, fs_bio_set->bio_pool);
+}

This function uses fs_bio_set instead of the function parameter bio_set.

Clear mistake, thanks.

Quoted 9 lines

fs/bio.c struct bio *bio_alloc_bioset(unsigned in · line 178

Quoted 7 lines
fs/bio.c +0 -2
@@ -171,8 +175,6 @@ struct bio *bio_alloc_bioset(unsigned in
 			bio->bi_max_vecs = bvec_slabs[idx].nr_vecs;
 		}
 		bio->bi_io_vec = bvl;
-		bio->bi_destructor = bio_destructor;
-		bio->bi_set = bs;
 	}

This change means that all code that calls bio_alloc_bioset() must now

Correct. The alternative was to require a destructor function passed into bio_alloc_bioset().

Quoted 2 lines

set bi_destructor, but this is forgotten in bio_clone() in bio.c and in split_bvec() in dm.c.

Thanks, I'll go over these and submit a fixed version.

Signature · 1 line
Jens Axboe
#3 lore raw
Quoted 10 lines · Jens Axboe
Quoted 7 lines · Peter Osterlund
Quoted 3 lines · Jens Axboe

Dunno why I didn't notice before, but ->bi_set is totally unnecessary bloat of struct bio. Just define a proper destructor for the bio and it already knows what bio_set it belongs too.

This causes crashes on my computer.

Did I neglect to mention it was untested? :)

...

Quoted 1 line

Thanks, I'll go over these and submit a fixed version.

I fixed this myself. The patch below is tested with dm-crypt on top of pktcdvd on top of usb-storage, and worked fine in my tests.

Signed-off-by Jens Axboe <[email protected]>
Signed-off-by Peter Osterlund <[email protected]>

---

 drivers/md/dm-io.c  |    6 ++++++
 drivers/md/dm.c     |    6 ++++++
 fs/bio.c            |   32 +++++++++++++++++++++-----------
 include/linux/bio.h |    2 +-
 4 files changed, 34 insertions(+), 12 deletions(-)
drivers/md/dm-io.c +6 -0
@@ -239,6 +239,11 @@ static void vm_dp_init(struct dpages *dp
 	dp->context_ptr = data;
 }
 
+static void dm_bio_destructor(struct bio *bio)
+{
+	bio_free(bio, _bios);
+}
+
 /*-----------------------------------------------------------------
  * IO routines that accept a list of pages.
  *---------------------------------------------------------------*/

@@ -263,6 +268,7 @@ static void do_region(int rw, unsigned i
 		bio->bi_bdev = where->bdev;
 		bio->bi_end_io = endio;
 		bio->bi_private = io;
+		bio->bi_destructor = dm_bio_destructor;
 		bio_set_region(bio, region);
 
 		/*
drivers/md/dm.c +6 -0
@@ -399,6 +399,11 @@ struct clone_info {
 	unsigned short idx;
 };
 
+static void dm_bio_destructor(struct bio *bio)
+{
+	bio_free(bio, dm_set);
+}
+
 /*
  * Creates a little bio that is just does part of a bvec.
  */

@@ -410,6 +415,7 @@ static struct bio *split_bvec(struct bio
 	struct bio_vec *bv = bio->bi_io_vec + idx;
 
 	clone = bio_alloc_bioset(GFP_NOIO, 1, dm_set);
+	clone->bi_destructor = dm_bio_destructor;
 	*clone->bi_io_vec = *bv;
 
 	clone->bi_sector = sector;
fs/bio.c +21 -11
@@ -104,18 +104,22 @@ static inline struct bio_vec *bvec_alloc
 	return bvl;
 }
 
-/*
- * default destructor for a bio allocated with bio_alloc_bioset()
- */
-static void bio_destructor(struct bio *bio)
+void bio_free(struct bio *bio, struct bio_set *bio_set)
 {
 	const int pool_idx = BIO_POOL_IDX(bio);
-	struct bio_set *bs = bio->bi_set;
 
 	BIO_BUG_ON(pool_idx >= BIOVEC_NR_POOLS);
 
-	mempool_free(bio->bi_io_vec, bs->bvec_pools[pool_idx]);
-	mempool_free(bio, bs->bio_pool);
+	mempool_free(bio->bi_io_vec, bio_set->bvec_pools[pool_idx]);
+	mempool_free(bio, bio_set->bio_pool);
+}
+
+/*
+ * default destructor for a bio allocated with bio_alloc_bioset()
+ */
+static void bio_fs_destructor(struct bio *bio)
+{
+	bio_free(bio, fs_bio_set);
 }
 
 inline void bio_init(struct bio *bio)

@@ -171,8 +175,6 @@ struct bio *bio_alloc_bioset(unsigned in
 			bio->bi_max_vecs = bvec_slabs[idx].nr_vecs;
 		}
 		bio->bi_io_vec = bvl;
-		bio->bi_destructor = bio_destructor;
-		bio->bi_set = bs;
 	}
 out:
 	return bio;

@@ -180,7 +182,12 @@ out:
 
 struct bio *bio_alloc(unsigned int __nocast gfp_mask, int nr_iovecs)
 {
-	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
+	struct bio *bio = bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
+
+	if (bio)
+		bio->bi_destructor = bio_fs_destructor;
+
+	return bio;
 }
 
 void zero_fill_bio(struct bio *bio)

@@ -276,8 +283,10 @@ struct bio *bio_clone(struct bio *bio, u
 {
 	struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
 
-	if (b)
+	if (b) {
+		b->bi_destructor = bio_fs_destructor;
 		__bio_clone(b, bio);
+	}
 
 	return b;
 }

@@ -1078,6 +1087,7 @@ subsys_initcall(init_bio);
 
 EXPORT_SYMBOL(bio_alloc);
 EXPORT_SYMBOL(bio_put);
+EXPORT_SYMBOL(bio_free);
 EXPORT_SYMBOL(bio_endio);
 EXPORT_SYMBOL(bio_init);
 EXPORT_SYMBOL(__bio_clone);
include/linux/bio.h +1 -1
@@ -111,7 +111,6 @@ struct bio {
 	void			*bi_private;
 
 	bio_destructor_t	*bi_destructor;	/* destructor */
-	struct bio_set		*bi_set;	/* memory pools set */
 };
 
 /*

@@ -280,6 +279,7 @@ extern void bioset_free(struct bio_set *
 extern struct bio *bio_alloc(unsigned int __nocast, int);
 extern struct bio *bio_alloc_bioset(unsigned int __nocast, int, struct bio_set *);
 extern void bio_put(struct bio *);
+extern void bio_free(struct bio *, struct bio_set *);
 
 extern void bio_endio(struct bio *, unsigned int, int);
 struct request_queue;
Signature · 2 lines
Peter Osterlund - [email protected]
http://web.telia.com/~u89404340
author
#4 lore raw
Quoted 9 lines · Peter Osterlund
Quoted 7 lines · Jens Axboe
Quoted 6 lines · Peter Osterlund
Quoted 3 lines · Jens Axboe

Dunno why I didn't notice before, but ->bi_set is totally unnecessary bloat of struct bio. Just define a proper destructor for the bio and it already knows what bio_set it belongs too.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

This causes crashes on my computer.

Did I neglect to mention it was untested? :)

...

Quoted 1 line

Thanks, I'll go over these and submit a fixed version.

I fixed this myself. The patch below is tested with dm-crypt on top of pktcdvd on top of usb-storage, and worked fine in my tests.

Thanks! I was travelling, so I didn't get to it myself.

Signature · 1 line
Jens Axboe