[dm-devel] dm: Devices not having .iterate_devices may want to set max_io_len

[Kirill Tkhai]

On 30.11.2020 20:30, Mike Snitzer wrote:
> On Wed, Nov 25 2020 at  3:49pm -0500,
> Kirill Tkhai <ktkhai at virtuozzo.com> wrote:
> 
>> After commit 5091cdec56fa "dm: change max_io_len() to use
>> blk_max_size_offset()" my out-of-tree driver stopped to work.
>> The reason is that now ti->max_io_len from such target is ignored:
>> max_io_len() ignores ti->max_io_len, while
>> dm_calculate_queue_limits() never stacks ti->chunk_sectors into
>> ti_limits.chunk_sectors.
>>
>> Here I see two possible solutions, both are in dm_calculate_queue_limits():
>>
>> 1)Move ti_limits.chunk_sectors assignment down, so it will be made
>> right under combine_limits label. Thus, every time ti->max_io_len
>> will transform into chunk_sectors, even in case of device
>> has no .iterate_devices method;
>>
>> 2)Move io_hints call under the label (like it's made in this patch),
>> so one can set desired chunk_sectors there.
>>
>> First solution looks less clear, since in two drivers chunk_sectors
>> are assigned in io_hints (see unstripe_io_hints() and dmz_io_hints()),
>> and this rewrites, and we should not rewrite it.
>>
>> Second solution does not break anything since we change only
>> order with ->iterate_devices(device_area_is_invalid), while
>> device_area_is_invalid never touches chunk_sectors. So I choosed it.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>> ---
>>  drivers/md/dm-table.c |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>> index 2073ee8d18f4..9994c767dc82 100644
>> --- a/drivers/md/dm-table.c
>> +++ b/drivers/md/dm-table.c
>> @@ -1453,10 +1453,6 @@ int dm_calculate_queue_limits(struct dm_table *table,
>>  		if (ti->max_io_len)
>>  			ti_limits.chunk_sectors = lcm_not_zero(ti->max_io_len,
>>  							       ti_limits.chunk_sectors);
>> -		/* Set I/O hints portion of queue limits */
>> -		if (ti->type->io_hints)
>> -			ti->type->io_hints(ti, &ti_limits);
>> -
>>  		/*
>>  		 * Check each device area is consistent with the target's
>>  		 * overall queue limits.
>> @@ -1466,6 +1462,10 @@ int dm_calculate_queue_limits(struct dm_table *table,
>>  			return -EINVAL;
>>  
>>  combine_limits:
>> +		/* Set I/O hints portion of queue limits */
>> +		if (ti->type->io_hints)
>> +			ti->type->io_hints(ti, &ti_limits);
>> +
>>  		/*
>>  		 * Merge this target's queue limits into the overall limits
>>  		 * for the table.
>>
>>
> 
> Sorry for the slow response, just got back from PTO today.
> 
> So while I appreciate that commit 5091cdec56fa caused your DM target to
> regress I don't think the proper solution is for your driver to take on
> setting chunk_sectors based on the ti->max_io_len you've established.
> 
> The use of chunk_sectors is an implementation detail.  One that every DM
> target that doesn't establish .iterate_devices should not need to take
> on worrying about.
> 
> Your above proposed patch changes DM core to _start_ to fix the
> regression you've reported but still requires your DM target to change
> too.  Does you DM target have one or more data device(s)?  If so you
> really should have it provide a .iterate_devices hook.  But that aside,

This is a virtual device, which never interact with data device, so there
is no .iterate_devices.

> here is the fix I'll be staging in linux-next shortly and that I'll be
> sending to Linus by the end of the week:
> 
> From: Mike Snitzer <snitzer at redhat.com>
> Date: Mon, 30 Nov 2020 10:57:43 -0500
> Subject: [PATCH] dm table: fix IO splitting
> 
> Commit 882ec4e609c1 ("dm table: stack 'chunk_sectors' limit to account
> for target-specific splitting") caused a regression for a couple
> reasons:
> 
> 1) Using lcm_not_zero() instead of min_not_zero() when stacking
>    chunk_sectors was a bug because chunk_sectors must reflect the most
>    limited of all devices in the IO stack.
> 2) DM targets that set max_io_len but that do _not_ provide an
>    .iterate_devices method no longer had there IO split properly.
> 
> dm_calculate_queue_limits() must establish the appropriate IO
> splitting limits early, without any dependency on iterating
> data_devices, otherwise IO will not be split as intended.
> 
> Fixes: 882ec4e609c1 ("dm table: stack 'chunk_sectors' limit to account for target-specific splitting")
> Cc: stable at vger.kernel.org
> Reported-by: John Dorminy <jdorminy at redhat.com>
> Reported-by: Bruce Johnston <bjohnsto at redhat.com>
> Reported-by: Kirill Tkhai <ktkhai at virtuozzo.com>
> Signed-off-by: Mike Snitzer <snitzer at redhat.com>

This works for me. Thanks, Mike.

You may add my Tested-by: Kirill Tkhai <ktkhai at virtuozzo.com>

> ---
>  drivers/md/dm-table.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 2073ee8d18f4..4e58f5c68ac0 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -18,7 +18,6 @@
>  #include <linux/mutex.h>
>  #include <linux/delay.h>
>  #include <linux/atomic.h>
> -#include <linux/lcm.h>
>  #include <linux/blk-mq.h>
>  #include <linux/mount.h>
>  #include <linux/dax.h>
> @@ -1431,6 +1430,10 @@ int dm_calculate_queue_limits(struct dm_table *table,
>  
>  		ti = dm_table_get_target(table, i);
>  
> +		/* Set appropriate limits if target-specific splitting is required */
> +		if (ti->max_io_len)
> +			ti_limits.chunk_sectors = ti->max_io_len;
> +
>  		if (!ti->type->iterate_devices)
>  			goto combine_limits;
>  
> @@ -1449,10 +1452,6 @@ int dm_calculate_queue_limits(struct dm_table *table,
>  			zone_sectors = ti_limits.chunk_sectors;
>  		}
>  
> -		/* Stack chunk_sectors if target-specific splitting is required */
> -		if (ti->max_io_len)
> -			ti_limits.chunk_sectors = lcm_not_zero(ti->max_io_len,
> -							       ti_limits.chunk_sectors);
>  		/* Set I/O hints portion of queue limits */
>  		if (ti->type->io_hints)
>  			ti->type->io_hints(ti, &ti_limits);
>