[dm-devel] [PATCH] block: revert to using min_not_zero() when stacking chunk_sectors

[John Dorminy]

I don't think this suffices, as it allows IOs that span max(a,b) chunk
boundaries.

Chunk sectors is defined as "if set, it will prevent merging across
chunk boundaries". Pulling the example from the last change:
It is possible, albeit more unlikely, for a block device to have a non
power-of-2 for chunk_sectors (e.g. 10+2 RAID6 with 128K chunk_sectors,
which results in a full-stripe size of 1280K. This causes the RAID6's
io_opt to be advertised as 1280K, and a stacked device _could_ then be
made to use a blocksize, aka chunk_sectors, that matches non power-of-2
io_opt of underlying RAID6 -- resulting in stacked device's
chunk_sectors being a non power-of-2).

Suppose the stacked device had a block size/chunk_sectors of 256k.
Then, with this change, some IOs issued by the stacked device to the
RAID beneath could span 1280k sector boundaries, and require further
splitting still. I think combining as the GCD is better, since any IO
of size gcd(a,b) definitely spans neither a a-chunk nor a b-chunk
boundary.

But it's possible I'm misunderstanding the purpose of chunk_sectors,
or there should be a check that the one of the two devices' chunk
sizes divides the other.

Thanks.

-John

On Mon, Nov 30, 2020 at 12:18 PM Mike Snitzer <snitzer at redhat.com> wrote:
>
> chunk_sectors must reflect the most limited of all devices in the IO
> stack.
>
> Otherwise malformed IO may result. E.g.: prior to this fix,
> ->chunk_sectors = lcm_not_zero(8, 128) would result in
> blk_max_size_offset() splitting IO at 128 sectors rather than the
> required more restrictive 8 sectors.
>
> Fixes: 22ada802ede8 ("block: use lcm_not_zero() when stacking chunk_sectors")
> Cc: stable at vger.kernel.org
> Reported-by: John Dorminy <jdorminy at redhat.com>
> Reported-by: Bruce Johnston <bjohnsto at redhat.com>
> Signed-off-by: Mike Snitzer <snitzer at redhat.com>
> ---
>  block/blk-settings.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 9741d1d83e98..1d9decd4646e 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -547,7 +547,10 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
>
>         t->io_min = max(t->io_min, b->io_min);
>         t->io_opt = lcm_not_zero(t->io_opt, b->io_opt);
> -       t->chunk_sectors = lcm_not_zero(t->chunk_sectors, b->chunk_sectors);
> +
> +       if (b->chunk_sectors)
> +               t->chunk_sectors = min_not_zero(t->chunk_sectors,
> +                                               b->chunk_sectors);
>
>         /* Physical block size a multiple of the logical block size? */
>         if (t->physical_block_size & (t->logical_block_size - 1)) {
> --
> 2.15.0
>