Re: [PATCH 5.20 1/4] block: add bio_rewind() API

[Mike Snitzer <snitzer@redhat.com>]

On Tue, Jun 28 2022 at 12:36P -0400,
Kent Overstreet <kent.overstreet@gmail.com> wrote:

> On Tue, Jun 28, 2022 at 03:49:28PM +0800, Ming Lei wrote:
> > On Tue, Jun 28, 2022 at 12:26:10AM -0400, Kent Overstreet wrote:
> > > On Mon, Jun 27, 2022 at 03:36:22PM +0800, Ming Lei wrote:
> > > > Not mention bio_iter, bvec_iter has been 32 bytes, which is too big to
> > > > hold in per-io data structure. With this patch, 8bytes is enough
> > > > to rewind one bio if the end sector is fixed.
> > > 
> > > And with rewind, you're making an assumption about the state the iterator is
> > > going to be in when the IO has completed.
> > > 
> > > What if the iterator was never advanced?
> > 
> > bio_rewind() works as expected if the iterator doesn't advance, since bytes
> > between the recorded position and the end position isn't changed, same
> > with the end position.
> > 
> > > 
> > > So say you check for that by saving some other part of the iterator - but that
> > > may have legitimately changed too, if the bio was redirected (bi_sector changes)
> > > or trimmed (bi_size changes)
> > > 
> > > I still think this is an inherently buggy interface, the way it's being proposed
> > > to be used.
> > 
> > The patch did mention that the interface should be for situation in which end
> > sector of bio won't change.
> 
> But that's an assumption that you simply can't make!

Why not?  There is clear use-case for this API, as demonstrated in the
patchset: DM can/will make use of it for the purposes of enhancing its
more unlikely bio requeuing work (that is needed to make the more
likely bio splitting scenario more efficient).

> We allow block device drivers to be stacked in _any_ combination. After a bio is
> completed it may have been partially advanced, fully advanced, trimmed, not
> trimmed, anything - and bi_sector and thus also bio_end_sector() may have
> changed, and will have if there's partition tables involved.

We don't _need_ this API to cure cancer for all hypothetical block
drivers.

If consumers of the API follow the rule that end sector of the
_original bio_ isn't changed then it is all fine.  It is that simple.

Stacked drivers will work just fine.  The lower layers will be
modifying their bios as needed. Because for DM those bios happen to
be clones, so there is isolation to the broader design flaw you are
trying to say is a major problem. As this patchset demonstrates.

I do concede that policing who can use an API is hard.  But if some
consumer of an API tries something that invalidates rules of the API
they get to keep the N pieces when it breaks.

Mike