[PATCH 1/6] dm: change sector_t len to unsigned

[PATCH 1/6] dm: change sector_t len to unsigned

[Mikulas Patocka]

It is impossible to create bios with 2^23 or more sectors (the size is
stored as a 32-bit byte count in the bio). So we convert some sector_t
values to unsigned integers.

It is needed for the next patch that replaces the values with pointers, so
the size of the integer must match.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-3.14-rc5/drivers/md/dm.c
===================================================================
--- linux-3.14-rc5.orig/drivers/md/dm.c	2014-03-14 01:55:24.000000000 +0100
+++ linux-3.14-rc5/drivers/md/dm.c	2014-03-14 04:31:41.000000000 +0100
@@ -1147,10 +1147,10 @@ struct clone_info {
 	struct bio *bio;
 	struct dm_io *io;
 	sector_t sector;
-	sector_t sector_count;
+	unsigned sector_count;
 };
 
-static void bio_setup_sector(struct bio *bio, sector_t sector, sector_t len)
+static void bio_setup_sector(struct bio *bio, sector_t sector, unsigned len)
 {
 	bio->bi_iter.bi_sector = sector;
 	bio->bi_iter.bi_size = to_bytes(len);
@@ -1195,7 +1195,7 @@ static struct dm_target_io *alloc_tio(st
 
 static void __clone_and_map_simple_bio(struct clone_info *ci,
 				       struct dm_target *ti,
-				       unsigned target_bio_nr, sector_t len)
+				       unsigned target_bio_nr, unsigned len)
 {
 	struct dm_target_io *tio = alloc_tio(ci, ti, ci->bio->bi_max_vecs, target_bio_nr);
 	struct bio *clone = &tio->clone;
@@ -1213,7 +1213,7 @@ static void __clone_and_map_simple_bio(s
 }
 
 static void __send_duplicate_bios(struct clone_info *ci, struct dm_target *ti,
-				  unsigned num_bios, sector_t len)
+				  unsigned num_bios, unsigned len)
 {
 	unsigned target_bio_nr;
 
@@ -1278,7 +1278,7 @@ static int __send_changing_extent_only(s
 				       is_split_required_fn is_split_required)
 {
 	struct dm_target *ti;
-	sector_t len;
+	unsigned len;
 	unsigned num_bios;
 
 	do {
@@ -1297,9 +1297,9 @@ static int __send_changing_extent_only(s
 			return -EOPNOTSUPP;
 
 		if (is_split_required && !is_split_required(ti))
-			len = min(ci->sector_count, max_io_len_target_boundary(ci->sector, ti));
+			len = min((sector_t)ci->sector_count, max_io_len_target_boundary(ci->sector, ti));
 		else
-			len = min(ci->sector_count, max_io_len(ci->sector, ti));
+			len = min((sector_t)ci->sector_count, max_io_len(ci->sector, ti));
 
 		__send_duplicate_bios(ci, ti, num_bios, len);
 

[PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mikulas Patocka]

The function dm_accept_partial_bio allows the target to specify how many
sectors does it want to process. If the target wants to accept only a part
of the bio, it calls dm_accept_partial_bio and the dm core sends the rest
of the data in next bio.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm.c               |   58 ++++++++++++++++++++++++++++++++++++------
 include/linux/device-mapper.h |    2 +
 2 files changed, 52 insertions(+), 8 deletions(-)

Index: linux-3.14-rc5/drivers/md/dm.c
===================================================================
--- linux-3.14-rc5.orig/drivers/md/dm.c	2014-03-14 04:31:41.000000000 +0100
+++ linux-3.14-rc5/drivers/md/dm.c	2014-03-14 04:32:08.000000000 +0100
@@ -1105,6 +1105,45 @@ int dm_set_target_max_io_len(struct dm_t
 }
 EXPORT_SYMBOL_GPL(dm_set_target_max_io_len);
 
+/*
+ * A target may call dm_accept_partial_bio only from the map routine. It is
+ * allowed for all bio types except REQ_FLUSH.
+ *
+ * dm_accept_partial_bio informs the dm that the target only wants to process
+ * additional n_sectors sectors of the bio and the rest of the data should be
+ * sent in a next bio.
+ *
+ * A diagram that explains the arithmetics:
+ * +--------------------+---------------+-------+
+ * |         1          |       2       |   3   |
+ * +--------------------+---------------+-------+
+ *
+ * <-------------- *tio->len_ptr --------------->
+ *                      <------- bi_size ------->
+ *                      <-- n_sectors -->
+ *
+ * Region 1 was already iterated over with bio_advance or similar function.
+ *	(it may be empty if the target doesn't use bio_advance)
+ * Region 2 is the remaining bio size that the target wants to process.
+ *	(it may be empty if region 1 is non-empty, although there is no reason
+ *	 to make it empty)
+ * The target requires that region 3 is to be sent in the next bio.
+ *
+ * If the target wants to receive multiple copies of the bio with num_write_bios
+ * or num_write_same_bios, the partially processed part (the sum of regions 1+2)
+ * must be the same for all copies of the bio.
+ */
+void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
+{
+	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
+	unsigned bi_size = bio->bi_iter.bi_size >> SECTOR_SHIFT;
+	BUG_ON(bi_size > *tio->len_ptr);
+	BUG_ON(n_sectors > bi_size);
+	*tio->len_ptr -= bi_size - n_sectors;
+	bio->bi_iter.bi_size = n_sectors << SECTOR_SHIFT;
+}
+EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
+
 static void __map_bio(struct dm_target_io *tio)
 {
 	int r;
@@ -1195,11 +1234,13 @@ static struct dm_target_io *alloc_tio(st
 
 static void __clone_and_map_simple_bio(struct clone_info *ci,
 				       struct dm_target *ti,
-				       unsigned target_bio_nr, unsigned len)
+				       unsigned target_bio_nr, unsigned *len)
 {
 	struct dm_target_io *tio = alloc_tio(ci, ti, ci->bio->bi_max_vecs, target_bio_nr);
 	struct bio *clone = &tio->clone;
 
+	tio->len_ptr = len;
+
 	/*
 	 * Discard requests require the bio's inline iovecs be initialized.
 	 * ci->bio->bi_max_vecs is BIO_INLINE_VECS anyway, for both flush
@@ -1207,13 +1248,13 @@ static void __clone_and_map_simple_bio(s
 	 */
 	 __bio_clone_fast(clone, ci->bio);
 	if (len)
-		bio_setup_sector(clone, ci->sector, len);
+		bio_setup_sector(clone, ci->sector, *len);
 
 	__map_bio(tio);
 }
 
 static void __send_duplicate_bios(struct clone_info *ci, struct dm_target *ti,
-				  unsigned num_bios, unsigned len)
+				  unsigned num_bios, unsigned *len)
 {
 	unsigned target_bio_nr;
 
@@ -1228,13 +1269,13 @@ static int __send_empty_flush(struct clo
 
 	BUG_ON(bio_has_data(ci->bio));
 	while ((ti = dm_table_get_target(ci->map, target_nr++)))
-		__send_duplicate_bios(ci, ti, ti->num_flush_bios, 0);
+		__send_duplicate_bios(ci, ti, ti->num_flush_bios, NULL);
 
 	return 0;
 }
 
 static void __clone_and_map_data_bio(struct clone_info *ci, struct dm_target *ti,
-				     sector_t sector, unsigned len)
+				     sector_t sector, unsigned *len)
 {
 	struct bio *bio = ci->bio;
 	struct dm_target_io *tio;
@@ -1249,7 +1290,8 @@ static void __clone_and_map_data_bio(str
 
 	for (target_bio_nr = 0; target_bio_nr < num_target_bios; target_bio_nr++) {
 		tio = alloc_tio(ci, ti, 0, target_bio_nr);
-		clone_bio(tio, bio, sector, len);
+		tio->len_ptr = len;
+		clone_bio(tio, bio, sector, *len);
 		__map_bio(tio);
 	}
 }
@@ -1301,7 +1343,7 @@ static int __send_changing_extent_only(s
 		else
 			len = min((sector_t)ci->sector_count, max_io_len(ci->sector, ti));
 
-		__send_duplicate_bios(ci, ti, num_bios, len);
+		__send_duplicate_bios(ci, ti, num_bios, &len);
 
 		ci->sector += len;
 	} while (ci->sector_count -= len);
@@ -1340,7 +1382,7 @@ static int __split_and_process_non_flush
 
 	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
 
-	__clone_and_map_data_bio(ci, ti, ci->sector, len);
+	__clone_and_map_data_bio(ci, ti, ci->sector, &len);
 
 	ci->sector += len;
 	ci->sector_count -= len;
Index: linux-3.14-rc5/include/linux/device-mapper.h
===================================================================
--- linux-3.14-rc5.orig/include/linux/device-mapper.h	2014-03-14 04:30:51.000000000 +0100
+++ linux-3.14-rc5/include/linux/device-mapper.h	2014-03-14 04:32:08.000000000 +0100
@@ -291,6 +291,7 @@ struct dm_target_io {
 	struct dm_io *io;
 	struct dm_target *ti;
 	unsigned target_bio_nr;
+	unsigned *len_ptr;
 	struct bio clone;
 };
 
@@ -401,6 +402,7 @@ int dm_copy_name_and_uuid(struct mapped_
 struct gendisk *dm_disk(struct mapped_device *md);
 int dm_suspended(struct dm_target *ti);
 int dm_noflush_suspending(struct dm_target *ti);
+void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors);
 union map_info *dm_get_rq_mapinfo(struct request *rq);
 
 struct queue_limits *dm_get_queue_limits(struct mapped_device *md);

[PATCH 3/6] dm-snapshot: make the origin target allocate per-target structure

[Mikulas Patocka]

Allocate a per-target structure dm_origin. This is needed for the next
patch, that adds new entry to the structure.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap.c |   53 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 18 deletions(-)

Index: linux-3.14-rc5/drivers/md/dm-snap.c
===================================================================
--- linux-3.14-rc5.orig/drivers/md/dm-snap.c	2014-03-14 03:20:43.000000000 +0100
+++ linux-3.14-rc5/drivers/md/dm-snap.c	2014-03-14 04:29:02.000000000 +0100
@@ -2141,6 +2141,10 @@ static int origin_write_extent(struct dm
  * Origin: maps a linear range of a device, with hooks for snapshotting.
  */
 
+struct dm_origin {
+	struct dm_dev *dev;
+};
+
 /*
  * Construct an origin mapping: <dev_path>
  * The context for an origin is merely a 'struct dm_dev *'
@@ -2149,41 +2153,54 @@ static int origin_write_extent(struct dm
 static int origin_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 {
 	int r;
-	struct dm_dev *dev;
+	struct dm_origin *o;
 
 	if (argc != 1) {
 		ti->error = "origin: incorrect number of arguments";
 		return -EINVAL;
 	}
 
-	r = dm_get_device(ti, argv[0], dm_table_get_mode(ti->table), &dev);
+	o = kmalloc(sizeof(struct dm_origin), GFP_KERNEL);
+	if (!o) {
+		ti->error = "Cannot allocate private origin structure";
+		r = -ENOMEM;
+		goto bad_alloc;
+	}
+
+	r = dm_get_device(ti, argv[0], dm_table_get_mode(ti->table), &o->dev);
 	if (r) {
 		ti->error = "Cannot get target device";
-		return r;
+		goto bad_open;
 	}
 
-	ti->private = dev;
+	ti->private = o;
 	ti->num_flush_bios = 1;
 
 	return 0;
+
+bad_open:
+	kfree(o);
+bad_alloc:
+	return r;
 }
 
 static void origin_dtr(struct dm_target *ti)
 {
-	struct dm_dev *dev = ti->private;
-	dm_put_device(ti, dev);
+	struct dm_origin *o = ti->private;
+	dm_put_device(ti, o->dev);
+	kfree(o);
 }
 
 static int origin_map(struct dm_target *ti, struct bio *bio)
 {
-	struct dm_dev *dev = ti->private;
-	bio->bi_bdev = dev->bdev;
+	struct dm_origin *o = ti->private;
+	bio->bi_bdev = o->dev->bdev;
 
 	if (bio->bi_rw & REQ_FLUSH)
 		return DM_MAPIO_REMAPPED;
 
 	/* Only tell snapshots if this is a write */
-	return (bio_rw(bio) == WRITE) ? do_origin(dev, bio) : DM_MAPIO_REMAPPED;
+	return (bio_rw(bio) == WRITE) ? do_origin(o->dev, bio) : DM_MAPIO_REMAPPED;
 }
 
 /*
@@ -2192,15 +2209,15 @@ static int origin_map(struct dm_target *
  */
 static void origin_resume(struct dm_target *ti)
 {
-	struct dm_dev *dev = ti->private;
+	struct dm_origin *o = ti->private;
 
-	ti->max_io_len = get_origin_minimum_chunksize(dev->bdev);
+	ti->max_io_len = get_origin_minimum_chunksize(o->dev->bdev);
 }
 
 static void origin_status(struct dm_target *ti, status_type_t type,
 			  unsigned status_flags, char *result, unsigned maxlen)
 {
-	struct dm_dev *dev = ti->private;
+	struct dm_origin *o = ti->private;
 
 	switch (type) {
 	case STATUSTYPE_INFO:
@@ -2208,7 +2225,7 @@ static void origin_status(struct dm_targ
 		break;
 
 	case STATUSTYPE_TABLE:
-		snprintf(result, maxlen, "%s", dev->name);
+		snprintf(result, maxlen, "%s", o->dev->name);
 		break;
 	}
 }
@@ -2216,13 +2233,13 @@ static void origin_status(struct dm_targ
 static int origin_merge(struct dm_target *ti, struct bvec_merge_data *bvm,
 			struct bio_vec *biovec, int max_size)
 {
-	struct dm_dev *dev = ti->private;
-	struct request_queue *q = bdev_get_queue(dev->bdev);
+	struct dm_origin *o = ti->private;
+	struct request_queue *q = bdev_get_queue(o->dev->bdev);
 
 	if (!q->merge_bvec_fn)
 		return max_size;
 
-	bvm->bi_bdev = dev->bdev;
+	bvm->bi_bdev = o->dev->bdev;
 
 	return min(max_size, q->merge_bvec_fn(q, bvm, biovec));
 }
@@ -2230,9 +2247,9 @@ static int origin_merge(struct dm_target
 static int origin_iterate_devices(struct dm_target *ti,
 				  iterate_devices_callout_fn fn, void *data)
 {
-	struct dm_dev *dev = ti->private;
+	struct dm_origin *o = ti->private;
 
-	return fn(ti, dev, 0, ti->len, data);
+	return fn(ti, o->dev, 0, ti->len, data);
 }
 
 static struct target_type origin_target = {

[PATCH 4/6] dm origin: split only write bios

[Mikulas Patocka]

Change the dm origin target so that only write bios are split on chunk
boundary. Read bios are passed unchanged to the underlying device, so they
don't have to be split.

Later, we could change the target so that it accepts a larger write bio if
it spans an area that is completely covered by the snapshots.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Index: linux-3.14-rc6/drivers/md/dm-snap.c
===================================================================
--- linux-3.14-rc6.orig/drivers/md/dm-snap.c	2014-03-14 04:37:54.000000000 +0100
+++ linux-3.14-rc6/drivers/md/dm-snap.c	2014-03-14 05:23:42.000000000 +0100
@@ -2143,6 +2143,7 @@ static int origin_write_extent(struct dm
 
 struct dm_origin {
 	struct dm_dev *dev;
+	unsigned split_boundary;
 };
 
 /*
@@ -2194,13 +2195,24 @@ static void origin_dtr(struct dm_target 
 static int origin_map(struct dm_target *ti, struct bio *bio)
 {
 	struct dm_origin *o = ti->private;
+	unsigned available_sectors;
+
 	bio->bi_bdev = o->dev->bdev;
 
-	if (bio->bi_rw & REQ_FLUSH)
+	if (unlikely(bio->bi_rw & REQ_FLUSH))
+		return DM_MAPIO_REMAPPED;
+
+	if (bio_rw(bio) != WRITE)
 		return DM_MAPIO_REMAPPED;
 
+	available_sectors = o->split_boundary -
+		((unsigned)bio->bi_iter.bi_sector & (o->split_boundary - 1));
+
+	if (bio_sectors(bio) > available_sectors)
+		dm_accept_partial_bio(bio, available_sectors);
+
 	/* Only tell snapshots if this is a write */
-	return (bio_rw(bio) == WRITE) ? do_origin(o->dev, bio) : DM_MAPIO_REMAPPED;
+	return do_origin(o->dev, bio);
 }
 
 /*
@@ -2211,7 +2223,7 @@ static void origin_resume(struct dm_targ
 {
 	struct dm_origin *o = ti->private;
 
-	ti->max_io_len = get_origin_minimum_chunksize(o->dev->bdev);
+	o->split_boundary = get_origin_minimum_chunksize(o->dev->bdev);
 }
 
 static void origin_status(struct dm_target *ti, status_type_t type,

[PATCH 5/6] dm: remove num_write_bios

[Mikulas Patocka]

The target can set the function num_write_bios - dm will issue this
callback to ask the target how many bios does it want to receive.

This was intended for the dm-cache target, but it is not useable due to a
race condition (see the description of
e2e74d617eadc15f601983270c4f4a6935c5a943). num_write_bios is unused, so we
remove it.

Note that we deliberately leave the for loop in __clone_and_map_data_bio -
it will be used in the next patch.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm.c               |    6 ------
 include/linux/device-mapper.h |   15 ---------------
 2 files changed, 21 deletions(-)

Index: linux-3.14-rc6/drivers/md/dm.c
===================================================================
--- linux-3.14-rc6.orig/drivers/md/dm.c	2014-03-14 22:02:31.000000000 +0100
+++ linux-3.14-rc6/drivers/md/dm.c	2014-03-14 22:04:39.000000000 +0100
@@ -1282,12 +1282,6 @@ static void __clone_and_map_data_bio(str
 	unsigned target_bio_nr;
 	unsigned num_target_bios = 1;
 
-	/*
-	 * Does the target want to receive duplicate copies of the bio?
-	 */
-	if (bio_data_dir(bio) == WRITE && ti->num_write_bios)
-		num_target_bios = ti->num_write_bios(ti, bio);
-
 	for (target_bio_nr = 0; target_bio_nr < num_target_bios; target_bio_nr++) {
 		tio = alloc_tio(ci, ti, 0, target_bio_nr);
 		tio->len_ptr = len;
Index: linux-3.14-rc6/include/linux/device-mapper.h
===================================================================
--- linux-3.14-rc6.orig/include/linux/device-mapper.h	2014-03-14 22:02:05.000000000 +0100
+++ linux-3.14-rc6/include/linux/device-mapper.h	2014-03-14 22:02:21.000000000 +0100
@@ -190,14 +190,6 @@ struct target_type {
 #define DM_TARGET_IMMUTABLE		0x00000004
 #define dm_target_is_immutable(type)	((type)->features & DM_TARGET_IMMUTABLE)
 
-/*
- * Some targets need to be sent the same WRITE bio severals times so
- * that they can send copies of it to different devices.  This function
- * examines any supplied bio and returns the number of copies of it the
- * target requires.
- */
-typedef unsigned (*dm_num_write_bios_fn) (struct dm_target *ti, struct bio *bio);
-
 struct dm_target {
 	struct dm_table *table;
 	struct target_type *type;
@@ -237,13 +229,6 @@ struct dm_target {
 	 */
 	unsigned per_bio_data_size;
 
-	/*
-	 * If defined, this function is called to find out how many
-	 * duplicate bios should be sent to the target when writing
-	 * data.
-	 */
-	dm_num_write_bios_fn num_write_bios;
-
 	/* target specific data */
 	void *private;
 

[PATCH 6/6] dm: introduce dm_ask_for_duplicate_bios

[Mikulas Patocka]

This function can be used if the target needs to receive another duplicate
of the current bio.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm.c               |   24 +++++++++++++++++++-----
 include/linux/device-mapper.h |    2 ++
 2 files changed, 21 insertions(+), 5 deletions(-)

Index: linux-3.14-rc6/drivers/md/dm.c
===================================================================
--- linux-3.14-rc6.orig/drivers/md/dm.c	2014-03-14 23:25:27.000000000 +0100
+++ linux-3.14-rc6/drivers/md/dm.c	2014-03-14 23:33:09.000000000 +0100
@@ -1129,9 +1129,9 @@ EXPORT_SYMBOL_GPL(dm_set_target_max_io_l
  *	 to make it empty)
  * The target requires that region 3 is to be sent in the next bio.
  *
- * If the target wants to receive multiple copies of the bio with num_write_bios
- * or num_write_same_bios, the partially processed part (the sum of regions 1+2)
- * must be the same for all copies of the bio.
+ * If the target wants to receive multiple copies of the bio with num_*_bios or
+ * dm_ask_for_duplicate_bio, the partially processed part (the sum of regions
+ * 1+2) must be the same for all copies of the bio.
  */
 void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
 {
@@ -1144,6 +1144,17 @@ void dm_accept_partial_bio(struct bio *b
 }
 EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
 
+/*
+ * The target driver can call this function only from the map routine. The
+ * target driver requests that the dm sends more duplicates of the current bio.
+ */
+void dm_ask_for_duplicate_bios(struct bio *bio, unsigned n_duplicates)
+{
+	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
+	(*tio->num_bios) += n_duplicates;
+}
+EXPORT_SYMBOL_GPL(dm_ask_for_duplicate_bios);
+
 static void __map_bio(struct dm_target_io *tio)
 {
 	int r;
@@ -1234,12 +1245,14 @@ static struct dm_target_io *alloc_tio(st
 
 static void __clone_and_map_simple_bio(struct clone_info *ci,
 				       struct dm_target *ti,
-				       unsigned target_bio_nr, unsigned *len)
+				       unsigned target_bio_nr, unsigned *len,
+				       unsigned *num_bios)
 {
 	struct dm_target_io *tio = alloc_tio(ci, ti, ci->bio->bi_max_vecs, target_bio_nr);
 	struct bio *clone = &tio->clone;
 
 	tio->len_ptr = len;
+	tio->num_bios = num_bios;
 
 	/*
 	 * Discard requests require the bio's inline iovecs be initialized.
@@ -1259,7 +1272,7 @@ static void __send_duplicate_bios(struct
 	unsigned target_bio_nr;
 
 	for (target_bio_nr = 0; target_bio_nr < num_bios; target_bio_nr++)
-		__clone_and_map_simple_bio(ci, ti, target_bio_nr, len);
+		__clone_and_map_simple_bio(ci, ti, target_bio_nr, len, &num_bios);
 }
 
 static int __send_empty_flush(struct clone_info *ci)
@@ -1285,6 +1298,7 @@ static void __clone_and_map_data_bio(str
 	for (target_bio_nr = 0; target_bio_nr < num_target_bios; target_bio_nr++) {
 		tio = alloc_tio(ci, ti, 0, target_bio_nr);
 		tio->len_ptr = len;
+		tio->num_bios = &num_target_bios;
 		clone_bio(tio, bio, sector, *len);
 		__map_bio(tio);
 	}
Index: linux-3.14-rc6/include/linux/device-mapper.h
===================================================================
--- linux-3.14-rc6.orig/include/linux/device-mapper.h	2014-03-14 23:25:27.000000000 +0100
+++ linux-3.14-rc6/include/linux/device-mapper.h	2014-03-14 23:25:27.000000000 +0100
@@ -277,6 +277,7 @@ struct dm_target_io {
 	struct dm_target *ti;
 	unsigned target_bio_nr;
 	unsigned *len_ptr;
+	unsigned *num_bios;
 	struct bio clone;
 };
 
@@ -388,6 +389,7 @@ struct gendisk *dm_disk(struct mapped_de
 int dm_suspended(struct dm_target *ti);
 int dm_noflush_suspending(struct dm_target *ti);
 void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors);
+void dm_ask_for_duplicate_bios(struct bio *bio, unsigned n_duplicates);
 union map_info *dm_get_rq_mapinfo(struct request *rq);
 
 struct queue_limits *dm_get_queue_limits(struct mapped_device *md);

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mike Snitzer]

On Fri, Mar 14 2014 at  6:41pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> The function dm_accept_partial_bio allows the target to specify how many
> sectors does it want to process. If the target wants to accept only a part
> of the bio, it calls dm_accept_partial_bio and the dm core sends the rest
> of the data in next bio.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm.c               |   58 ++++++++++++++++++++++++++++++++++++------
>  include/linux/device-mapper.h |    2 +
>  2 files changed, 52 insertions(+), 8 deletions(-)
> 
> Index: linux-3.14-rc5/drivers/md/dm.c
> ===================================================================
> --- linux-3.14-rc5.orig/drivers/md/dm.c	2014-03-14 04:31:41.000000000 +0100
> +++ linux-3.14-rc5/drivers/md/dm.c	2014-03-14 04:32:08.000000000 +0100
> @@ -1105,6 +1105,45 @@ int dm_set_target_max_io_len(struct dm_t
>  }
>  EXPORT_SYMBOL_GPL(dm_set_target_max_io_len);
>  
> +/*
> + * A target may call dm_accept_partial_bio only from the map routine. It is
> + * allowed for all bio types except REQ_FLUSH.
> + *
> + * dm_accept_partial_bio informs the dm that the target only wants to process
> + * additional n_sectors sectors of the bio and the rest of the data should be
> + * sent in a next bio.
> + *
> + * A diagram that explains the arithmetics:
> + * +--------------------+---------------+-------+
> + * |         1          |       2       |   3   |
> + * +--------------------+---------------+-------+
> + *
> + * <-------------- *tio->len_ptr --------------->
> + *                      <------- bi_size ------->
> + *                      <-- n_sectors -->
> + *
> + * Region 1 was already iterated over with bio_advance or similar function.
> + *	(it may be empty if the target doesn't use bio_advance)
> + * Region 2 is the remaining bio size that the target wants to process.
> + *	(it may be empty if region 1 is non-empty, although there is no reason
> + *	 to make it empty)
> + * The target requires that region 3 is to be sent in the next bio.
> + *
> + * If the target wants to receive multiple copies of the bio with num_write_bios
> + * or num_write_same_bios, the partially processed part (the sum of regions 1+2)
> + * must be the same for all copies of the bio.
> + */
> +void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
> +{
> +	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
> +	unsigned bi_size = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> +	BUG_ON(bi_size > *tio->len_ptr);
> +	BUG_ON(n_sectors > bi_size);
> +	*tio->len_ptr -= bi_size - n_sectors;
> +	bio->bi_iter.bi_size = n_sectors << SECTOR_SHIFT;
> +}
> +EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
> +

The dm_accept_partial_bio interface should return an error (e.g. if bio
has REQ_FLUSH set, or the above BUG_ON conditions are met).  The method
should probably also be designated with __must_check.  And it should
_not_ BUG_ON.  BUG_ON is a crutch that I do not appreciate seeing in new
code.  If you'd like to keep them, and get stacktraces, I'd prefer
seeing them converted to WARN_ON that is followed with an error return.

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mikulas Patocka]

On Mon, 17 Mar 2014, Mike Snitzer wrote:

> > +void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
> > +{
> > +	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
> > +	unsigned bi_size = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> > +	BUG_ON(bi_size > *tio->len_ptr);
> > +	BUG_ON(n_sectors > bi_size);
> > +	*tio->len_ptr -= bi_size - n_sectors;
> > +	bio->bi_iter.bi_size = n_sectors << SECTOR_SHIFT;
> > +}
> > +EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
> > +
> 
> The dm_accept_partial_bio interface should return an error (e.g. if bio
> has REQ_FLUSH set, or the above BUG_ON conditions are met).  The method
> should probably also be designated with __must_check.  And it should
> _not_ BUG_ON.  BUG_ON is a crutch that I do not appreciate seeing in new
> code.  If you'd like to keep them, and get stacktraces, I'd prefer
> seeing them converted to WARN_ON that is followed with an error return.

You need to return error for externally triggered events (like disk 
errors, memory allocation failures, etc.), not for bugs in the code.

Here, if someone passes invalid n_sectors, it is not externally triggered 
event (the code that passed the invalid argument is just buggy) - so 
crashing on BUG_ON is OK.


Another problem is that warnings can be easily missed, BUG won't be 
missed. For example, the lvm testsuite generates large number of messages 
to the syslog. Consequently, when I run the testsuite, I don't read 
syslog. Consequently, if you convert BUG_ON to WARN_ON and the testsuite 
triggers it, I will miss it.


Regarding REQ_FLUSH - it would already crash on NULL pointer dereference 
when accessing *tio->len_ptr.

Mikulas

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mike Snitzer]

On Mon, Mar 17 2014 at  1:08pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 17 Mar 2014, Mike Snitzer wrote:
> 
> > > +void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
> > > +{
> > > +	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
> > > +	unsigned bi_size = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> > > +	BUG_ON(bi_size > *tio->len_ptr);
> > > +	BUG_ON(n_sectors > bi_size);
> > > +	*tio->len_ptr -= bi_size - n_sectors;
> > > +	bio->bi_iter.bi_size = n_sectors << SECTOR_SHIFT;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
> > > +
> > 
> > The dm_accept_partial_bio interface should return an error (e.g. if bio
> > has REQ_FLUSH set, or the above BUG_ON conditions are met).  The method
> > should probably also be designated with __must_check.  And it should
> > _not_ BUG_ON.  BUG_ON is a crutch that I do not appreciate seeing in new
> > code.  If you'd like to keep them, and get stacktraces, I'd prefer
> > seeing them converted to WARN_ON that is followed with an error return.
> 
> You need to return error for externally triggered events (like disk 
> errors, memory allocation failures, etc.), not for bugs in the code.
> 
> Here, if someone passes invalid n_sectors, it is not externally triggered 
> event (the code that passed the invalid argument is just buggy) - so 
> crashing on BUG_ON is OK.
> 
> 
> Another problem is that warnings can be easily missed, BUG won't be 
> missed. For example, the lvm testsuite generates large number of messages 
> to the syslog. Consequently, when I run the testsuite, I don't read 
> syslog. Consequently, if you convert BUG_ON to WARN_ON and the testsuite 
> triggers it, I will miss it.
> 
> 
> Regarding REQ_FLUSH - it would already crash on NULL pointer dereference 
> when accessing *tio->len_ptr.

None of what you responded with here is acceptable.  Your preferred
conventions don't accomodate people implementing a new target, or
adapting an existing target, to use this new interface you've proposed.
In this instance, BUG() is a hostile response to what is likely a minor
oversight.

Returning an error will not allow the caller to do what they thought
would happen.  So even you should catch that (despite you apparently not
looking at the kernel log messages).

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mike Snitzer]

On Mon, Mar 17 2014 at  1:43pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Mon, Mar 17 2014 at  1:08pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > Regarding REQ_FLUSH - it would already crash on NULL pointer dereference 
> > when accessing *tio->len_ptr.

I realize if any of these BUG_ON checks hits the calling target code has
a bug.  I just think it a fairly harsh response to BUG the system.  But
I can let this issue go.

I do however want you to add yet another BUG_ON for any bio that isn't
meant to be sent to this interface (only one so far is REQ_FLUSH AFAIK).

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mikulas Patocka]

On Mon, 17 Mar 2014, Mike Snitzer wrote:

> On Mon, Mar 17 2014 at  1:43pm -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
> > On Mon, Mar 17 2014 at  1:08pm -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > 
> > > Regarding REQ_FLUSH - it would already crash on NULL pointer dereference 
> > > when accessing *tio->len_ptr.
> 
> I realize if any of these BUG_ON checks hits the calling target code has
> a bug.  I just think it a fairly harsh response to BUG the system.  But
> I can let this issue go.
> 
> I do however want you to add yet another BUG_ON for any bio that isn't
> meant to be sent to this interface (only one so far is REQ_FLUSH AFAIK).

Then, it triggers NULL pointer dereference. You don't have to add BUG_ON 
for it, it crashes anyway.


BTW. just a quote from a historical article to show how far have these 
attempts to handle errors went - about half of the code in Multics was 
error recovery (http://www.multicians.org/unix.html):

	We went to lunch afterward, and I remarked to Dennis that easily half the 
	code I was writing in Multics was error recovery code. He said, "We left
	all that stuff out. If there's an error, we have this routine called 
	panic, and when it is called, the machine crashes, and you holler down the 
	hall, 'Hey, reboot it.'"

Mikulas

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mike Snitzer]

On Mon, Mar 17 2014 at  3:18pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 17 Mar 2014, Mike Snitzer wrote:
> 
> > On Mon, Mar 17 2014 at  1:43pm -0400,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > > On Mon, Mar 17 2014 at  1:08pm -0400,
> > > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > 
> > > > 
> > > > Regarding REQ_FLUSH - it would already crash on NULL pointer dereference 
> > > > when accessing *tio->len_ptr.
> > 
> > I realize if any of these BUG_ON checks hits the calling target code has
> > a bug.  I just think it a fairly harsh response to BUG the system.  But
> > I can let this issue go.
> > 
> > I do however want you to add yet another BUG_ON for any bio that isn't
> > meant to be sent to this interface (only one so far is REQ_FLUSH AFAIK).
> 
> Then, it triggers NULL pointer dereference. You don't have to add BUG_ON 
> for it, it crashes anyway.

I understand it'll NULL pointer, but that is _always_ confusing for
someone who wasn't the original developer.  Please add the explicit
BUG_ON() for REQ_FLUSH.

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mikulas Patocka]

On Mon, 17 Mar 2014, Mike Snitzer wrote:

> > > I do however want you to add yet another BUG_ON for any bio that isn't
> > > meant to be sent to this interface (only one so far is REQ_FLUSH AFAIK).
> > 
> > Then, it triggers NULL pointer dereference. You don't have to add BUG_ON 
> > for it, it crashes anyway.
> 
> I understand it'll NULL pointer, but that is _always_ confusing for
> someone who wasn't the original developer.  Please add the explicit
> BUG_ON() for REQ_FLUSH.

The user will not hit a BUG with REQ_FLUSH. Regarding the developer - it 
is expected that the kernel developer is able to decode the oops message, 
compare it with disassembled code and find out which variable was NULL. 
The function is small, so it's an easy task.

We don't really need
BUG_ON(!pointer);
variable = *pointer;

Mikulas

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mike Snitzer]

On Mon, Mar 17 2014 at  6:45pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 17 Mar 2014, Mike Snitzer wrote:
> 
> > > > I do however want you to add yet another BUG_ON for any bio that isn't
> > > > meant to be sent to this interface (only one so far is REQ_FLUSH AFAIK).
> > > 
> > > Then, it triggers NULL pointer dereference. You don't have to add BUG_ON 
> > > for it, it crashes anyway.
> > 
> > I understand it'll NULL pointer, but that is _always_ confusing for
> > someone who wasn't the original developer.  Please add the explicit
> > BUG_ON() for REQ_FLUSH.
> 
> The user will not hit a BUG with REQ_FLUSH. Regarding the developer - it 
> is expected that the kernel developer is able to decode the oops message, 
> compare it with disassembled code and find out which variable was NULL. 
> The function is small, so it's an easy task.

By that logic then no BUG_ON() should ever be needed.  You need to know
to quit while you're ahead ;)

> We don't really need
> BUG_ON(!pointer);
> variable = *pointer;

Right, we don't.. but it helps resolve issues quicker if thoughtful
BUG_ON()s hit before more obscure crashes do.  Whereby obviating the
need to reverse engineer _why_ the NULL pointer occurred.

Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mikulas Patocka]

On Mon, 17 Mar 2014, Mike Snitzer wrote:

> > The user will not hit a BUG with REQ_FLUSH. Regarding the developer - it 
> > is expected that the kernel developer is able to decode the oops message, 
> > compare it with disassembled code and find out which variable was NULL. 
> > The function is small, so it's an easy task.
> 
> By that logic then no BUG_ON() should ever be needed.  You need to know
> to quit while you're ahead ;)

The difference is this - if you ignore BUG_ON on bad arithmetics in bio 
processing, you may introduce subtle data corruption. So, the developer 
may spend hours or days trying to reproduce the data corruption. When he 
finds a reproducible case, he has to spend additional time trying to find 
the reason for the corruption, in worst case it may involve recording the 
content of all incoming and outcoming bios and searching for a 
discrepancy.

BUG_ON saves all this work.

> > We don't really need
> > BUG_ON(!pointer);
> > variable = *pointer;
> 
> Right, we don't.. but it helps resolve issues quicker if thoughtful
> BUG_ON()s hit before more obscure crashes do.  Whereby obviating the
> need to reverse engineer _why_ the NULL pointer occurred.

On the other hand, if the developer calls dm_accept_partial_bio on a flush 
request, he gets a NULL pointer dereference, the crash is perfectly 
reproducible, there is no subltle data corruption. You can add 
BUG_ON(bio->bi_rw & REQ_FLUSH), I'm not strongly opposed to that, I just 
think it is not needed.

Mikulas

Re: [PATCH 6/6] dm: introduce dm_ask_for_duplicate_bios

[Mike Snitzer]

On Fri, Mar 14 2014 at  6:44pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> This function can be used if the target needs to receive another duplicate
> of the current bio.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm.c               |   24 +++++++++++++++++++-----
>  include/linux/device-mapper.h |    2 ++
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> Index: linux-3.14-rc6/drivers/md/dm.c
> ===================================================================
> --- linux-3.14-rc6.orig/drivers/md/dm.c	2014-03-14 23:25:27.000000000 +0100
> +++ linux-3.14-rc6/drivers/md/dm.c	2014-03-14 23:33:09.000000000 +0100
> @@ -1129,9 +1129,9 @@ EXPORT_SYMBOL_GPL(dm_set_target_max_io_l
>   *	 to make it empty)
>   * The target requires that region 3 is to be sent in the next bio.
>   *
> - * If the target wants to receive multiple copies of the bio with num_write_bios
> - * or num_write_same_bios, the partially processed part (the sum of regions 1+2)
> - * must be the same for all copies of the bio.
> + * If the target wants to receive multiple copies of the bio with num_*_bios or
> + * dm_ask_for_duplicate_bio, the partially processed part (the sum of regions
> + * 1+2) must be the same for all copies of the bio.
>   */
>  void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
>  {
> @@ -1144,6 +1144,17 @@ void dm_accept_partial_bio(struct bio *b
>  }
>  EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
>  
> +/*
> + * The target driver can call this function only from the map routine. The
> + * target driver requests that the dm sends more duplicates of the current bio.
> + */
> +void dm_ask_for_duplicate_bios(struct bio *bio, unsigned n_duplicates)
> +{
> +	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
> +	(*tio->num_bios) += n_duplicates;
> +}
> +EXPORT_SYMBOL_GPL(dm_ask_for_duplicate_bios);
> +
>  static void __map_bio(struct dm_target_io *tio)
>  {
>  	int r;

Hey Mikulas,

I'm not immediately seeing how increasing tio->num_bios from within a
target's map method, using dm_ask_for_duplicate_bios, will cause DM core
to send duplicates.

Nothing in DM core actually consumes tio->num_bios -- please advise.

Thanks,
Mike

Re: [PATCH 6/6] dm: introduce dm_ask_for_duplicate_bios

[Mikulas Patocka]

On Fri, 25 Apr 2014, Mike Snitzer wrote:

> On Fri, Mar 14 2014 at  6:44pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > This function can be used if the target needs to receive another duplicate
> > of the current bio.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  drivers/md/dm.c               |   24 +++++++++++++++++++-----
> >  include/linux/device-mapper.h |    2 ++
> >  2 files changed, 21 insertions(+), 5 deletions(-)
> > 
> > Index: linux-3.14-rc6/drivers/md/dm.c
> > ===================================================================
> > --- linux-3.14-rc6.orig/drivers/md/dm.c	2014-03-14 23:25:27.000000000 +0100
> > +++ linux-3.14-rc6/drivers/md/dm.c	2014-03-14 23:33:09.000000000 +0100
> > @@ -1129,9 +1129,9 @@ EXPORT_SYMBOL_GPL(dm_set_target_max_io_l
> >   *	 to make it empty)
> >   * The target requires that region 3 is to be sent in the next bio.
> >   *
> > - * If the target wants to receive multiple copies of the bio with num_write_bios
> > - * or num_write_same_bios, the partially processed part (the sum of regions 1+2)
> > - * must be the same for all copies of the bio.
> > + * If the target wants to receive multiple copies of the bio with num_*_bios or
> > + * dm_ask_for_duplicate_bio, the partially processed part (the sum of regions
> > + * 1+2) must be the same for all copies of the bio.
> >   */
> >  void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
> >  {
> > @@ -1144,6 +1144,17 @@ void dm_accept_partial_bio(struct bio *b
> >  }
> >  EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
> >  
> > +/*
> > + * The target driver can call this function only from the map routine. The
> > + * target driver requests that the dm sends more duplicates of the current bio.
> > + */
> > +void dm_ask_for_duplicate_bios(struct bio *bio, unsigned n_duplicates)
> > +{
> > +	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
> > +	(*tio->num_bios) += n_duplicates;
> > +}
> > +EXPORT_SYMBOL_GPL(dm_ask_for_duplicate_bios);
> > +
> >  static void __map_bio(struct dm_target_io *tio)
> >  {
> >  	int r;
> 
> Hey Mikulas,
> 
> I'm not immediately seeing how increasing tio->num_bios from within a
> target's map method, using dm_ask_for_duplicate_bios, will cause DM core
> to send duplicates.
> 
> Nothing in DM core actually consumes tio->num_bios -- please advise.
> 
> Thanks,
> Mike

tio->num_bios is a pointer. You don't increase tio->num_bios, you increase 
the value it points to. It points to the local variable that is the limit 
of the for loop.

Mikulas

Re: [PATCH 6/6] dm: introduce dm_ask_for_duplicate_bios

[Mike Snitzer]

On Mon, Apr 28 2014 at 11:09am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Fri, 25 Apr 2014, Mike Snitzer wrote:
> > 
> > Hey Mikulas,
> > 
> > I'm not immediately seeing how increasing tio->num_bios from within a
> > target's map method, using dm_ask_for_duplicate_bios, will cause DM core
> > to send duplicates.
> > 
> > Nothing in DM core actually consumes tio->num_bios -- please advise.
> > 
> > Thanks,
> > Mike
> 
> tio->num_bios is a pointer. You don't increase tio->num_bios, you increase 
> the value it points to. It points to the local variable that is the limit 
> of the for loop.

I see, thanks for clarifying.