device-mapper patchset

PATCH In review Joe Thornber 5 participants 17 messages Feb 20, 2004 – Feb 24, 2004

Subsystems (2)

MD RAID Device mapper

lore

Share & tools

lore

Joe Thornber

author

2004-02-20 15:28 UTC #0 lore raw

Hi,

Here's another device mapper update, some of these are quite big patches, so I'll run through the list:

endio method
We've been using this code for many months (years?). Needed for the more complicated targets.

Remove the version-1 ioctl interface
This didn't get in last time I submitted it. Leave it out if you still disagree.

Audit for list_for_each_*entry*
Trivial, please merge

Queue limits
Please merge.

List targets ioctl
Adds a command that lets tools query the kernel to see what targets/versions are available.

Multipath target
People really want this, so I'm probably pushing it sooner than I'd like. It would be good if it got a wider audience in the -mm tree or as an experimental target in vanilla.

Thanks,

- Joe

Joe Thornber

author

2004-02-20 15:31 UTC #1 lore raw

PATCH 1/6 dm: endio method

Add an endio method to targets. This method is allowed to request another shot at failed ios (think multipath). Context can be passed between the map method and the endio method.

drivers/md/dm-crypt.c +2 -1

@@ -601,7 +601,8 @@ crypt_clone(struct crypt_config *cc, str
 	return clone;
 }
 
