[dm-devel] block, dm: don't copy bios for request clones

[Mike Snitzer]

On Fri, Apr 24 2015 at  3:33pm -0400,
Christoph Hellwig <hch at lst.de> wrote:

> 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.

Thanks for working on this (it slipped in priority on my TODO list).
Will be great to get this reviewed/tested/staged for 4.2.

But your patch needs rebasing against latest upstream code.  I'd imagine
this means you also haven't tested with "use_blk_mq" enabled (from
commit 17e149b8f).

I'll do a much more careful review once you rebase but I noticed one
thing worth mentioning now:

> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 6554d91..4ac0a47 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -941,23 +941,30 @@ bool dm_table_mq_request_based(struct dm_table *t)
>  
>  static int dm_table_alloc_md_mempools(struct dm_table *t)
>  {
> -	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) {

You're missing a case statement here for 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(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(type, t->integrity_supported, per_bio_data_size);
>  	if (!t->mempools)
>  		return -ENOMEM;
>