[RFC PATCH 0/7] dm-mpath: Do not clone requests

[RFC PATCH 0/7] dm-mpath: Do not clone requests

[Hannes Reinecke]

Hi all,

this is a patchset I made some time ago to remove the
'insane request cloning' from dm-multipath.

Currently, multipath clones each request + attached bios before sending
them down to the underlying path. This not only increases the memory
requirements, but also leads to quite some interesting interactions
with the block layer.

This RFC pursues an alternative approach, leaving the request as-is
and just modify the rq_end_io handler to call the correct end_io function.
As the rq_end_io is called _before_ any bio_endio handler there is no
need to clone the bios nor the bio_endio handler.

In theory. This is just a test balloon to get some response.
I never got around to actually _test_ it. You have been warned.

Hannes Reinecke (7):
  dm: use dm_rq_target_io as argument for dm_done()
  dm: remove handling of DM_ENDIO_INCOMPLETE
  dm: move rq_completed() out of enclosing functions
  dm: open-code dm_kill_unmapped_request()
  dm: move free_rq_clone() out of dm_unprep_request()
  dm: open-code free_rq_clone()
  dm: do not clone requests

 drivers/md/dm.c               | 337 +++++++++---------------------------------
 include/linux/device-mapper.h |   1 -
 2 files changed, 73 insertions(+), 265 deletions(-)

-- 
1.7.12.4

[PATCH 1/7] dm: use dm_rq_target_io as argument for dm_done()

[Hannes Reinecke]

No functional change.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 455e649..de88fac 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -964,17 +964,17 @@ static void start_queue(struct request_queue *q)
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 
-static void dm_done(struct request *clone, int error, bool mapped)
+static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
+		    bool mapped)
 {
-	int r = error;
-	struct dm_rq_target_io *tio = clone->end_io_data;
+	int r = tio->error;
 	dm_request_endio_fn rq_end_io = NULL;
 
 	if (tio->ti) {
 		rq_end_io = tio->ti->type->rq_end_io;
 
 		if (mapped && rq_end_io)
-			r = rq_end_io(tio->ti, clone, error, &tio->info);
+			r = rq_end_io(tio->ti, clone, tio->error, &tio->info);
 	}
 
 	if (r <= 0)
@@ -1004,7 +1004,7 @@ static void dm_softirq_done(struct request *rq)
 	if (rq->cmd_flags & REQ_FAILED)
 		mapped = false;
 
-	dm_done(clone, tio->error, mapped);
+	dm_done(clone, tio, mapped);
 }
 
 /*
-- 
1.7.12.4

[PATCH 2/7] dm: remove handling of DM_ENDIO_INCOMPLETE

[Hannes Reinecke]

Never used, so no need to handle it.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index de88fac..0fa0884 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -980,9 +980,6 @@ static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
 	if (r <= 0)
 		/* The target wants to complete the I/O */
 		dm_end_request(clone, r);
-	else if (r == DM_ENDIO_INCOMPLETE)
-		/* The target will handle the I/O */
-		return;
 	else if (r == DM_ENDIO_REQUEUE)
 		/* The target wants to requeue the I/O */
 		dm_requeue_unmapped_request(clone);
-- 
1.7.12.4

[PATCH 3/7] dm: move rq_completed() out of enclosing functions

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 0fa0884..f55ca0c 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -880,9 +880,7 @@ static void free_rq_clone(struct request *clone)
  */
 static void dm_end_request(struct request *clone, int error)
 {
-	int rw = rq_data_dir(clone);
 	struct dm_rq_target_io *tio = clone->end_io_data;
-	struct mapped_device *md = tio->md;
 	struct request *rq = tio->orig;
 
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
@@ -900,7 +898,6 @@ static void dm_end_request(struct request *clone, int error)
 
 	free_rq_clone(clone);
 	blk_end_request_all(rq, error);
-	rq_completed(md, rw, true);
 }
 
 static void dm_unprep_request(struct request *rq)
@@ -916,12 +913,8 @@ static void dm_unprep_request(struct request *rq)
 /*
  * Requeue the original request of a clone.
  */
-void dm_requeue_unmapped_request(struct request *clone)
+void dm_requeue_unmapped_request(struct request *rq)
 {
-	int rw = rq_data_dir(clone);
-	struct dm_rq_target_io *tio = clone->end_io_data;
-	struct mapped_device *md = tio->md;
-	struct request *rq = tio->orig;
 	struct request_queue *q = rq->q;
 	unsigned long flags;
 
@@ -930,8 +923,6 @@ void dm_requeue_unmapped_request(struct request *clone)
 	spin_lock_irqsave(q->queue_lock, flags);
 	blk_requeue_request(q, rq);
 	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	rq_completed(md, rw, 0);
 }
 EXPORT_SYMBOL_GPL(dm_requeue_unmapped_request);
 
