Skip to contentSkip to conversation

device-mapper core should use private bioset

PATCH In review Stefan Bader 3 participants 3 messages Aug 14, 2006 1 message not in the archive

Subsystems (2)
MD RAID Device mapper
lore
Share & tools
lore

Stefan Bader

author
#0 lore raw

PATCH 1/1 device-mapper core should use private bioset

Hi Alasdair,

I found a problem within device-mapper that occurs in low-mem situations. It was found using a mirror target but I think in theory it would hit any setup that stacks device-mapper devices (like LVM on top of multipath).
Since device-mapper core uses the common fs_bioset it is possible that the filesystem and the first level target successfully get bios but the lower level target doesn't because there is no more memory and the pool was drained by upper layers. So the remapping will be stuck forever. To solve this device-mapper core would have to use its own bioset. The following patch is against RHEL4-U4 but for head I guess only the stuff for dm.c (without hunk 1) and probably dm-io.c (if the bioset stuff there hasn't been replaced by the kernel built-in code, yet).

Regards,
Stefan Bader

drivers/md/dm.c +45 -9
diff -Nurp linux-RHEL4-U4/drivers/md/dm.c linux-RHEL4-U4-bioset/drivers/md/dm.c
@@ -6,6 +6,7 @@
 
 #include "dm.h"
 #include "dm-bio-list.h"
+#include "dm-compat-bioset.h"
 
 #include <linux/init.h>
 #include <linux/module.h>

@@ -103,6 +104,11 @@ struct mapped_device {
 	 */
 	struct super_block *frozen_sb;
 	struct block_device *suspended_bdev;
+
+	/*
+	 * Allocate bios from private pool not the global one.
+	 */
+	struct bio_set *bs;
 };
 
 #define MIN_IOS 256

@@ -376,9 +382,13 @@ static int clone_endio(struct bio *bio, 
 			return 1;
 	}
 
-	free_tio(io->md, tio);
 	dec_pending(io, error);
+	/*
+	 * First release bio because destructor needs valid reference to
+	 * target_io to find the correct bio_set.
+	 */
 	bio_put(bio);
+	free_tio(io->md, tio);
 	return r;
 }
 

@@ -429,9 +439,13 @@ static void __map_bio(struct dm_target *
 	else if (r < 0) {
 		/* error the io and bail out */
 		struct dm_io *io = tio->io;
-		free_tio(tio->io->md, tio);
 		dec_pending(io, r);
+		/*
+		 * First release bio because destructor needs valid reference
+		 * to target_io to find the correct bio_set.
+		 */
 		bio_put(clone);
+		free_tio(tio->io->md, tio);
 	}
 }
 

@@ -445,19 +459,29 @@ struct clone_info {
 	unsigned short idx;
 };
 
+void dm_bio_destructor(struct bio *bio)
+{
+	struct target_io *	tio = bio->bi_private;
+
+	BUG_ON(!tio);
+
+	bio_free(bio, tio->io->md->bs);
+}
+
 /*
  * Creates a little bio that is just does part of a bvec.
  */
 static struct bio *split_bvec(struct bio *bio, sector_t sector,
 			      unsigned short idx, unsigned int offset,
-			      unsigned int len)
+			      unsigned int len, struct bio_set *bs)
 {
 	struct bio *clone;
 	struct bio_vec *bv = bio->bi_io_vec + idx;
 
-	clone = bio_alloc(GFP_NOIO, 1);
+	clone = bio_alloc_bioset(GFP_NOIO, 1, bs);
 	*clone->bi_io_vec = *bv;
 
+	clone->bi_destructor = dm_bio_destructor;
 	clone->bi_sector = sector;
 	clone->bi_bdev = bio->bi_bdev;
 	clone->bi_rw = bio->bi_rw;

@@ -474,11 +498,13 @@ static struct bio *split_bvec(struct bio
  */
 static struct bio *clone_bio(struct bio *bio, sector_t sector,
 			     unsigned short idx, unsigned short bv_count,
-			     unsigned int len)
+			     unsigned int len, struct bio_set *bs)
 {
 	struct bio *clone;
 
-	clone = bio_clone(bio, GFP_NOIO);
+	clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
+	__bio_clone(clone, bio);
+	clone->bi_destructor = dm_bio_destructor;
 	clone->bi_sector = sector;
 	clone->bi_idx = idx;
 	clone->bi_vcnt = idx + bv_count;

@@ -509,7 +535,8 @@ static void __clone_and_map(struct clone
 		 * the remaining io with a single clone.
 		 */
 		clone = clone_bio(bio, ci->sector, ci->idx,
-				  bio->bi_vcnt - ci->idx, ci->sector_count);
+				  bio->bi_vcnt - ci->idx, ci->sector_count,
+				  ci->md->bs);
 		__map_bio(ti, clone, tio);
 		ci->sector_count = 0;
 

@@ -532,7 +559,8 @@ static void __clone_and_map(struct clone
 			len += bv_len;
 		}
 
-		clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len);
+		clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len,
+				  ci->md->bs);
 		__map_bio(ti, clone, tio);
 
 		ci->sector += len;

