block, dm: don't copy bios for request clones
Subsystems (2)
PATCH v2 1/1 block, dm: don't copy bios for request clones
Currently dm-multipath has to clone the bios for every request sent to the lower devices, which wastes cpu cycles and ties down memory.
This patch instead adds a new REQ_CLONE flag that instructs req_bio_endio to not complete bios attached to a request, which we set on clone requests similar to bios in a flush sequence. With this change I/O errors on a path failure only get propagated to dm-multipath, which can then either resubmit the I/O or complete the bios on the original request.
I've done some basic testing of this on a Linux target with ALUA support, and it survives path failures during I/O nicely.
--- block/blk-core.c | 94 +++---------------------- drivers/md/dm-table.c | 28 +++++--- drivers/md/dm.c | 173 +++++++++++----------------------------------- drivers/md/dm.h | 6 +- include/linux/blk_types.h | 2 + include/linux/blkdev.h | 6 +- 6 files changed, 78 insertions(+), 231 deletions(-)
block/blk-core.c +11 -83
@@ -117,7 +117,7 @@ EXPORT_SYMBOL(blk_rq_init);static void req_bio_endio (struct request *rq,struct bio *bio,unsigned int nbytes,int error) { -if (error) +if (error && !(rq->cmd_flags &REQ_CLONE ))clear_bit (BIO_UPTODATE , &bio->bi_flags);else if (!test_bit (BIO_UPTODATE , &bio->bi_flags)) error = -EIO ;
@@ -128,7 +128,8 @@ static void req_bio_endio(struct request *rq, struct bio *bio,bio_advance (bio, nbytes);/* don't actually finish bio if it's part of flush sequence */ -if (bio->bi_iter.bi_size ==0 && !(rq->cmd_flags &REQ_FLUSH_SEQ )) +if (bio->bi_iter.bi_size ==0 && + !(rq->cmd_flags & (REQ_FLUSH_SEQ |REQ_CLONE )))bio_endio (bio, error); }
@@ -2901,95 +2902,22 @@ int blk_lld_busy(struct request_queue *q) }EXPORT_SYMBOL_GPL (blk_lld_busy); -/** -* blk_rq_unprep_clone - Helper function to free all bios in a cloned request -* @rq: the clone request to be cleaned up -* -* Description: -* Free all bios in @rq for a cloned request. -*/ -void blk_rq_unprep_clone (struct request *rq) -{ -struct bio *bio; - -while ((bio = rq->bio) !=NULL ) { - rq->bio = bio->bi_next; - -bio_put (bio); - } -} -EXPORT_SYMBOL_GPL (blk_rq_unprep_clone); - -/* -* Copy attributes of the original request to the clone request. -* The actual data parts (e.g. ->cmd, ->sense) are not copied. -*/ -static void __blk_rq_prep_clone (struct request *dst,struct request *src) +void blk_rq_prep_clone (struct request *dst,struct request *src) { dst->cpu = src->cpu; - dst->cmd_flags |= (src->cmd_flags &REQ_CLONE_MASK ) |REQ_NOMERGE ; + dst->cmd_flags |= (src->cmd_flags &REQ_CLONE_MASK ); + dst->cmd_flags |=REQ_NOMERGE |REQ_CLONE ; dst->cmd_type = src->cmd_type; dst->__sector =blk_rq_pos (src); dst->__data_len =blk_rq_bytes (src); dst->nr_phys_segments = src->nr_phys_segments; dst->ioprio = src->ioprio; dst->extra_len = src->extra_len; -} - -/** -* blk_rq_prep_clone - Helper function to setup clone request -* @rq: the request to be setup -* @rq_src: original request to be cloned -* @bs: bio_set that bios for clone are allocated from -* @gfp_mask: memory allocation mask for bio -* @bio_ctr: setup function to be called for each clone bio. -* Returns %0 for success, non %0 for failure. -* @data: private data to be passed to @bio_ctr -* -* Description: -* Clones bios in @rq_src to @rq, and copies attributes of @rq_src to @rq. -* The actual data parts of @rq_src (e.g. ->cmd, ->sense) -* are not copied, and copying such parts is the caller's responsibility. -* Also, pages which the original bios are pointing to are not copied -* and the cloned bios just point same pages. -* So cloned bios must be completed before original bios, which means -* the caller must complete @rq before @rq_src. -*/ -int blk_rq_prep_clone (struct request *rq,struct request *rq_src, -struct bio_set *bs,gfp_t gfp_mask, -int (*bio_ctr)(struct bio *,struct bio *,void *), -void *data) -{ -struct bio *bio, *bio_src; - -if (!bs) - bs = fs_bio_set; - -__rq_for_each_bio (bio_src, rq_src) { - bio =bio_clone_fast (bio_src, gfp_mask, bs); -if (!bio) - goto free_and_out; - -if (bio_ctr &&bio_ctr (bio, bio_src, data)) - goto free_and_out; - -if (rq->bio) { - rq->biotail->bi_next = bio; - rq->biotail = bio; - }else - rq->bio = rq->biotail = bio; - } - -__blk_rq_prep_clone (rq, rq_src); - -return 0 ; - -free_and_out: -if (bio) -bio_put (bio); -blk_rq_unprep_clone (rq); - -return -ENOMEM ; + dst->bio = src->bio; + dst->biotail = src->biotail; + dst->cmd = src->cmd; + dst->cmd_len = src->cmd_len; + dst->sense = src->sense; }EXPORT_SYMBOL_GPL (blk_rq_prep_clone);
drivers/md/dm-table.c +18 -10
@@ -938,23 +938,31 @@ bool dm_table_mq_request_based(struct dm_table *t)static int dm_table_alloc_md_mempools (struct dm_table *t,struct mapped_device *md) { -unsigned type =dm_table_get_type (t); +int type =dm_table_get_type (t);unsigned per_bio_data_size =0 ; -struct dm_target *tgt;unsigned i; -if (unlikely (type ==DM_TYPE_NONE )) { +switch (type) { +case DM_TYPE_BIO_BASED : +for (i =0 ; i < t->num_targets; i++) { +struct dm_target *tgt = t->targets + i; + + per_bio_data_size =max (per_bio_data_size, + tgt->per_bio_data_size); + } + + t->mempools =dm_alloc_bio_mempools (t->integrity_supported, + per_bio_data_size); +break ; +case DM_TYPE_REQUEST_BASED : +case DM_TYPE_MQ_REQUEST_BASED : + t->mempools =dm_alloc_rq_mempools (md, type); +break ; +default :DMWARN ("no table type is set, can't allocate mempools" );return -EINVAL ; } -if (type ==DM_TYPE_BIO_BASED ) -for (i =0 ; i < t->num_targets; i++) { - tgt = t->targets + i; - per_bio_data_size =max (per_bio_data_size, tgt->per_bio_data_size); - } - - t->mempools =dm_alloc_md_mempools (md, type, t->integrity_supported, per_bio_data_size);if (!t->mempools)return -ENOMEM ;
drivers/md/dm.c +42 -131
233 lines not on this page · raw message
drivers/md/dm.h +4 -2
@@ -222,8 +222,10 @@ void dm_kcopyd_exit(void);/* * Mempool operations */ -struct dm_md_mempools *dm_alloc_md_mempools (struct mapped_device *md,unsigned type, -unsigned integrity,unsigned per_bio_data_size); +struct dm_md_mempools *dm_alloc_bio_mempools (unsigned integrity, +unsigned per_bio_data_size); +struct dm_md_mempools *dm_alloc_rq_mempools (struct mapped_device *md, +unsigned type);void dm_free_md_mempools (struct dm_md_mempools *pools);/*
include/linux/blk_types.h +2 -0
@@ -193,6 +193,7 @@ enum rq_flag_bits { __REQ_HASHED,/* on IO scheduler merge hash */ __REQ_MQ_INFLIGHT,/* track inflight for MQ */ __REQ_NO_TIMEOUT,/* requests may never expire */ + __REQ_CLONE,/* cloned bios */ __REQ_NR_BITS, /* stops here */ };
@@ -247,5 +248,6 @@ enum rq_flag_bits {#define REQ_HASHED (1ULL << __REQ_HASHED)#define REQ_MQ_INFLIGHT (1ULL << __REQ_MQ_INFLIGHT)#define REQ_NO_TIMEOUT (1ULL << __REQ_NO_TIMEOUT) +#define REQ_CLONE (1ULL << __REQ_CLONE)#endif /* __LINUX_BLK_TYPES_H */
include/linux/blkdev.h +1 -5
@@ -804,11 +804,7 @@ extern void blk_add_request_payload(struct request *rq, struct page *page, unsigned int len);extern int blk_rq_check_limits (struct request_queue *q,struct request *rq);extern int blk_lld_busy (struct request_queue *q); -extern int blk_rq_prep_clone (struct request *rq,struct request *rq_src, -struct bio_set *bs,gfp_t gfp_mask, -int (*bio_ctr)(struct bio *,struct bio *,void *), -void *data); -extern void blk_rq_unprep_clone (struct request *rq); +extern void blk_rq_prep_clone (struct request *rq,struct request *rq_src);extern int blk_insert_cloned_request (struct request_queue *q,struct request *rq);extern void blk_delay_queue (struct request_queue *,unsigned long );
Signature · 1 line
1.9.1
Quoted 10 lines · Christoph Hellwig
Currently dm-multipath has to clone the bios for every request sent to the lower devices, which wastes cpu cycles and ties down memory.
This patch instead adds a new
REQ_CLONEflag that instructsreq_bio_endioto not complete bios attached to a request, which we set on clone requests similar to bios in a flush sequence. With this change I/O errors on a path failure only get propagated to dm-multipath, which can then either resubmit the I/O or complete the bios on the original request.
Hehe.
I seem to remember having sent a similar patch about a year ago; which then got shot down due to the missing partial completion handling.
Quoted 3 lines
I've done some basic testing of this on a Linux target with ALUA support, and it survives path failures during I/O nicely.
So did I ...
Anyway; we've discussed this at LSF in Boston, haven't we? AFAICR we've found that having to resubmit the entire command in the case of partial completion is okay with the storage vendors, so this patch is a viable way of handling things.
_But_ I really would like to have a consensus here that this _is_ the correct way of handling partial request; because if that is the case then we should adopt this strategy throughout the SCSI layer (ie in scsi_io_completion()) and document the fact.
I really don't like to have two different completion paths; we should decide on one way and then use it throughout the stack.
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +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)
Quoted 11 lines · Hannes Reinecke
Quoted 10 lines · Christoph Hellwig
Currently dm-multipath has to clone the bios for every request sent to the lower devices, which wastes cpu cycles and ties down memory.
This patch instead adds a new
REQ_CLONEflag that instructsreq_bio_endioto not complete bios attached to a request, which we set on clone requests similar to bios in a flush sequence. With this change I/O errors on a path failure only get propagated to dm-multipath, which can then either resubmit the I/O or complete the bios on the original request.
Quoted 5 lines
Hehe.
I seem to remember having sent a similar patch about a year ago; which then got shot down due to the missing partial completion handling.
But your approch was entirely different and _not_ acceptable considering it completely eliminated request cloning. In the context of blk-mq we need the request to be allocated directly from the blk-mq device -- "cloning" allows enough indirection to make that workable (as has already landed upstream).
And based on discussion that hch, Jens and I had at LSF eliminating the cloning of a request's bios was very much a near term goal. I even forecast as much in this commit 022333427 ("dm: optimize dm_mq_queue_rq to _not_ use kthread if using pure blk-mq"):
"In the future the bioset allocations will hopefully go away (by
removing support for partial completions of bios in a cloned request)."
Quoted 16 lines
Quoted 3 lines
I've done some basic testing of this on a Linux target with ALUA support, and it survives path failures during I/O nicely.
So did I ...
Anyway; we've discussed this at LSF in Boston, haven't we? AFAICR we've found that having to resubmit the entire command in the case of partial completion is okay with the storage vendors, so this patch is a viable way of handling things.
_But_ I really would like to have a consensus here that this _is_ the correct way of handling partial request; because if that is the case then we should adopt this strategy throughout the SCSI layer (ie in
scsi_io_completion()) and document the fact.
Quoted 3 lines
I really don't like to have two different completion paths; we should decide on one way and then use it throughout the stack.
Can you elaborate on why DM should be constrained by what SCSI does? For DM partial completion was more about trapping failures more quickly (not concerns about command resubmission, etc).
AFAICT there is no reason to impose that both eliminate partial completion at the same time. What am I missing?
But if you have consensus from storage vendors that eliminating partial completion is OK then why not just make it happen? If you do so for 4.2 then it'll look like we coordinated between DM and SCSI ;)
Mike
Quoted 1 line · Hannes Reinecke
I seem to remember having sent a similar patch about a year ago;
This one?
http://www.redhat.com/archives/dm-devel/2014-June/msg00023.html
Quoted 2 lines
which then got shot down due to the missing partial completion handling.
It got shot down because it passed the upper level request down, which is a non-no for blk-mq.
Quoted 4 lines
Anyway; we've discussed this at LSF in Boston, haven't we? AFAICR we've found that having to resubmit the entire command in the case of partial completion is okay with the storage vendors, so this patch is a viable way of handling things.
Yes. But even if it wasn't for some reason we could inspect blk_rq_bytes() on the clone and do a partial completion on the original request, but I'd prefer to avoid special casing this case if we can avoid it.
Quoted 5 lines
_But_ I really would like to have a consensus here that this _is_ the correct way of handling partial request; because if that is the case then we should adopt this strategy throughout the SCSI layer (ie in
scsi_io_completion()) and document the fact.
SCSI is a very different case, we do regularly get patial completions from arrays, and we need to handle them efficiently in the normal I/O path. The only case where we will resubmit completed I/O in dm-mpath is when we got a successfull partial completion in SCSI, and then a following completions returns an error that causes a failover. I don't think there is a need to micro-optimize this case.
PATCH v3 1/1 block, dm: don't copy bios for request clones
In reply to Christoph HellwigFrom: Christoph Hellwig <[email protected]>
Currently dm-multipath has to clone the bios for every request sent to the lower devices, which wastes cpu cycles and ties down memory.
This patch instead adds a new REQ_CLONE flag that instructs req_bio_endio to not complete bios attached to a request, which we set on clone requests similar to bios in a flush sequence. With this change I/O errors on a path failure only get propagated to dm-multipath, which can then either resubmit the I/O or complete the bios on the original request.
I've done some basic testing of this on a Linux target with ALUA support, and it survives path failures during I/O nicely.
--- block/blk-core.c | 94 +++---------------------- drivers/md/dm-table.c | 25 ++++--- drivers/md/dm.c | 171 +++++++++++----------------------------------- drivers/md/dm.h | 5 +- include/linux/blk_types.h | 2 + include/linux/blkdev.h | 6 +- 6 files changed, 73 insertions(+), 230 deletions(-)
v3: really just some line-wrapping, whitespace, and misc small nits
block/blk-core.c +11 -83
@@ -117,7 +117,7 @@ EXPORT_SYMBOL(blk_rq_init);static void req_bio_endio (struct request *rq,struct bio *bio,unsigned int nbytes,int error) { -if (error) +if (error && !(rq->cmd_flags &REQ_CLONE ))clear_bit (BIO_UPTODATE , &bio->bi_flags);else if (!test_bit (BIO_UPTODATE , &bio->bi_flags)) error = -EIO ;
@@ -128,7 +128,8 @@ static void req_bio_endio(struct request *rq, struct bio *bio,bio_advance (bio, nbytes);/* don't actually finish bio if it's part of flush sequence */ -if (bio->bi_iter.bi_size ==0 && !(rq->cmd_flags &REQ_FLUSH_SEQ )) +if (bio->bi_iter.bi_size ==0 && + !(rq->cmd_flags & (REQ_FLUSH_SEQ |REQ_CLONE )))bio_endio (bio, error); }
@@ -2901,95 +2902,22 @@ int blk_lld_busy(struct request_queue *q) }EXPORT_SYMBOL_GPL (blk_lld_busy); -/** -* blk_rq_unprep_clone - Helper function to free all bios in a cloned request -* @rq: the clone request to be cleaned up -* -* Description: -* Free all bios in @rq for a cloned request. -*/ -void blk_rq_unprep_clone (struct request *rq) -{ -struct bio *bio; - -while ((bio = rq->bio) !=NULL ) { - rq->bio = bio->bi_next; - -bio_put (bio); - } -} -EXPORT_SYMBOL_GPL (blk_rq_unprep_clone); - -/* -* Copy attributes of the original request to the clone request. -* The actual data parts (e.g. ->cmd, ->sense) are not copied. -*/ -static void __blk_rq_prep_clone (struct request *dst,struct request *src) +void blk_rq_prep_clone (struct request *dst,struct request *src) { dst->cpu = src->cpu; - dst->cmd_flags |= (src->cmd_flags &REQ_CLONE_MASK ) |REQ_NOMERGE ; + dst->cmd_flags |= (src->cmd_flags &REQ_CLONE_MASK ); + dst->cmd_flags |=REQ_NOMERGE |REQ_CLONE ; dst->cmd_type = src->cmd_type; dst->__sector =blk_rq_pos (src); dst->__data_len =blk_rq_bytes (src); dst->nr_phys_segments = src->nr_phys_segments; dst->ioprio = src->ioprio; dst->extra_len = src->extra_len; -} - -/** -* blk_rq_prep_clone - Helper function to setup clone request -* @rq: the request to be setup -* @rq_src: original request to be cloned -* @bs: bio_set that bios for clone are allocated from -* @gfp_mask: memory allocation mask for bio -* @bio_ctr: setup function to be called for each clone bio. -* Returns %0 for success, non %0 for failure. -* @data: private data to be passed to @bio_ctr -* -* Description: -* Clones bios in @rq_src to @rq, and copies attributes of @rq_src to @rq. -* The actual data parts of @rq_src (e.g. ->cmd, ->sense) -* are not copied, and copying such parts is the caller's responsibility. -* Also, pages which the original bios are pointing to are not copied -* and the cloned bios just point same pages. -* So cloned bios must be completed before original bios, which means -* the caller must complete @rq before @rq_src. -*/ -int blk_rq_prep_clone (struct request *rq,struct request *rq_src, -struct bio_set *bs,gfp_t gfp_mask, -int (*bio_ctr)(struct bio *,struct bio *,void *), -void *data) -{ -struct bio *bio, *bio_src; - -if (!bs) - bs = fs_bio_set; - -__rq_for_each_bio (bio_src, rq_src) { - bio =bio_clone_fast (bio_src, gfp_mask, bs); -if (!bio) - goto free_and_out; - -if (bio_ctr &&bio_ctr (bio, bio_src, data)) - goto free_and_out; - -if (rq->bio) { - rq->biotail->bi_next = bio; - rq->biotail = bio; - }else - rq->bio = rq->biotail = bio; - } - -__blk_rq_prep_clone (rq, rq_src); - -return 0 ; - -free_and_out: -if (bio) -bio_put (bio); -blk_rq_unprep_clone (rq); - -return -ENOMEM ; + dst->bio = src->bio; + dst->biotail = src->biotail; + dst->cmd = src->cmd; + dst->cmd_len = src->cmd_len; + dst->sense = src->sense; }EXPORT_SYMBOL_GPL (blk_rq_prep_clone);
drivers/md/dm-table.c +16 -9
@@ -940,21 +940,28 @@ static int dm_table_alloc_md_mempools(struct dm_table *t, struct mapped_device * {unsigned type =dm_table_get_type (t);unsigned per_bio_data_size =0 ; -struct dm_target *tgt;unsigned i; -if (unlikely (type ==DM_TYPE_NONE )) { +switch (type) { +case DM_TYPE_BIO_BASED : +for (i =0 ; i < t->num_targets; i++) { +struct dm_target *tgt = t->targets + i; + + per_bio_data_size =max (per_bio_data_size, + tgt->per_bio_data_size); + } + t->mempools =dm_alloc_bio_mempools (t->integrity_supported, + per_bio_data_size); +break ; +case DM_TYPE_REQUEST_BASED : +case DM_TYPE_MQ_REQUEST_BASED : + t->mempools =dm_alloc_rq_mempools (md, type); +break ; +default :DMWARN ("no table type is set, can't allocate mempools" );return -EINVAL ; } -if (type ==DM_TYPE_BIO_BASED ) -for (i =0 ; i < t->num_targets; i++) { - tgt = t->targets + i; - per_bio_data_size =max (per_bio_data_size, tgt->per_bio_data_size); - } - - t->mempools =dm_alloc_md_mempools (md, type, t->integrity_supported, per_bio_data_size);if (!t->mempools)return -ENOMEM ;
drivers/md/dm.c +40 -131
233 lines not on this page · raw message
drivers/md/dm.h +3 -2
@@ -222,8 +222,9 @@ void dm_kcopyd_exit(void);/* * Mempool operations */ -struct dm_md_mempools *dm_alloc_md_mempools (struct mapped_device *md,unsigned type, -unsigned integrity,unsigned per_bio_data_size); +struct dm_md_mempools *dm_alloc_bio_mempools (unsigned integrity, +unsigned per_bio_data_size); +struct dm_md_mempools *dm_alloc_rq_mempools (struct mapped_device *md,unsigned type);void dm_free_md_mempools (struct dm_md_mempools *pools);/*
include/linux/blk_types.h +2 -0
@@ -193,6 +193,7 @@ enum rq_flag_bits { __REQ_HASHED,/* on IO scheduler merge hash */ __REQ_MQ_INFLIGHT,/* track inflight for MQ */ __REQ_NO_TIMEOUT,/* requests may never expire */ + __REQ_CLONE,/* cloned bios */ __REQ_NR_BITS, /* stops here */ };
@@ -247,5 +248,6 @@ enum rq_flag_bits {#define REQ_HASHED (1ULL << __REQ_HASHED)#define REQ_MQ_INFLIGHT (1ULL << __REQ_MQ_INFLIGHT)#define REQ_NO_TIMEOUT (1ULL << __REQ_NO_TIMEOUT) +#define REQ_CLONE (1ULL << __REQ_CLONE)#endif /* __LINUX_BLK_TYPES_H */
include/linux/blkdev.h +1 -5
@@ -804,11 +804,7 @@ extern void blk_add_request_payload(struct request *rq, struct page *page, unsigned int len);extern int blk_rq_check_limits (struct request_queue *q,struct request *rq);extern int blk_lld_busy (struct request_queue *q); -extern int blk_rq_prep_clone (struct request *rq,struct request *rq_src, -struct bio_set *bs,gfp_t gfp_mask, -int (*bio_ctr)(struct bio *,struct bio *,void *), -void *data); -extern void blk_rq_unprep_clone (struct request *rq); +extern void blk_rq_prep_clone (struct request *rq,struct request *rq_src);extern int blk_insert_cloned_request (struct request_queue *q,struct request *rq);extern void blk_delay_queue (struct request_queue *,unsigned long );
Signature · 1 line
2.3.2 (Apple Git-55)
[PATCH 2/1] dm: do not allocate any mempools for blk-mq request-based DM
Do not allocate the io_pool mempool for blk-mq request-based DM (DM_TYPE_MQ_REQUEST_BASED) in dm_alloc_rq_mempools().
Also refine __bind_mempools() to have more precise awareness of which mempools each type of DM device uses -- avoids mempool churn when reloading DM tables (particularly for DM_TYPE_REQUEST_BASED).
--- drivers/md/dm-table.c | 4 +-- drivers/md/dm.c | 69 ++++++++++++++++++++++++++++----------------------- 2 files changed, 40 insertions(+), 33 deletions(-)
drivers/md/dm-table.c +2 -2
@@ -962,8 +962,8 @@ static int dm_table_alloc_md_mempools(struct dm_table *t, struct mapped_device *return -EINVAL ; } -if (!t->mempools) -return -ENOMEM ; +if (IS_ERR (t->mempools)) +return PTR_ERR (t->mempools);return 0 ; }
drivers/md/dm.c +38 -31
129 lines not on this page · raw message
Signature · 1 line
2.3.2 (Apple Git-55)
Re: [PATCH 2/1] dm: do not allocate any mempools for blk-mq request-based DM
Quoted 6 lines · Mike Snitzer
Do not allocate the
io_poolmempool for blk-mq request-based DM (DM_TYPE_MQ_REQUEST_BASED) indm_alloc_rq_mempools().Also refine
__bind_mempools()to have more precise awareness of which mempools each type of DM device uses -- avoids mempool churn when reloading DM tables (particularly forDM_TYPE_REQUEST_BASED).
Btw, I looked into this code and didn't dare to touch it before I understood how we deal with the case of dm-mpath using blk-mq and low level driver not or the other way around.
As far as I can see we'd need the request mempool as long as the low level driver does not use dm-mq, independent of what dm-mpath itsel uses.
The code doesn't seem to handle this right, but as mentioned I'm not 100% sure and need to dive deeper into this. Is there a place enforcing dm-mpath is using the same request type as the underlying devices?
Re: [PATCH 2/1] dm: do not allocate any mempools for blk-mq request-based DM
Quoted 11 lines · Christoph Hellwig
Quoted 6 lines · Mike Snitzer
Do not allocate the
io_poolmempool for blk-mq request-based DM (DM_TYPE_MQ_REQUEST_BASED) indm_alloc_rq_mempools().Also refine
__bind_mempools()to have more precise awareness of which mempools each type of DM device uses -- avoids mempool churn when reloading DM tables (particularly forDM_TYPE_REQUEST_BASED).Btw, I looked into this code and didn't dare to touch it before I understood how we deal with the case of dm-mpath using blk-mq and low level driver not or the other way around.
Current code does deal with:
1) blk-mq queue on non-blk-mq underlying devices
2) blk-mq queue on blk-mq underlying devices
3) non-blk-mq queue on non-blk-mq underlying devices
4) non-blk-mq queue on blk-mq underlying devices
But all these permutations do definitely make the code less approachable and maintainable.
Quoted 7 lines
As far as I can see we'd need the request mempool as long as the low level driver does not use dm-mq, independent of what dm-mpath itsel uses.
The code doesn't seem to handle this right, but as mentioned I'm not 100% sure and need to dive deeper into this. Is there a place enforcing dm-mpath is using the same request type as the underlying devices?
It is dm-table.c:dm_table_set_type() that determines whether a given DM table load references underlying devices that are all blk-mq or not. Based on what it finds it either establishes the type as DM_TYPE_REQUEST_BASED or DM_TYPE_MQ_REQUEST_BASED.
DM_TYPE_REQUEST_BASED will make use of io_pool, whereas DM_TYPE_MQ_REQUEST_BASED won't. There is an awkward duality where use_blk_mq governs which mempools are created based on filter_md_type(). filter_md_type() is needed because the table's type is really only concerned with the underlying devices' types -- not the top-level DM queue. So things are awkward until we establish the top-level queue (then we just make use of q->mq_ops like normal).
It is dm.c:dm_setup_md_queue() that establishes the top-level DM queue based on the type.
How the DM queue's requests are cloned depends the underlying devices (e.g. blk-mq vs not).
Top-level blk-mq DM device uses the md's type (DM_TYPE_REQUEST_BASED vs DM_TYPE_MQ_REQUEST_BASED) to judge how to clone the request. DM_TYPE_REQUEST_BASED implies legacy path.
There is still some awkward branching in the IO path but it ended up being surprisingly manageable. Only concern I have is: in the long-run will all this duality in the code compromise maintainability? But hopefully we can kill the old request_fn path in block core (if/when blk-mq IO scheduling lands) and the DM code can be cleaned up accordingly.
Quoted 12 lines · Mike Snitzer
From: Christoph Hellwig <[email protected]>
Currently dm-multipath has to clone the bios for every request sent to the lower devices, which wastes cpu cycles and ties down memory.
This patch instead adds a new
REQ_CLONEflag that instructsreq_bio_endioto not complete bios attached to a request, which we set on clone requests similar to bios in a flush sequence. With this change I/O errors on a path failure only get propagated to dm-multipath, which can then either resubmit the I/O or complete the bios on the original request.
Quoted 5 lines
I've done some basic testing of this on a Linux target with ALUA support, and it survives path failures during I/O nicely.
Signed-off-by Christoph Hellwig <[email protected]>Signed-off-by Mike Snitzer <[email protected]>
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
What is the change from v2? I've not really noticed anything.
Quoted 1 line · Christoph Hellwig
What is the change from v2? I've not really noticed anything.
Just whitespace really, I said:
"v3: really just some line-wrapping, whitespace, and misc small nits"
I could get you a diff but I just had some superficial changes.
Quoted 26 lines · Mike Snitzer
From: Christoph Hellwig <[email protected]>
Currently dm-multipath has to clone the bios for every request sent to the lower devices, which wastes cpu cycles and ties down memory.
This patch instead adds a new
REQ_CLONEflag that instructsreq_bio_endioto not complete bios attached to a request, which we set on clone requests similar to bios in a flush sequence. With this change I/O errors on a path failure only get propagated to dm-multipath, which can then either resubmit the I/O or complete the bios on the original request.I've done some basic testing of this on a Linux target with ALUA support, and it survives path failures during I/O nicely.
Signed-off-by Christoph Hellwig <[email protected]>Signed-off-by Mike Snitzer <[email protected]>--- block/blk-core.c | 94 +++---------------------- drivers/md/dm-table.c | 25 ++++--- drivers/md/dm.c | 171 +++++++++++----------------------------------- drivers/md/dm.h | 5 +- include/linux/blk_types.h | 2 + include/linux/blkdev.h | 6 +- 6 files changed, 73 insertions(+), 230 deletions(-)
Quoted 1 line
v3: really just some line-wrapping, whitespace, and misc small nits
Hey Jens,
Given the blk-core changes, would you like to pick this up for 4.2 and I'll just base DM ontop of linux-block.git again for the 4.2 cycle?
The patch is available here: https://patchwork.kernel.org/patch/6285081/
Mike
p.s. I do have a followup DM patch that cleans things up further but I can just stage it via linux-dm.git:
https://patchwork.kernel.org/patch/6285091/