[dm-devel] [PATCH v2] block: use gcd() to fix chunk_sectors limit stacking

[Mike Snitzer]

On Wed, Dec 02 2020 at 10:26pm -0500,
Ming Lei <ming.lei at redhat.com> wrote:

> On Tue, Dec 01, 2020 at 11:07:09AM -0500, Mike Snitzer wrote:
> > commit 22ada802ede8 ("block: use lcm_not_zero() when stacking
> > chunk_sectors") broke chunk_sectors limit stacking. 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.
> 
> What is the user-visible result of splitting IO at 128 sectors?

The VDO dm target fails because it requires IO it receives to be split
as it advertised (8 sectors).

> I understand it isn't related with correctness, because the underlying
> queue can split by its own chunk_sectors limit further. So is the issue
> too many further-splitting on queue with chunk_sectors 8? then CPU
> utilization is increased? Or other issue?

No, this is all about correctness.

Seems you're confining the definition of the possible stacking so that
the top-level device isn't allowed to have its own hard requirements on
IO sizes it sends to its internal implementation.  Just because the
underlying device can split further doesn't mean that the top-level
virtual driver can service larger IO sizes (not if the chunk_sectors
stacking throws away the hint the virtual driver provided because it
used lcm_not_zero).

> > And since commit 07d098e6bbad ("block: allow 'chunk_sectors' to be
> > non-power-of-2") care must be taken to properly stack chunk_sectors to
> > be compatible with the possibility that a non-power-of-2 chunk_sectors
> > may be stacked. This is why gcd() is used instead of reverting back
> > to using min_not_zero().
> 
> I guess gcd() won't be better because gcd(a,b) is <= max(a, b), so bio
> size is decreased much with gcd(a, b), and IO performance should be affected.
> Maybe worse than min_not_zero(a, b) which is often > gcd(a, b).

Doesn't matter, it is about correctness.

We cannot stack up a chunk_sectors that violates requirements of a given
layer.

Mike