@@ -561,7 +589,8 @@ static void __clone_and_map(struct clone
 			len = (max > remaining) ? remaining : max;
 
 			clone = split_bvec(bio, ci->sector, ci->idx,
-					   bv->bv_offset + offset, len);
+					   bv->bv_offset + offset, len,
+					   ci->md->bs);
 
 			__map_bio(ti, clone, tio);
 

@@ -831,6 +860,10 @@ static struct mapped_device *alloc_dev(u
 	if (!md->tio_pool)
 		goto bad3;
 
+	md->bs = bioset_create(256, 32, 4);
+	if (!md->bs)
+		goto bad_no_bioset;
+
 	md->disk = alloc_disk(1);
 	if (!md->disk)
 		goto bad4;

@@ -857,6 +890,8 @@ static struct mapped_device *alloc_dev(u
 	return md;
 
  bad4:
+	bioset_free(md->bs);
+ bad_no_bioset:
 	mempool_destroy(md->tio_pool);
  bad3:
 	mempool_destroy(md->io_pool);

@@ -880,6 +915,7 @@ static void free_dev(struct mapped_devic
 	}
 	mempool_destroy(md->tio_pool);
 	mempool_destroy(md->io_pool);
+	bioset_free(md->bs);
 	del_gendisk(md->disk);
 	free_minor(minor);
drivers/md/dm-compat-bioset.c +264 -0

264 lines not on this page · raw message

drivers/md/dm-compat-bioset.h +26 -0
diff -Nurp linux-RHEL4-U4/drivers/md/dm-compat-bioset.h linux-RHEL4-U4-bioset/drivers/md/dm-compat-bioset.h
@@ -0,0 +1,26 @@
+/*===========================================================================*/
+/*=========================== COMPAT START ==================================*/
+/* COMPAT: This is only required for older kernels that do not have bioset   */
+/*         functions exported by bio.h.                                      */
+/*===========================================================================*/
+#ifndef __DM_COMPAT_BIOSET_H__
+#define __DM_COMPAT_BIOSET_H__
+
+#include <linux/bio.h>
+#include <linux/mempool.h>
+
+#define BIOVEC_NR_POOLS	6
+
+struct bio_set;
+
+extern struct bio_set *bioset_create(int bio_pool_size, int bvec_pool_size,
+					int scale);
+extern void bioset_free(struct bio_set *bs);
+
+extern struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs,
+					struct bio_set *bs);
+
+extern void bio_free(struct bio *bio, struct bio_set *bio_set);
+
+#endif /* __DM_COMPAT_BIOSET_H__ */
+
drivers/md/dm-io.c +8 -205

252 lines not on this page · raw message

drivers/md/Makefile +1 -1
diff -Nurp linux-RHEL4-U4/drivers/md/Makefile linux-RHEL4-U4-bioset/drivers/md/Makefile
@@ -3,7 +3,7 @@
 #
 
 dm-mod-objs	:= dm.o dm-table.o dm-target.o dm-linear.o dm-stripe.o \
-		   dm-ioctl.o dm-io.o kcopyd.o
+		   dm-ioctl.o dm-io.o kcopyd.o dm-compat-bioset.c
 dm-multipath-objs := dm-hw-handler.o dm-path-selector.o dm-mpath.o
 dm-snapshot-objs := dm-snap.o dm-exception-store.o
 dm-mirror-objs	:= dm-log.o dm-raid1.o
#2 lore raw
In reply to a message not in the archive
Quoted 4 lines · Stefan Bader

I found a problem within device-mapper that occurs in low-mem situations. It was found using a mirror target but I think in theory it would hit any setup that stacks device-mapper devices (like LVM on top of multipath).

Several dm targets suffer from this.

Mempools are not yet used correctly everywhere in device-mapper: they can get shared when devices are stacked, and some targets share them across multiple instances. I made fixing this one of the prerequisites for this patch:
md-dm-reduce-stack-usage-with-stacked-block-devices.patch
which in some cases makes people more likely to hit the problem.

There's been some progress on this recently with (unfinished) dm-crypt patches at:
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/
(dm-crypt-move-io-to-workqueue.patch plus dependencies)

I'll add (the first part of) your patch to the mix as it's another required piece of the jigsaw, thanks.

Alasdair

maintainer
#3 lore raw
In reply to Stefan Bader
Quoted 4 lines · Stefan Bader

I found a problem within device-mapper that occurs in low-mem situations. It was found using a mirror target but I think in theory it would hit any setup that stacks device-mapper devices (like LVM on top of multipath).

Quoted 8 lines

Since device-mapper core uses the common fs_bioset it is possible that the filesystem and the first level target successfully get bios but the lower level target doesn't because there is no more memory and the pool was drained by upper layers. So the remapping will be stuck forever. To solve this device-mapper core would have to use its own bioset. The following patch is against RHEL4-U4 but for head I guess only the stuff for dm.c (without hunk 1) and probably dm-io.c (if the bioset stuff there hasn't been replaced by the kernel built-in code, yet).