@@ -969,6 +960,9 @@ static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
 {
 	int r = tio->error;
 	dm_request_endio_fn rq_end_io = NULL;
+	struct mapped_device *md = tio->md;
+	struct request *rq = tio->orig;
+	int rw = rq_data_dir(clone);
 
 	if (tio->ti) {
 		rq_end_io = tio->ti->type->rq_end_io;
@@ -977,13 +971,15 @@ static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
 			r = rq_end_io(tio->ti, clone, tio->error, &tio->info);
 	}
 
-	if (r <= 0)
+	if (r <= 0) {
 		/* The target wants to complete the I/O */
 		dm_end_request(clone, r);
-	else if (r == DM_ENDIO_REQUEUE)
+		rq_completed(md, rw, true);
+	} else if (r == DM_ENDIO_REQUEUE) {
 		/* The target wants to requeue the I/O */
-		dm_requeue_unmapped_request(clone);
-	else {
+		dm_requeue_unmapped_request(rq);
+		rq_completed(md, rw, false);
+	} else {
 		DMWARN("unimplemented target endio return value: %d", r);
 		BUG();
 	}
@@ -1607,6 +1603,8 @@ static int map_request(struct dm_target *ti, struct request *clone,
 {
 	int r, requeued = 0;
 	struct dm_rq_target_io *tio = clone->end_io_data;
+	struct request *rq = tio->orig;
+	int rw = rq_data_dir(clone);
 
 	tio->ti = ti;
 	r = ti->type->map_rq(ti, clone, &tio->info);
@@ -1617,12 +1615,13 @@ static int map_request(struct dm_target *ti, struct request *clone,
 	case DM_MAPIO_REMAPPED:
 		/* The target has remapped the I/O so dispatch it */
 		trace_block_rq_remap(clone->q, clone, disk_devt(dm_disk(md)),
-				     blk_rq_pos(tio->orig));
+				     blk_rq_pos(rq));
 		dm_dispatch_request(clone);
 		break;
 	case DM_MAPIO_REQUEUE:
 		/* The target wants to requeue the I/O */
-		dm_requeue_unmapped_request(clone);
+		dm_requeue_unmapped_request(rq);
+		rq_completed(md, rw, false);
 		requeued = 1;
 		break;
 	default:
-- 
1.7.12.4

[PATCH 4/7] dm: open-code dm_kill_unmapped_request()

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm.c               | 24 +++++-------------------
 include/linux/device-mapper.h |  1 -
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index f55ca0c..5109c76 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1015,22 +1015,6 @@ static void dm_complete_request(struct request *clone, int error)
 }
 
 /*
- * Complete the not-mapped clone and the original request with the error status
- * through softirq context.
- * Target's rq_end_io() function isn't called.
- * This may be used when the target's map_rq() function fails.
- */
-void dm_kill_unmapped_request(struct request *clone, int error)
-{
-	struct dm_rq_target_io *tio = clone->end_io_data;
-	struct request *rq = tio->orig;
-
-	rq->cmd_flags |= REQ_FAILED;
-	dm_complete_request(clone, error);
-}
-EXPORT_SYMBOL_GPL(dm_kill_unmapped_request);
-
-/*
  * Called with the queue lock held
  */
 static void end_clone_request(struct request *clone, int error)
@@ -1631,7 +1615,8 @@ static int map_request(struct dm_target *ti, struct request *clone,
 		}
 
 		/* The target wants to complete the I/O */
-		dm_kill_unmapped_request(clone, r);
+		rq->cmd_flags |= REQ_FAILED;
+		dm_complete_request(clone, r);
 		break;
 	}
 
@@ -1691,11 +1676,12 @@ static void dm_request_fn(struct request_queue *q)
 		if (!dm_target_is_valid(ti)) {
 			/*
 			 * Must perform setup, that dm_done() requires,
-			 * before calling dm_kill_unmapped_request
+			 * before calling dm_complete_request
 			 */
 			DMERR_LIMIT("request attempted access beyond the end of device");
 			clone = dm_start_request(md, rq);
-			dm_kill_unmapped_request(clone, -EIO);
+			rq->cmd_flags |= REQ_FAILED;
+			dm_complete_request(clone, -EIO);
 			continue;
 		}
 
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 63da56e..3ad5db8 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -604,7 +604,6 @@ static inline unsigned long to_bytes(sector_t n)
  *---------------------------------------------------------------*/
 void dm_dispatch_request(struct request *rq);
 void dm_requeue_unmapped_request(struct request *rq);
-void dm_kill_unmapped_request(struct request *rq, int error);
 int dm_underlying_device_busy(struct request_queue *q);
 
 #endif	/* _LINUX_DEVICE_MAPPER_H */
-- 
1.7.12.4

[PATCH 5/7] dm: move free_rq_clone() out of dm_unprep_request()

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 5109c76..b2240f7 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -902,12 +902,8 @@ static void dm_end_request(struct request *clone, int error)
 
 static void dm_unprep_request(struct request *rq)
 {
-	struct request *clone = rq->special;
-
 	rq->special = NULL;
 	rq->cmd_flags &= ~REQ_DONTPREP;
-
-	free_rq_clone(clone);
 }
 
 /*
@@ -977,6 +973,7 @@ static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
 		rq_completed(md, rw, true);
 	} else if (r == DM_ENDIO_REQUEUE) {
 		/* The target wants to requeue the I/O */
+		free_rq_clone(clone);
 		dm_requeue_unmapped_request(rq);
 		rq_completed(md, rw, false);
 	} else {
@@ -1604,6 +1601,7 @@ static int map_request(struct dm_target *ti, struct request *clone,
 		break;
 	case DM_MAPIO_REQUEUE:
 		/* The target wants to requeue the I/O */
+		free_rq_clone(clone);
 		dm_requeue_unmapped_request(rq);
 		rq_completed(md, rw, false);
 		requeued = 1;
-- 
1.7.12.4

[PATCH 6/7] dm: open-code free_rq_clone()

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index b2240f7..4da9941 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -866,23 +866,13 @@ static void rq_completed(struct mapped_device *md, int rw, int run_queue)
 	dm_put(md);
 }
 
-static void free_rq_clone(struct request *clone)
-{
-	struct dm_rq_target_io *tio = clone->end_io_data;
-
-	blk_rq_unprep_clone(clone);
-	free_rq_tio(tio);
-}
-
 /*
  * Complete the clone and the original request.
  * Must be called without queue lock.
  */
-static void dm_end_request(struct request *clone, int error)
+static void dm_end_request(struct request *clone, struct request *rq,
+			   int error)
 {
-	struct dm_rq_target_io *tio = clone->end_io_data;
-	struct request *rq = tio->orig;
-
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
 		rq->errors = clone->errors;
 		rq->resid_len = clone->resid_len;
@@ -896,7 +886,6 @@ static void dm_end_request(struct request *clone, int error)
 			rq->sense_len = clone->sense_len;
 	}
 
-	free_rq_clone(clone);
 	blk_end_request_all(rq, error);
 }
 
@@ -967,14 +956,16 @@ static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
 			r = rq_end_io(tio->ti, clone, tio->error, &tio->info);
 	}
 
