Re: [PATCH v2] block: use gcd() to fix chunk_sectors limit stacking

From: Ming Lei <ming.lei@redhat.com>
To: Mike Snitzer <snitzer@redhat.com>
Cc: axboe@kernel.dk, martin.petersen@oracle.com,
	linux-block@vger.kernel.org, dm-devel@redhat.com,
	jdorminy@redhat.com, bjohnsto@redhat.com
Subject: Re: [PATCH v2] block: use gcd() to fix chunk_sectors limit stacking
Date: Fri, 4 Dec 2020 11:59:24 +0800	[thread overview]
Message-ID: <20201204035924.GD661914@T590> (raw)
In-Reply-To: <20201204020343.GA32150@redhat.com>

On Thu, Dec 03, 2020 at 09:03:43PM -0500, Mike Snitzer wrote:
> On Thu, Dec 03 2020 at  8:12pm -0500,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > On Thu, Dec 03, 2020 at 09:33:59AM -0500, Mike Snitzer wrote:
> > > On Wed, Dec 02 2020 at 10:26pm -0500,
> > > Ming Lei <ming.lei@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).
> > 
> > OK, looks VDO's chunk_sector limit is one hard constraint, even though it
> > is one DM device, so I guess you are talking about DM over VDO?
> > 
> > Another reason should be that VDO doesn't use blk_queue_split(), otherwise it
> > won't be a trouble, right?
> > 
> > Frankly speaking, if the stacking driver/device has its own hard queue limit
> > like normal hardware drive, the driver should be responsible for the splitting.
> 
> DM core does the splitting for VDO (just like any other DM target).
> In 5.9 I updated DM to use chunk_sectors, use blk_stack_limits()
> stacking of it, and also use blk_max_size_offset().
> 
> But all that block core code has shown itself to be too rigid for DM.  I
> tried to force the issue by stacking DM targets' ti->max_io_len with
> chunk_sectors.  But really I'd need to be able to pass in the per-target
> max_io_len to blk_max_size_offset() to salvage using it.
> 
> Stacking chunk_sectors seems ill-conceived.  One size-fits-all splitting
> is too rigid.

DM/VDO knows exactly it is one hard chunk_sectors limit, and DM shouldn't play
the stacking trick on VDO's chunk_sectors limit, should it?

Thanks, 
Ming