Here's a port of this to an upstream kernel: untested - the reordering of the dec_pending looks suspicious - io now referenced after it's freed?

Please send patches against upstream kernels where possible, and remember to add 'Signed-off-by' lines to everything!

Alasdair

From: Stefan Bader <[email protected]>

I found a problem within device-mapper that occurs in low-mem situations. It was found using a mirror target but I think in theory it would hit any setup that stacks device-mapper devices (like LVM on top of multipath).

Since device-mapper core uses the common fs_bioset
[in clone_bio(), and a private, but still global, bio_set in split_bvec()] it is possible that the filesystem and the first level target successfully get bios but the lower level target doesn't because there is no more memory and the pool was drained by upper layers. So the remapping will be stuck forever. To solve this device-mapper core [needs to use a private bio_set for each device].

drivers/md/dm.c +26 -18
Index: linux-2.6.17/drivers/md/dm.c
===================================================================
@@ -102,6 +102,8 @@ struct mapped_device {
 	mempool_t *io_pool;
 	mempool_t *tio_pool;
 
+	struct bio_set *bs;
+
 	/*
 	 * Event handling.
 	 */

@@ -122,16 +124,10 @@ struct mapped_device {
 static kmem_cache_t *_io_cache;
 static kmem_cache_t *_tio_cache;
 
-static struct bio_set *dm_set;
-
 static int __init local_init(void)
 {
 	int r;
 
-	dm_set = bioset_create(16, 16, 4);
-	if (!dm_set)
-		return -ENOMEM;
-
 	/* allocate a slab for the dm_ios */
 	_io_cache = kmem_cache_create("dm_io",
 				      sizeof(struct dm_io), 0, 0, NULL, NULL);

@@ -165,8 +161,6 @@ static void local_exit(void)
 	kmem_cache_destroy(_tio_cache);
 	kmem_cache_destroy(_io_cache);
 
-	bioset_free(dm_set);
-
 	if (unregister_blkdev(_major, _name) < 0)
 		DMERR("devfs_unregister_blkdev failed");
 

@@ -494,9 +488,9 @@ static int clone_endio(struct bio *bio, 
 			return 1;
 	}
 
-	free_tio(io->md, tio);
 	dec_pending(io, error);
 	bio_put(bio);
+	free_tio(io->md, tio);
 	return r;
 }
 

@@ -555,9 +549,9 @@ static void __map_bio(struct dm_target *
 	else if (r < 0) {
 		/* error the io and bail out */
 		struct dm_io *io = tio->io;
-		free_tio(tio->io->md, tio);
 		dec_pending(io, r);
 		bio_put(clone);
+		free_tio(tio->io->md, tio);
 	}
 }
 

@@ -573,7 +567,9 @@ struct clone_info {
 
 static void dm_bio_destructor(struct bio *bio)
 {
-	bio_free(bio, dm_set);
+	struct target_io *tio = bio->bi_private;
+
+	bio_free(bio, tio->io->md->bs);
 }
 
 /*

@@ -581,12 +577,12 @@ static void dm_bio_destructor(struct bio
  */
 static struct bio *split_bvec(struct bio *bio, sector_t sector,
 			      unsigned short idx, unsigned int offset,
-			      unsigned int len)
+			      unsigned int len, struct bio_set *bs)
 {
 	struct bio *clone;
 	struct bio_vec *bv = bio->bi_io_vec + idx;
 
-	clone = bio_alloc_bioset(GFP_NOIO, 1, dm_set);
+	clone = bio_alloc_bioset(GFP_NOIO, 1, bs);
 	clone->bi_destructor = dm_bio_destructor;
 	*clone->bi_io_vec = *bv;
 

@@ -606,11 +602,13 @@ static struct bio *split_bvec(struct bio
  */
 static struct bio *clone_bio(struct bio *bio, sector_t sector,
 			     unsigned short idx, unsigned short bv_count,
-			     unsigned int len)
+			     unsigned int len, struct bio_set *bs)
 {
 	struct bio *clone;
 
-	clone = bio_clone(bio, GFP_NOIO);
+	clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
+	__bio_clone(clone, bio);
+	clone->bi_destructor = dm_bio_destructor;
 	clone->bi_sector = sector;
 	clone->bi_idx = idx;
 	clone->bi_vcnt = idx + bv_count;

@@ -641,7 +639,8 @@ static void __clone_and_map(struct clone
 		 * the remaining io with a single clone.
 		 */
 		clone = clone_bio(bio, ci->sector, ci->idx,
-				  bio->bi_vcnt - ci->idx, ci->sector_count);
+				  bio->bi_vcnt - ci->idx, ci->sector_count,
+				  ci->md->bs);
 		__map_bio(ti, clone, tio);
 		ci->sector_count = 0;
 

@@ -664,7 +663,8 @@ static void __clone_and_map(struct clone
 			len += bv_len;
 		}
 
-		clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len);
+		clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len,
+				  ci->md->bs);
 		__map_bio(ti, clone, tio);
 
 		ci->sector += len;

@@ -693,7 +693,8 @@ static void __clone_and_map(struct clone
 			len = min(remaining, max);
 
 			clone = split_bvec(bio, ci->sector, ci->idx,
-					   bv->bv_offset + offset, len);
+					   bv->bv_offset + offset, len,
+					   ci->md->bs);
 
 			__map_bio(ti, clone, tio);
 

@@ -961,6 +962,10 @@ static struct mapped_device *alloc_dev(i
 	if (!md->tio_pool)
 		goto bad3;
 
+	md->bs = bioset_create(16, 16, 4);
+	if (!md->bs)
+		goto bad_no_bioset;
+
 	md->disk = alloc_disk(1);
 	if (!md->disk)
 		goto bad4;

@@ -988,6 +993,8 @@ static struct mapped_device *alloc_dev(i
 	return md;
 
  bad4:
+	bioset_free(md->bs);
+ bad_no_bioset:
 	mempool_destroy(md->tio_pool);
  bad3:
 	mempool_destroy(md->io_pool);

@@ -1012,6 +1019,7 @@ static void free_dev(struct mapped_devic
 	}
 	mempool_destroy(md->tio_pool);
 	mempool_destroy(md->io_pool);
+	bioset_free(md->bs);
 	del_gendisk(md->disk);
 	free_minor(minor);