+	free_rq_tio(tio);
 	if (r <= 0) {
 		/* The target wants to complete the I/O */
-		dm_end_request(clone, r);
+		dm_end_request(clone, rq, r);
+		blk_rq_unprep_clone(clone);
 		rq_completed(md, rw, true);
 	} else if (r == DM_ENDIO_REQUEUE) {
 		/* The target wants to requeue the I/O */
-		free_rq_clone(clone);
 		dm_requeue_unmapped_request(rq);
+		blk_rq_unprep_clone(clone);
 		rq_completed(md, rw, false);
 	} else {
 		DMWARN("unimplemented target endio return value: %d", r);
@@ -1601,7 +1592,8 @@ static int map_request(struct dm_target *ti, struct request *clone,
 		break;
 	case DM_MAPIO_REQUEUE:
 		/* The target wants to requeue the I/O */
-		free_rq_clone(clone);
+		free_rq_tio(tio);
+		blk_rq_unprep_clone(clone);
 		dm_requeue_unmapped_request(rq);
 		rq_completed(md, rw, false);
 		requeued = 1;
-- 
1.7.12.4

[PATCH 7/7] dm: do not clone requests

[Hannes Reinecke]

Instead of cloning the request we should pass it down directly.
This saves a memory allocation and avoid the usage of the 'special'
pointer.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm.c | 273 ++++++++++++--------------------------------------------
 1 file changed, 55 insertions(+), 218 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 4da9941..5b0dc00 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -75,25 +75,13 @@ struct dm_io {
 struct dm_rq_target_io {
 	struct mapped_device *md;
 	struct dm_target *ti;
-	struct request *orig, clone;
+	struct request_queue *q;
+	rq_end_io_fn *end_io;
+	void *end_io_data;
 	int error;
 	union map_info info;
 };
 
-/*
- * For request-based dm - the bio clones we allocate are embedded in these
- * structs.
- *
- * We allocate these with bio_alloc_bioset, using the front_pad parameter when
- * the bioset is created - this means the bio has to come at the end of the
- * struct.
- */
-struct dm_rq_clone_bio_info {
-	struct bio *orig;
-	struct dm_rq_target_io *tio;
-	struct bio clone;
-};
-
 union map_info *dm_get_rq_mapinfo(struct request *rq)
 {
 	if (rq && rq->end_io_data)
@@ -788,57 +776,6 @@ static void clone_endio(struct bio *bio, int error)
 }
 
 /*
- * Partial completion handling for request-based dm
- */
-static void end_clone_bio(struct bio *clone, int error)
-{
-	struct dm_rq_clone_bio_info *info =
-		container_of(clone, struct dm_rq_clone_bio_info, clone);
-	struct dm_rq_target_io *tio = info->tio;
-	struct bio *bio = info->orig;
-	unsigned int nr_bytes = info->orig->bi_iter.bi_size;
-
-	bio_put(clone);
-
-	if (tio->error)
-		/*
-		 * An error has already been detected on the request.
-		 * Once error occurred, just let clone->end_io() handle
-		 * the remainder.
-		 */
-		return;
-	else if (error) {
-		/*
-		 * Don't notice the error to the upper layer yet.
-		 * The error handling decision is made by the target driver,
-		 * when the request is completed.
-		 */
-		tio->error = error;
-		return;
-	}
-
-	/*
-	 * I/O for the bio successfully completed.
-	 * Notice the data completion to the upper layer.
-	 */
-
-	/*
-	 * bios are processed from the head of the list.
-	 * So the completing bio should always be rq->bio.
-	 * If it's not, something wrong is happening.
-	 */
-	if (tio->orig->bio != bio)
-		DMERR("bio completion is going in the middle of the request");
-
-	/*
-	 * Update the original request.
-	 * Do not use blk_end_request() here, because it may complete
-	 * the original request before the clone, and break the ordering.
-	 */
-	blk_update_request(tio->orig, 0, nr_bytes);
-}
-
-/*
  * Don't touch any member of the md after calling this function because
  * the md may be freed in dm_put() at the end of this function.
  * Or do dm_get() before calling this function and dm_put() later.
@@ -867,35 +804,6 @@ static void rq_completed(struct mapped_device *md, int rw, int run_queue)
 }
 
 /*
- * Complete the clone and the original request.
- * Must be called without queue lock.
- */
-static void dm_end_request(struct request *clone, struct request *rq,
-			   int error)
-{
-	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
-		rq->errors = clone->errors;
-		rq->resid_len = clone->resid_len;
-
-		if (rq->sense)
-			/*
-			 * We are using the sense buffer of the original
-			 * request.
-			 * So setting the length of the sense data is enough.
-			 */
-			rq->sense_len = clone->sense_len;
-	}
-
-	blk_end_request_all(rq, error);
-}
-
-static void dm_unprep_request(struct request *rq)
-{
-	rq->special = NULL;
-	rq->cmd_flags &= ~REQ_DONTPREP;
-}
-
-/*
  * Requeue the original request of a clone.
  */
 void dm_requeue_unmapped_request(struct request *rq)
@@ -903,8 +811,6 @@ void dm_requeue_unmapped_request(struct request *rq)
 	struct request_queue *q = rq->q;
 	unsigned long flags;
 
-	dm_unprep_request(rq);
-
 	spin_lock_irqsave(q->queue_lock, flags);
 	blk_requeue_request(q, rq);
 	spin_unlock_irqrestore(q->queue_lock, flags);
@@ -940,32 +846,29 @@ static void start_queue(struct request_queue *q)
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 
-static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
+static void dm_done(struct request *rq, struct dm_rq_target_io *tio,
 		    bool mapped)
 {
 	int r = tio->error;
 	dm_request_endio_fn rq_end_io = NULL;
 	struct mapped_device *md = tio->md;
-	struct request *rq = tio->orig;
-	int rw = rq_data_dir(clone);
+	int rw = rq_data_dir(rq);
 
 	if (tio->ti) {
 		rq_end_io = tio->ti->type->rq_end_io;
 
 		if (mapped && rq_end_io)
-			r = rq_end_io(tio->ti, clone, tio->error, &tio->info);
+			r = rq_end_io(tio->ti, rq, tio->error, &tio->info);
 	}
 
 	free_rq_tio(tio);
 	if (r <= 0) {
 		/* The target wants to complete the I/O */
-		dm_end_request(clone, rq, r);
-		blk_rq_unprep_clone(clone);
+		blk_end_request_all(rq, r);
 		rq_completed(md, rw, true);
 	} else if (r == DM_ENDIO_REQUEUE) {
 		/* The target wants to requeue the I/O */
 		dm_requeue_unmapped_request(rq);
-		blk_rq_unprep_clone(clone);
 		rq_completed(md, rw, false);
 	} else {
 		DMWARN("unimplemented target endio return value: %d", r);
@@ -979,54 +882,38 @@ static void dm_done(struct request *clone, struct dm_rq_target_io *tio,
 static void dm_softirq_done(struct request *rq)
 {
 	bool mapped = true;
-	struct request *clone = rq->completion_data;
-	struct dm_rq_target_io *tio = clone->end_io_data;
+	struct dm_rq_target_io *tio = rq->completion_data;
 
-	if (rq->cmd_flags & REQ_FAILED)
+	if (rq->cmd_flags & REQ_FAILED) {
 		mapped = false;
+		rq->cmd_flags &= ~REQ_FAILED;
+	}
 
-	dm_done(clone, tio, mapped);
+	dm_done(rq, tio, mapped);
 }
 
 /*
- * Complete the clone and the original request with the error status
+ * Complete the remapped request with the error status
  * through softirq context.
+ *
+ * Called with the queue lock held
  */
-static void dm_complete_request(struct request *clone, int error)
+static void dm_complete_request(struct request *rq, int error)
 {
-	struct dm_rq_target_io *tio = clone->end_io_data;
-	struct request *rq = tio->orig;
+	struct dm_rq_target_io *tio = rq->end_io_data;
 
 	tio->error = error;
-	rq->completion_data = clone;
+	rq->completion_data = tio;
+	rq->end_io = tio->end_io;
+	rq->end_io_data = tio->end_io_data;
+	rq->q = tio->q;
+	tio->end_io = NULL;
+	tio->end_io_data = NULL;
+	tio->q = NULL;
 	blk_complete_request(rq);
 }
 
 /*
- * Called with the queue lock held
- */
-static void end_clone_request(struct request *clone, int error)
-{
-	/*
-	 * For just cleaning up the information of the queue in which
-	 * the clone was dispatched.
-	 * The clone is *NOT* freed actually here because it is alloced from
-	 * dm own mempool and REQ_ALLOCED isn't set in clone->cmd_flags.
-	 */
-	__blk_put_request(clone->q, clone);
-
-	/*
-	 * Actual request completion is done in a softirq context which doesn't
-	 * hold the queue lock.  Otherwise, deadlock could occur because:
-	 *     - another request may be submitted by the upper level driver
-	 *       of the stacking during the completion
-	 *     - the submission which requires queue lock may be done
-	 *       against this queue
-	 */
-	dm_complete_request(clone, error);
-}
-
-/*
  * Return maximum size of I/O possible at the supplied sector up to the current
  * target boundary.
  */
@@ -1482,64 +1369,27 @@ void dm_dispatch_request(struct request *rq)
 }
 EXPORT_SYMBOL_GPL(dm_dispatch_request);
 
-static int dm_rq_bio_constructor(struct bio *bio, struct bio *bio_orig,
-				 void *data)
+static int setup_rq(struct request *rq, struct mapped_device *md,
+		    gfp_t gfp_mask)
 {
-	struct dm_rq_target_io *tio = data;
-	struct dm_rq_clone_bio_info *info =
-		container_of(bio, struct dm_rq_clone_bio_info, clone);
-
-	info->orig = bio_orig;
-	info->tio = tio;
-	bio->bi_end_io = end_clone_bio;
-
-	return 0;
-}
-
-static int setup_clone(struct request *clone, struct request *rq,
-		       struct dm_rq_target_io *tio)
-{
-	int r;
-
-	r = blk_rq_prep_clone(clone, rq, tio->md->bs, GFP_ATOMIC,
-			      dm_rq_bio_constructor, tio);
-	if (r)
-		return r;
-
-	clone->cmd = rq->cmd;
-	clone->cmd_len = rq->cmd_len;
-	clone->sense = rq->sense;
-	clone->buffer = rq->buffer;
-	clone->end_io = end_clone_request;
-	clone->end_io_data = tio;
-
-	return 0;
-}
-
-static struct request *clone_rq(struct request *rq, struct mapped_device *md,
-				gfp_t gfp_mask)
-{
-	struct request *clone;
 	struct dm_rq_target_io *tio;
 
 	tio = alloc_rq_tio(md, gfp_mask);
 	if (!tio)
-		return NULL;
+		return -ENOMEM;
 
 	tio->md = md;
 	tio->ti = NULL;
-	tio->orig = rq;
+	tio->q = rq->q;
+	tio->end_io = rq->end_io;
+	tio->end_io_data = rq->end_io_data;
 	tio->error = 0;
 	memset(&tio->info, 0, sizeof(tio->info));
 
-	clone = &tio->clone;
-	if (setup_clone(clone, rq, tio)) {
-		/* -ENOMEM */
-		free_rq_tio(tio);
-		return NULL;
-	}
+	rq->end_io = dm_complete_request;
+	rq->end_io_data = tio;
 
-	return clone;
+	return 0;
 }
 
 /*
@@ -1548,18 +1398,10 @@ static struct request *clone_rq(struct request *rq, struct mapped_device *md,
 static int dm_prep_fn(struct request_queue *q, struct request *rq)
 {
 	struct mapped_device *md = q->queuedata;
-	struct request *clone;
 
-	if (unlikely(rq->special)) {
-		DMWARN("Already has something in rq->special.");
-		return BLKPREP_KILL;
-	}
-
-	clone = clone_rq(rq, md, GFP_ATOMIC);
-	if (!clone)
+	if (setup_rq(rq, md, GFP_ATOMIC) < 0)
 		return BLKPREP_DEFER;
 
-	rq->special = clone;
 	rq->cmd_flags |= REQ_DONTPREP;
 
 	return BLKPREP_OK;
@@ -1570,30 +1412,28 @@ static int dm_prep_fn(struct request_queue *q, struct request *rq)
  * 0  : the request has been processed (not requeued)
  * !0 : the request has been requeued
  */
-static int map_request(struct dm_target *ti, struct request *clone,
+static int map_request(struct dm_target *ti, struct request *rq,
 		       struct mapped_device *md)
 {
 	int r, requeued = 0;
-	struct dm_rq_target_io *tio = clone->end_io_data;
-	struct request *rq = tio->orig;
-	int rw = rq_data_dir(clone);
+	struct dm_rq_target_io *tio = rq->end_io_data;
+	int rw = rq_data_dir(rq);
 
 	tio->ti = ti;
-	r = ti->type->map_rq(ti, clone, &tio->info);
+	r = ti->type->map_rq(ti, rq, &tio->info);
 	switch (r) {
 	case DM_MAPIO_SUBMITTED:
 		/* The target has taken the I/O to submit by itself later */
 		break;
 	case DM_MAPIO_REMAPPED:
 		/* The target has remapped the I/O so dispatch it */
-		trace_block_rq_remap(clone->q, clone, disk_devt(dm_disk(md)),
+		trace_block_rq_remap(rq->q, rq, disk_devt(dm_disk(md)),
 				     blk_rq_pos(rq));
-		dm_dispatch_request(clone);
+		dm_dispatch_request(rq);
 		break;
 	case DM_MAPIO_REQUEUE:
 		/* The target wants to requeue the I/O */
 		free_rq_tio(tio);
-		blk_rq_unprep_clone(clone);
 		dm_requeue_unmapped_request(rq);
 		rq_completed(md, rw, false);
 		requeued = 1;
@@ -1606,20 +1446,17 @@ static int map_request(struct dm_target *ti, struct request *clone,
 
 		/* The target wants to complete the I/O */
 		rq->cmd_flags |= REQ_FAILED;
-		dm_complete_request(clone, r);
+		dm_complete_request(rq, r);
 		break;
 	}
 
 	return requeued;
 }
 
-static struct request *dm_start_request(struct mapped_device *md, struct request *orig)
+static void dm_start_request(struct mapped_device *md, struct request *rq)
 {
-	struct request *clone;
-
-	blk_start_request(orig);
-	clone = orig->special;
-	atomic_inc(&md->pending[rq_data_dir(clone)]);
+	blk_start_request(rq);
+	atomic_inc(&md->pending[rq_data_dir(rq)]);
 
 	/*
 	 * Hold the md reference here for the in-flight I/O.
@@ -1629,8 +1466,6 @@ static struct request *dm_start_request(struct mapped_device *md, struct request
 	 * See the comment in rq_completed() too.
 	 */
 	dm_get(md);
-
-	return clone;
 }
 
 /*
@@ -1643,7 +1478,7 @@ static void dm_request_fn(struct request_queue *q)
 	int srcu_idx;
 	struct dm_table *map = dm_get_live_table(md, &srcu_idx);
 	struct dm_target *ti;
-	struct request *rq, *clone;
+	struct request *rq;
 	sector_t pos;
 
 	/*
@@ -1669,19 +1504,19 @@ static void dm_request_fn(struct request_queue *q)
 			 * before calling dm_complete_request
 			 */
 			DMERR_LIMIT("request attempted access beyond the end of device");
-			clone = dm_start_request(md, rq);
+			dm_start_request(md, rq);
 			rq->cmd_flags |= REQ_FAILED;
-			dm_complete_request(clone, -EIO);
+			dm_complete_request(rq, -EIO);
 			continue;
 		}
 
 		if (ti->type->busy && ti->type->busy(ti))
 			goto delay_and_out;
 
-		clone = dm_start_request(md, rq);
+		dm_start_request(md, rq);
 
 		spin_unlock(q->queue_lock);
-		if (map_request(ti, clone, md))
+		if (map_request(ti, rq, md))
 			goto requeued;
 
 		BUG_ON(!irqs_disabled());
@@ -2794,7 +2629,7 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity, u
 	} else if (type == DM_TYPE_REQUEST_BASED) {
 		cachep = _rq_tio_cache;
 		pool_size = dm_get_reserved_rq_based_ios();
-		front_pad = offsetof(struct dm_rq_clone_bio_info, clone);
+		front_pad = 0;
 		/* per_bio_data_size is not used. See __bind_mempools(). */
 		WARN_ON(per_bio_data_size != 0);
 	} else
@@ -2804,9 +2639,11 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity, u
 	if (!pools->io_pool)
 		goto out;
 
-	pools->bs = bioset_create(pool_size, front_pad);
-	if (!pools->bs)
-		goto out;
+	if (front_pad) {
+		pools->bs = bioset_create(pool_size, front_pad);
+		if (!pools->bs)
+			goto out;
+	}
 
 	if (integrity && bioset_integrity_create(pools->bs, pool_size))
 		goto out;
-- 
1.7.12.4

Re: [RFC PATCH 0/7] dm-mpath: Do not clone requests

[Christoph Hellwig]

Oh, you're not even cloning the request.  I though you'd just avoid
cloning the bios.  Passing the requests through isn't going to work
when sitting on top of a blk-mq driver.

I have a queue I'm trying to start to test now that approaches this
a little bit differently:

 - request based dm is converted to use blk-mq itself, allowing us to
   allocate private data as part of the incoming request, and gets
   rid of the nasty prep_fn/request_fn split
 - it then just allocates a new request on the underlying device after
   chosing the path.  By using blk_get_request to allocate the lower
   request dm-mpath doesn't care if the underlying device uses blk-mq
   or not.

As said I'm already running into issues with plain dm mpath in my
trivial test setup, so this is stalled for the moment.

But I'd still love to understand why dm even bothers cloning the bios.
At the request layer we only touch the bios in two places: first
for merging in into the request, and second in blk_update_request.

Now with dm-mpath we'd never want to do this sort of merging for the
lower request anyway, and I don't see a real problem keeting the lower
driver complete the bio and just never call blk_update_request in
dm-mpath either.  At least that's my impression that hasn't made contact
with the ugly reality yet..

Re: [RFC PATCH 0/7] dm-mpath: Do not clone requests

[Hannes Reinecke]

On 06/05/2014 03:36 PM, Christoph Hellwig wrote:
> Oh, you're not even cloning the request.  I though you'd just avoid
> cloning the bios.  Passing the requests through isn't going to work
> when sitting on top of a blk-mq driver.
>
Ok, might be.

It just seemed the obvious thing to do. But if there are arguments
against it one should rework that bit.

> I have a queue I'm trying to start to test now that approaches this
> a little bit differently:
>
>   - request based dm is converted to use blk-mq itself, allowing us to
>     allocate private data as part of the incoming request, and gets
>     rid of the nasty prep_fn/request_fn split
>   - it then just allocates a new request on the underlying device after
>     chosing the path.  By using blk_get_request to allocate the lower
>     request dm-mpath doesn't care if the underlying device uses blk-mq
>     or not.
>
> As said I'm already running into issues with plain dm mpath in my
> trivial test setup, so this is stalled for the moment.
>
> But I'd still love to understand why dm even bothers cloning the bios.
> At the request layer we only touch the bios in two places: first
> for merging in into the request, and second in blk_update_request.
>
> Now with dm-mpath we'd never want to do this sort of merging for the
> lower request anyway, and I don't see a real problem keeting the lower
> driver complete the bio and just never call blk_update_request in
> dm-mpath either.  At least that's my impression that hasn't made contact
> with the ugly reality yet..
>
And which was precisely why I haven't continued with this patchset, too.
The primary reason for cloning the bios would from my POV would be 
so that we can handle partial completion properly. When eliminating 
it we would be always return an error here.
But then one does wonder whether a partial completion shouldn't be 
considered an error anyway.
I've never felt comfortable with that bit in the stack, and tried to 
shirk it as much as possible.

Maybe Mike C. has some ideas here; I seem to remember he once worked 
on that code ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [RFC PATCH 0/7] dm-mpath: Do not clone requests

[Christoph Hellwig]

On Thu, Jun 05, 2014 at 03:55:29PM +0200, Hannes Reinecke wrote:
> The primary reason for cloning the bios would from my POV would be
> so that we can handle partial completion properly. When eliminating
> it we would be always return an error here.

I don't see how partial completions matter - we never finish a partial
request in the low level driver and let DM retry.  All partial
completion handling stays in the low level driver.

If we'd didn't clone bios the workflow would look something like this:

 - allocate new clone request in dm-mpath
 - point clone->bio and clone->biotail to the original bio, zero
   them out in the original request
 - the low level driver does one or more calls to blk_update_request
   until clone->bio is NULL, and then calls blk_finish_request (or
   __blk_mq_end_io) once the request has been completed,
 - This hands control back to dm-mpath, which can now call
   __blk_mq_end_io without blk_update_request as the low level driver
   took care of the bio completions.

Re: [RFC PATCH 0/7] dm-mpath: Do not clone requests

[Junichi Nomura]

On 06/05/14 23:44, Christoph Hellwig wrote:
> If we'd didn't clone bios the workflow would look something like this:
> 
>  - allocate new clone request in dm-mpath
>  - point clone->bio and clone->biotail to the original bio, zero
>    them out in the original request
>  - the low level driver does one or more calls to blk_update_request

At this point, if blk_update_request is called with error code,
the error is returned straight up to submitter of original bio
before dm-mpath can try other paths.

>    until clone->bio is NULL, and then calls blk_finish_request (or
>    __blk_mq_end_io) once the request has been completed,
>  - This hands control back to dm-mpath, which can now call
>    __blk_mq_end_io without blk_update_request as the low level driver
>    took care of the bio completions.

Primary reason of cloning bios was to intercept lower-layer errors
in dm-mpath for path failover decision.
If we only cloned requests, original bios were already completed
at the time blk_finish_request was called for the clone.

Other implementations were discussed back then.

blk_update_request could skip completing bios until blk_finish_request
is called. But it would become inefficient in the case of partial completion.

A callback hook could be added to blk_update_request.
However such an additional hook was considered as bad approach.

-- 
Jun'ichi Nomura, NEC Corporation

Re: [RFC PATCH 0/7] dm-mpath: Do not clone requests

[Mike Snitzer]

On Thu, Jun 05 2014 at  9:36am -0400,
Christoph Hellwig <hch@infradead.org> wrote:

> Oh, you're not even cloning the request.  I though you'd just avoid
> cloning the bios.  Passing the requests through isn't going to work
> when sitting on top of a blk-mq driver.
> 
> I have a queue I'm trying to start to test now that approaches this
> a little bit differently:
> 
>  - request based dm is converted to use blk-mq itself, allowing us to
>    allocate private data as part of the incoming request, and gets
>    rid of the nasty prep_fn/request_fn split
>  - it then just allocates a new request on the underlying device after
>    chosing the path.  By using blk_get_request to allocate the lower
>    request dm-mpath doesn't care if the underlying device uses blk-mq
>    or not.

Interested to know how you'll ensure we'll never block on
blk_get_request() waiting for memory?  (something beyond gfp flags, more
context below).

> As said I'm already running into issues with plain dm mpath in my
> trivial test setup, so this is stalled for the moment.
> 
> But I'd still love to understand why dm even bothers cloning the bios.
> At the request layer we only touch the bios in two places: first
> for merging in into the request, and second in blk_update_request.

Junichi may have more context on "why?".  But the bio cloning is really
expensive, particularly due to the mempool reserves we keep on hand to
be able to avoid deadlock.

With the current rq-based DM we try to have enough memory available (per
DM device) to accomodate cloning a single request so that forward
progress can be made.  But we don't know how many bios are associated
with a given request so our reserves _seem_ excessive to cover something
approaching worst case (but we fall short of covering even that as a
tradeoff).

> Now with dm-mpath we'd never want to do this sort of merging for the
> lower request anyway, and I don't see a real problem keeting the lower
> driver complete the bio and just never call blk_update_request in
> dm-mpath either.  At least that's my impression that hasn't made contact
> with the ugly reality yet..

Definitely needs further review.

Re: [RFC PATCH 0/7] dm-mpath: Do not clone requests

[Christoph Hellwig]

On Thu, Jun 05, 2014 at 10:01:50AM -0400, Mike Snitzer wrote:
> Interested to know how you'll ensure we'll never block on
> blk_get_request() waiting for memory?  (something beyond gfp flags, more
> context below).

For requests it's pretty easy - we just do the non-blocking request
allocation on the low level driver, and that's either get us one or
fail.  The bios are the interesting part, that's why I'd love to
get rid of the cloning.