-static int crypt_map(struct dm_target *ti, struct bio *bio)
+static int crypt_map(struct dm_target *ti, struct bio *bio,
+		     union map_info *map_context)
 {
 	struct crypt_config *cc = (struct crypt_config *) ti->private;
 	struct crypt_io *io = mempool_alloc(cc->io_pool, GFP_NOIO);

drivers/md/dm-linear.c +2 -1

@@ -65,7 +65,8 @@ static void linear_dtr(struct dm_target 
 	kfree(lc);
 }
 
-static int linear_map(struct dm_target *ti, struct bio *bio)
+static int linear_map(struct dm_target *ti, struct bio *bio,
+		      union map_info *map_context)
 {
 	struct linear_c *lc = (struct linear_c *) ti->private;

drivers/md/dm-stripe.c +2 -1

@@ -166,7 +166,8 @@ static void stripe_dtr(struct dm_target 
 	kfree(sc);
 }
 
-static int stripe_map(struct dm_target *ti, struct bio *bio)
+static int stripe_map(struct dm_target *ti, struct bio *bio,
+		      union map_info *map_context)
 {
 	struct stripe_c *sc = (struct stripe_c *) ti->private;

drivers/md/dm-target.c +2 -1

@@ -157,7 +157,8 @@ static void io_err_dtr(struct dm_target 
 	/* empty */
 }
 
-static int io_err_map(struct dm_target *ti, struct bio *bio)
+static int io_err_map(struct dm_target *ti, struct bio *bio,
+		      union map_info *map_context)
 {
 	return -EIO;
 }

drivers/md/dm.c +122 ~~-26~~

326 lines not on this page · raw message

include/linux/device-mapper.h +19 -1

@@ -13,6 +13,11 @@ struct dm_dev;
 
 typedef enum { STATUSTYPE_INFO, STATUSTYPE_TABLE } status_type_t;
 
+union map_info {
+	void *ptr;
+	unsigned long long ll;
+};
+
 /*
  * In the constructor the target parameter will already have the
  * table, type, begin and len fields filled in.

@@ -32,7 +37,19 @@ typedef void (*dm_dtr_fn) (struct dm_tar
  * = 0: The target will handle the io by resubmitting it later
  * > 0: simple remap complete
  */
-typedef int (*dm_map_fn) (struct dm_target *ti, struct bio *bio);
+typedef int (*dm_map_fn) (struct dm_target *ti, struct bio *bio,
+			  union map_info *map_context);
+
+/*
+ * Returns:
+ * < 0 : error (currently ignored)
+ * 0   : ended successfully
+ * 1   : for some reason the io has still not completed (eg,
+ *       multipath target might want to requeue a failed io).
+ */
+typedef int (*dm_endio_fn) (struct dm_target *ti,
+			    struct bio *bio, int error,
+			    union map_info *map_context);
 
 typedef void (*dm_suspend_fn) (struct dm_target *ti);
 typedef void (*dm_resume_fn) (struct dm_target *ti);

@@ -60,6 +77,7 @@ struct target_type {
 	dm_ctr_fn ctr;
 	dm_dtr_fn dtr;
 	dm_map_fn map;
+	dm_endio_fn end_io;
 	dm_suspend_fn suspend;
 	dm_resume_fn resume;
 	dm_status_fn status;

Joe Thornber

author

2004-02-20 15:37 UTC #8 lore raw

PATCH 2/6 dm: remove v1 ioctl interface

In reply to Joe Thornber

Remove the version-1 ioctl interface.

drivers/md/Kconfig +0 -8

@@ -162,14 +162,6 @@ config BLK_DEV_DM
 
 	  If unsure, say N.
 
-config DM_IOCTL_V4
-	bool "ioctl interface version 4"
-	depends on BLK_DEV_DM
-	default y
-	---help---
-	  Recent tools use a new version of the ioctl interface, only
-          select this option if you intend using such tools.
-
 config DM_CRYPT
 	tristate "Crypt target support"
 	depends on BLK_DEV_DM && EXPERIMENTAL

drivers/md/dm-ioctl.c +1257 -6

1,270 lines not on this page · raw message

include/linux/dm-ioctl.h +229 ~~-10~~

247 lines not on this page · raw message

drivers/md/dm-ioctl-v1.c +0 ~~-1159~~

1,159 lines not on this page · raw message

drivers/md/dm-ioctl-v4.c +0 ~~-1264~~

310 lines not on this page · raw message

include/linux/dm-ioctl-v1.h +0 ~~-149~~

149 lines not on this page · raw message

include/linux/dm-ioctl-v4.h +0 ~~-237~~

237 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Part of the diff is not shown here. View the raw message.

Joe Thornber

author

2004-02-20 15:37 UTC #11 lore raw

PATCH 3/6 dm: list_for_each_entry audit

In reply to Joe Thornber

Audit for list_for_each_*entry*

drivers/md/dm-ioctl.c +5 ~~-12~~

@@ -88,30 +88,24 @@ static unsigned int hash_str(const char 
  *---------------------------------------------------------------*/
 static struct hash_cell *__get_name_cell(const char *str)
 {
-	struct list_head *tmp;
 	struct hash_cell *hc;
 	unsigned int h = hash_str(str);
 
-	list_for_each (tmp, _name_buckets + h) {
-		hc = list_entry(tmp, struct hash_cell, name_list);
+	list_for_each_entry (hc, _name_buckets + h, name_list)
 		if (!strcmp(hc->name, str))
 			return hc;
-	}
 
 	return NULL;
 }
 
 static struct hash_cell *__get_uuid_cell(const char *str)
 {
-	struct list_head *tmp;
 	struct hash_cell *hc;
 	unsigned int h = hash_str(str);
 
-	list_for_each (tmp, _uuid_buckets + h) {
-		hc = list_entry(tmp, struct hash_cell, uuid_list);
+	list_for_each_entry (hc, _uuid_buckets + h, uuid_list)
 		if (!strcmp(hc->uuid, str))
 			return hc;
-	}
 
 	return NULL;
 }

@@ -935,6 +929,7 @@ static void retrieve_deps(struct dm_tabl
 	unsigned int count = 0;
 	struct list_head *tmp;
 	size_t len, needed;
+	struct dm_dev *dd;
 	struct dm_target_deps *deps;
 
 	deps = get_result_buffer(param, param_size, &len);

@@ -942,7 +937,7 @@ static void retrieve_deps(struct dm_tabl
 	/*
 	 * Count the devices.
 	 */
-	list_for_each(tmp, dm_table_get_devices(table))
+	list_for_each (tmp, dm_table_get_devices(table))
 		count++;
 
 	/*

@@ -959,10 +954,8 @@ static void retrieve_deps(struct dm_tabl
 	 */
 	deps->count = count;
 	count = 0;
-	list_for_each(tmp, dm_table_get_devices(table)) {
-		struct dm_dev *dd = list_entry(tmp, struct dm_dev, list);
+	list_for_each_entry (dd, dm_table_get_devices(table), list)
 		deps->dev[count++] = huge_encode_dev(dd->bdev->bd_dev);
-	}
 
 	param->data_size = param->data_start + needed;
 }

drivers/md/dm-table.c +2 -4

@@ -329,13 +329,11 @@ static int lookup_device(const char *pat
  */
 static struct dm_dev *find_device(struct list_head *l, dev_t dev)
 {
-	struct list_head *tmp;
+	struct dm_dev *dd;
 
-	list_for_each(tmp, l) {
-		struct dm_dev *dd = list_entry(tmp, struct dm_dev, list);
+	list_for_each_entry (dd, l, list)
 		if (dd->bdev->bd_dev == dev)
 			return dd;
-	}
 
 	return NULL;
 }

drivers/md/dm-target.c +1 -5

@@ -25,15 +25,11 @@ static DECLARE_RWSEM(_lock);
 
 static inline struct tt_internal *__find_target_type(const char *name)
 {
-	struct list_head *tih;
 	struct tt_internal *ti;
 
-	list_for_each(tih, &_targets) {
-		ti = list_entry(tih, struct tt_internal, list);
-
+	list_for_each_entry (ti, &_targets, list)
 		if (!strcmp(name, ti->tt.name))
 			return ti;
-	}
 
 	return NULL;
 }

Joe Thornber

author

2004-02-20 15:39 UTC #14 lore raw

PATCH 5/6 dm: list targets cmd

In reply to Joe Thornber

List targets ioctl. [Patrick Caulfield]

drivers/md/dm-crypt.c +1 -0

@@ -720,6 +720,7 @@ static int crypt_status(struct dm_target
 
 static struct target_type crypt_target = {
 	.name   = "crypt",
+	.version= {1, 0, 0},
 	.module = THIS_MODULE,
 	.ctr    = crypt_ctr,
 	.dtr    = crypt_dtr,

drivers/md/dm-ioctl.c +88 -2

114 lines not on this page · raw message

drivers/md/dm-linear.c +2 -1

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2001 Sistina Software (UK) Limited.
+ * Copyright (C) 2001-2003 Sistina Software (UK) Limited.
  *
  * This file is released under the GPL.
  */

@@ -97,6 +97,7 @@ static int linear_status(struct dm_targe
 
 static struct target_type linear_target = {
 	.name   = "linear",
+	.version= {1, 0, 1},
 	.module = THIS_MODULE,
 	.ctr    = linear_ctr,
 	.dtr    = linear_dtr,

drivers/md/dm-stripe.c +2 -1

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2001 Sistina Software (UK) Limited.
+ * Copyright (C) 2001-2003 Sistina Software (UK) Limited.
  *
  * This file is released under the GPL.
  */

@@ -212,6 +212,7 @@ static int stripe_status(struct dm_targe
 
 static struct target_type stripe_target = {
 	.name   = "striped",
+	.version= {1, 0, 1},
 	.module = THIS_MODULE,
 	.ctr    = stripe_ctr,
 	.dtr    = stripe_dtr,

drivers/md/dm-target.c +15 -0

@@ -96,6 +96,20 @@ static struct tt_internal *alloc_target(
 	return ti;
 }
 
+
+int dm_target_iterate(void (*iter_func)(struct target_type *tt,
+					void *param), void *param)
+{
+	struct tt_internal *ti;
+
+	down_read(&_lock);
+	list_for_each_entry (ti, &_targets, list)
+		iter_func(&ti->tt, param);
+	up_read(&_lock);
+
+	return 0;
+}
+
 int dm_register_target(struct target_type *t)
 {
 	int rv = 0;

@@ -161,6 +175,7 @@ static int io_err_map(struct dm_target *
 
 static struct target_type error_target = {
 	.name = "error",
+	.version = {1, 0, 1},
 	.ctr  = io_err_ctr,
 	.dtr  = io_err_dtr,
 	.map  = io_err_map,

drivers/md/dm.h +2 -0

@@ -123,6 +123,8 @@ int dm_target_init(void);
 void dm_target_exit(void);
 struct target_type *dm_get_target_type(const char *name);
 void dm_put_target_type(struct target_type *t);
+int dm_target_iterate(void (*iter_func)(struct target_type *tt,
+					void *param), void *param);
 
 
 /*-----------------------------------------------------------------

include/linux/device-mapper.h +2 -1

@@ -74,6 +74,7 @@ void dm_put_device(struct dm_target *ti,
 struct target_type {
 	const char *name;
 	struct module *module;
+        unsigned version[3];
 	dm_ctr_fn ctr;
 	dm_dtr_fn dtr;
 	dm_map_fn map;

@@ -104,7 +105,7 @@ struct dm_target {
 	sector_t split_io;
 
 	/*
-	 * These are automaticall filled in by
+	 * These are automatically filled in by
 	 * dm_table_get_device.
 	 */
 	struct io_restrictions limits;

include/linux/dm-ioctl.h +17 -2

@@ -163,6 +163,16 @@ struct dm_name_list {
 };
 
 /*
+ * Used to retrieve the target versions
+ */
+struct dm_target_versions {
+        uint32_t next;
+        uint32_t version[3];
+
+        char name[0];
+};
+
+/*
  * If you change this make sure you make the corresponding change
  * to dm-ioctl.c:lookup_ioctl()
  */

@@ -185,6 +195,9 @@ enum {
 	DM_TABLE_CLEAR_CMD,
 	DM_TABLE_DEPS_CMD,
 	DM_TABLE_STATUS_CMD,
+
+	/* Added later */
+	DM_LIST_VERSIONS_CMD,
 };
 
 #define DM_IOCTL 0xfd

@@ -205,10 +218,12 @@ enum {
 #define DM_TABLE_DEPS    _IOWR(DM_IOCTL, DM_TABLE_DEPS_CMD, struct dm_ioctl)
 #define DM_TABLE_STATUS  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, struct dm_ioctl)
 
+#define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
+
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	0
+#define DM_VERSION_MINOR	1
 #define DM_VERSION_PATCHLEVEL	0
-#define DM_VERSION_EXTRA	"-ioctl (2003-06-04)"
+#define DM_VERSION_EXTRA	"-ioctl (2003-12-10)"
 
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */

Joe Thornber

author

2004-02-20 15:39 UTC #12 lore raw

PATCH 4/6 dm: default queue limits

In reply to Joe Thornber

Audit for list_for_each_*entry*

drivers/md/dm-ioctl.c +5 ~~-12~~

@@ -88,30 +88,24 @@ static unsigned int hash_str(const char 
  *---------------------------------------------------------------*/
 static struct hash_cell *__get_name_cell(const char *str)
 {
-	struct list_head *tmp;
 	struct hash_cell *hc;
 	unsigned int h = hash_str(str);
 
-	list_for_each (tmp, _name_buckets + h) {
-		hc = list_entry(tmp, struct hash_cell, name_list);
+	list_for_each_entry (hc, _name_buckets + h, name_list)
 		if (!strcmp(hc->name, str))
 			return hc;
-	}
 
 	return NULL;
 }
 
 static struct hash_cell *__get_uuid_cell(const char *str)
 {
-	struct list_head *tmp;
 	struct hash_cell *hc;
 	unsigned int h = hash_str(str);
 
-	list_for_each (tmp, _uuid_buckets + h) {
-		hc = list_entry(tmp, struct hash_cell, uuid_list);
+	list_for_each_entry (hc, _uuid_buckets + h, uuid_list)
 		if (!strcmp(hc->uuid, str))
 			return hc;
-	}
 
 	return NULL;
 }

@@ -935,6 +929,7 @@ static void retrieve_deps(struct dm_tabl
 	unsigned int count = 0;
 	struct list_head *tmp;
 	size_t len, needed;
+	struct dm_dev *dd;
 	struct dm_target_deps *deps;
 
 	deps = get_result_buffer(param, param_size, &len);

@@ -942,7 +937,7 @@ static void retrieve_deps(struct dm_tabl
 	/*
 	 * Count the devices.
 	 */
-	list_for_each(tmp, dm_table_get_devices(table))
+	list_for_each (tmp, dm_table_get_devices(table))
 		count++;
 
 	/*

@@ -959,10 +954,8 @@ static void retrieve_deps(struct dm_tabl
 	 */
 	deps->count = count;
 	count = 0;
-	list_for_each(tmp, dm_table_get_devices(table)) {
-		struct dm_dev *dd = list_entry(tmp, struct dm_dev, list);
+	list_for_each_entry (dd, dm_table_get_devices(table), list)
 		deps->dev[count++] = huge_encode_dev(dd->bdev->bd_dev);
-	}
 
 	param->data_size = param->data_start + needed;
 }

drivers/md/dm-table.c +2 -4

@@ -329,13 +329,11 @@ static int lookup_device(const char *pat
  */
 static struct dm_dev *find_device(struct list_head *l, dev_t dev)
 {
-	struct list_head *tmp;
+	struct dm_dev *dd;
 
-	list_for_each(tmp, l) {
-		struct dm_dev *dd = list_entry(tmp, struct dm_dev, list);
+	list_for_each_entry (dd, l, list)
 		if (dd->bdev->bd_dev == dev)
 			return dd;
-	}
 
 	return NULL;
 }

drivers/md/dm-target.c +1 -5

@@ -25,15 +25,11 @@ static DECLARE_RWSEM(_lock);
 
 static inline struct tt_internal *__find_target_type(const char *name)
 {
-	struct list_head *tih;
 	struct tt_internal *ti;
 
-	list_for_each(tih, &_targets) {
-		ti = list_entry(tih, struct tt_internal, list);
-
+	list_for_each_entry (ti, &_targets, list)
 		if (!strcmp(name, ti->tt.name))
 			return ti;
-	}
 
 	return NULL;
 }

Joe Thornber

author

2004-02-20 15:42 UTC #13 lore raw

Re: [Patch 4/6] dm: default queue limits

Fill in missing queue limitations when table is complete instead of enforcing the "default" limits on every dm device. Problem noticed by Mike Christie.

[Christophe Saout]

drivers/md/dm-table.c +15 -8

@@ -629,14 +629,20 @@ static int split_args(int *argc, char **
 	return 0;
 }
 
-static void set_default_limits(struct io_restrictions *rs)
+static void check_for_valid_limits(struct io_restrictions *rs)
 {
-	rs->max_sectors = MAX_SECTORS;
-	rs->max_phys_segments = MAX_PHYS_SEGMENTS;
-	rs->max_hw_segments = MAX_HW_SEGMENTS;
-	rs->hardsect_size = 1 << SECTOR_SHIFT;
-	rs->max_segment_size = MAX_SEGMENT_SIZE;
-	rs->seg_boundary_mask = -1;
+	if (!rs->max_sectors)
+		rs->max_sectors = MAX_SECTORS;
+	if (!rs->max_phys_segments)
+		rs->max_phys_segments = MAX_PHYS_SEGMENTS;
+	if (!rs->max_hw_segments)
+		rs->max_hw_segments = MAX_HW_SEGMENTS;
+	if (!rs->hardsect_size)
+		rs->hardsect_size = 1 << SECTOR_SHIFT;
+	if (!rs->max_segment_size)
+		rs->max_segment_size = MAX_SEGMENT_SIZE;
+	if (!rs->seg_boundary_mask)
+		rs->seg_boundary_mask = -1;
 }
 
 int dm_table_add_target(struct dm_table *t, const char *type,

@@ -651,7 +657,6 @@ int dm_table_add_target(struct dm_table 
 
 	tgt = t->targets + t->num_targets;
 	memset(tgt, 0, sizeof(*tgt));
-	set_default_limits(&tgt->limits);
 
 	if (!len) {
 		tgt->error = "zero-length target";

@@ -736,6 +741,8 @@ int dm_table_complete(struct dm_table *t
 	int r = 0;
 	unsigned int leaf_nodes;
 
+	check_for_valid_limits(&t->limits);
+
 	/* how many indexes will the btree have ? */
 	leaf_nodes = dm_div_up(t->num_targets, KEYS_PER_NODE);
 	t->depth = 1 + int_log(leaf_nodes, CHILDREN_PER_NODE);

Joe Thornber

author

2004-02-20 15:42 UTC #16 lore raw

PATCH 6/6 dm: multipath target

In reply to Joe Thornber

Multipath target

drivers/md/Kconfig +6 -1

@@ -180,5 +180,10 @@ config DM_CRYPT
 
 	  If unsure, say N.
 
-endmenu
+config DM_MULTIPATH
+       tristate "Multipath target (EXPERIMENTAL)"
+       depends on BLK_DEV_DM && EXPERIMENTAL
+       ---help---
+         Allow volume managers to support multipath hardware.
 
+endmenu

drivers/md/Makefile +2 -0

@@ -4,6 +4,7 @@
 
 dm-mod-objs	:= dm.o dm-table.o dm-target.o dm-linear.o dm-stripe.o \
 		   dm-ioctl.o
+dm-multipath-objs := dm-path-selector.o dm-mpath.o
 raid6-objs	:= raid6main.o raid6algos.o raid6recov.o raid6tables.o \
 		   raid6int1.o raid6int2.o raid6int4.o \
 		   raid6int8.o raid6int16.o raid6int32.o \

@@ -24,6 +25,7 @@ obj-$(CONFIG_MD_MULTIPATH)	+= multipath.
 obj-$(CONFIG_BLK_DEV_MD)	+= md.o
 obj-$(CONFIG_BLK_DEV_DM)	+= dm-mod.o
 obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
+obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o
 
 quiet_cmd_unroll = UNROLL  $@
       cmd_unroll = $(PERL) $(srctree)/$(src)/unroll.pl $(UNROLL) \

drivers/md/dm-mpath.c +729 -0

729 lines not on this page · raw message

drivers/md/dm-path-selector.c +298 -0

298 lines not on this page · raw message

drivers/md/dm-path-selector.h +103 -0

103 lines not on this page · raw message

Andrew Morton

2004-02-21 06:15 UTC #10 lore raw

Re: [Patch 2/6] dm: remove v1 ioctl interface

In reply to Joe Thornber

Quoted 2 lines · Joe Thornber

Remove the version-1 ioctl interface.

This breaks the build on 64-bit machines.

 include/linux/compat_ioctl.h |    7 -------
 1 files changed, 7 deletions(-)

include/linux/compat_ioctl.h renamed from include/linux/compat_ioctl.h~dm-02-compat_ioctl-fix +0 -7

diff -puN include/linux/compat_ioctl.h~dm-02-compat_ioctl-fix include/linux/compat_ioctl.h
@@ -140,13 +140,6 @@ COMPATIBLE_IOCTL(DM_VERSION)
 COMPATIBLE_IOCTL(DM_REMOVE_ALL)
 COMPATIBLE_IOCTL(DM_DEV_CREATE)
 COMPATIBLE_IOCTL(DM_DEV_REMOVE)
-COMPATIBLE_IOCTL(DM_DEV_RELOAD)
-COMPATIBLE_IOCTL(DM_DEV_SUSPEND)
-COMPATIBLE_IOCTL(DM_DEV_RENAME)
-COMPATIBLE_IOCTL(DM_DEV_DEPS)
-COMPATIBLE_IOCTL(DM_DEV_STATUS)
-COMPATIBLE_IOCTL(DM_TARGET_STATUS)
-COMPATIBLE_IOCTL(DM_TARGET_WAIT)
 #endif
 /* Big K */
 COMPATIBLE_IOCTL(PIO_FONT)

Andrew Morton

2004-02-21 06:16 UTC #15 lore raw

Re: [Patch 5/6] dm: list targets cmd

In reply to Joe Thornber

Quoted 6 lines · Joe Thornber

List targets ioctl. [Patrick Caulfield]
...
+#define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)

Does this not need 64-bit emulation support?

Has it been tested on a 64-bit system?

Mike Christie

2004-02-21 10:30 UTC #2 lore raw

Re: [Patch 1/6] dm: endio method

In reply to Joe Thornber

Quoted 11 lines · Joe Thornber

Add an endio method to targets. This method is allowed to request another shot at failed ios (think multipath).
drivers/md/dm.c +7 -0
+	if (endio) {
+		/* Restore bio fields. */
+		bio->bi_sector = tio->bi_sector;
+		bio->bi_bdev = tio->bi_bdev;
+		bio->bi_size = tio->bi_size;
+		bio->bi_idx = tio->bi_idx;
+

drivers/md/dm.c static inline void dec_pending(struct dm

Quoted 10 lines

drivers/md/dm.c +8 -0

+		r = endio(tio->ti, bio, error, &tio->info);
 
 
+	r = ti->type->map(ti, clone, &tio->info);
+	if (r > 0) {
+		/* Save the bio info so we can restore it during endio. */
+		tio->bi_sector = clone->bi_sector;
+		tio->bi_bdev = clone->bi_bdev;
+		tio->bi_size = clone->bi_size;
+		tio->bi_idx = clone->bi_idx;

Saving and restoring bi_bdev is going to break multipath. When a bio is remapped and resent multiple times by the target becuase of multiple path failures, restoring bi_bdev to the original value will cause only that path to be marked as failed instead of the paths that the bio was remapped to.

This is DM's cloned bio. Is there a guarantee that this value should be safe from lower level drivers overwriting it, or is it similar to b_rdev for buffer_heads?

Mike Christie

Christophe Saout

2004-02-21 10:44 UTC #3 lore raw

Re: [Patch 1/6] dm: endio method

Am Sa, den 21.02.2004 schrieb Mike Christie um 10:58:

Quoted 3 lines

This is DM's cloned bio. Is there a guarantee that this value should be safe from lower level drivers overwriting it, or is it similar to b_rdev for buffer_heads?

The block layer is allowed to remap the device. This is done for partitions, e.g. hda5 -> hda + sector number change (blk_partition_remap).

Joe Thornber

author

2004-02-23 10:05 UTC #4 lore raw

Re: [Patch 1/6] dm: endio method

In reply to Mike Christie

Mike,

Quoted 1 line · Mike Christie

Saving and restoring bi_bdev is going to break multipath.

Yes, we'll have to fall back to plan A and use the map_context pointer to hold the path being used (attached patch for illustration only). I had been hoping we could keep the map_context unused so that we could allow the path selectors to use it. I should have spotted this.

I'll also move the failed bio remap back to mpath_end_io(), so that the context can be reused there (it moved to the daemon when we were trying to do path testing in the kernel).

- Joe

drivers/md/dm-mpath.c +4 ~~-20~~

@@ -219,7 +219,7 @@ static struct path *get_current_path(str
 	return path;
 }
 
-static int map_io(struct multipath *m, struct bio *bio)
+static int map_io(struct multipath *m, struct bio *bio, union map_info *map_context)
 {
 	struct path *path;

@@ -228,6 +228,7 @@ static int map_io(struct multipath *m, s
 		return -EIO;
 
 	bio->bi_bdev = path->dev->bdev;
+	map_context->ptr = path;
 	return 0;
 }

@@ -517,29 +518,13 @@ static int multipath_map(struct dm_targe
 	struct multipath *m = (struct multipath *) ti->private;
 
 	bio->bi_rw |= (1 << BIO_RW_FAILFAST);
-	r = map_io(m, bio);
+	r = map_io(m, bio, map_context);
 	if (r)
 		return r;
 
 	return 1;
 }
 
-/*
- * Only called on the error path.
- */
-static struct path *find_path(struct multipath *m, struct block_device *bdev)
-{
-	struct path *p;
-	struct priority_group *pg;
-
-	list_for_each_entry (pg, &m->priority_groups, list)
-		list_for_each_entry (p, &pg->paths, list)
-			if (p->dev->bdev == bdev)
-				return p;
-
-	return NULL;
-}
-
 static void fail_path(struct path *path)
 {
 	unsigned long flags;

@@ -570,8 +555,8 @@ static void fail_path(struct path *path)
 static int multipath_end_io(struct dm_target *ti, struct bio *bio,
 			    int error, union map_info *map_context)
 {
-	struct path *path;
 	struct multipath *m = (struct multipath *) ti->private;
+	struct path *path = (struct path *) map_context->ptr;
 
 	if (error) {
 		spin_lock(&m->lock);

@@ -581,7 +566,6 @@ static int multipath_end_io(struct dm_ta
 		}
 		spin_unlock(&m->lock);
 
-		path = find_path(m, bio->bi_bdev);
 		fail_path(path);
 
 		/* queue for the daemon to resubmit */

Mike Christie

2004-02-23 22:15 UTC #5 lore raw

Re: [Patch 1/6] dm: endio method

Hey Joe,

Quoted 7 lines · Joe Thornber

Mike,
Quoted 1 line · Mike Christie
Saving and restoring bi_bdev is going to break multipath.

Quoted 9 lines

Yes, we'll have to fall back to plan A and use the map_context pointer to hold the path being used (attached patch for illustration only). I had been hoping we could keep the map_context unused so that we could allow the path selectors to use it. I should have spotted this.
I'll also move the failed bio remap back to mpath_end_io(), so that the context can be reused there (it moved to the daemon when we were trying to do path testing in the kernel).

With this move if the path has to be activated first, will the daemon have to call some sort of ps_path_is_initialized() function before it calls generic_make_request?

It might be easier if mp's map_io call did not move so it or the ps could send commands and wait for the response before selecting a path. I guess this would mean you would have to add a access function for the tio's map_info so it could be set from the daemon, or mp may need to allocate its own io wrapper. It seems the latter may now be needed to give ps's a a map_info, becuase dm-mpath needs to store the path in the tio's map_info.

Mike

Joe Thornber

author

2004-02-23 22:34 UTC #6 lore raw

Re: [Patch 1/6] dm: endio method

Quoted 3 lines · Mike Christie

With this move if the path has to be activated first, will the daemon have to call some sort of ps_path_is_initialized() function before it calls generic_make_request?

Yes, I am planning to add something like this. Whether it needs to be per path, or we could get away per priority group is probably a question that you could answer better than me ? Do we need a corresponding deactivate for some hardware ?

Quoted 4 lines

tio's map_info so it could be set from the daemon, or mp may need to allocate its own io wrapper. It seems the latter may now be needed to give ps's a a map_info, becuase dm-mpath needs to store the path in the tio's map_info.

I think the bio recording/reset is going to have to move inside the target. It makes sense that the mpath target should be the only one that incurs this overhead. So yes, there will have to be a wrapper which could be used to provide context for the ps.

- Joe

Mike Christie

2004-02-24 02:20 UTC #7 lore raw

Re: [Patch 1/6] dm: endio method

Quoted 9 lines · Joe Thornber

Quoted 1 line · Mike Christie
With this move if the path has to be activated first, will the daemon have to call some sort of ps_path_is_initialized() function before it calls generic_make_request?
Yes, I am planning to add something like this. Whether it needs to be per path, or we could get away per priority group is probably a question that you could answer better than me ? Do we need a corresponding deactivate for some hardware ?

Sorry, I do not know for sure. All the HW we have will activate one group and deactivate the other in one command or automatically.

Mike