Date:   Fri, 11 Dec 2020 11:30:49 -0500
From:   Mike Snitzer <snitzer@...hat.com>
To:     Sergei Shtepa <sergei.shtepa@...am.com>, axboe@...nel.dk,
        hch@....de
Cc:     "johannes.thumshirn@....com" <johannes.thumshirn@....com>,
        "koct9i@...il.com" <koct9i@...il.com>,
        "ming.lei@...hat.com" <ming.lei@...hat.com>,
        "hare@...e.de" <hare@...e.de>,
        "josef@...icpanda.com" <josef@...icpanda.com>,
        "steve@....org" <steve@....org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Pavel Tide <Pavel.TIde@...am.com>, dm-devel@...hat.com
Subject: Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

On Thu, Dec 10 2020 at 11:32am -0500,
Mike Snitzer <snitzer@...hat.com> wrote:

> On Thu, Dec 10 2020 at  9:58am -0500,
> Sergei Shtepa <sergei.shtepa@...am.com> wrote:
> 
> > The 12/09/2020 16:51, Mike Snitzer wrote:
> > > On Wed, Dec 09 2020 at  8:01am -0500,
> > > Sergei Shtepa <sergei.shtepa@...am.com> wrote:
> > > 
> > > > Hi all.
> > > > 
> > > > I try to suggest the Block Layer Interposer (blk_interposer) again.
> > > > It`s allows to intercept bio requests, remap bio to another devices
> > > > or add new bios.
> > > > 
> > > > Initially, blk_interposer was designed to be compatible with
> > > > device mapper. Our (my and Hannes) previous attempt to offer
> > > > blk_interposer integrated with device mapper did not receive
> > > > any comments from the dm-devel team, and without their help
> > > > I will not be able to make a full implementation. I hope later
> > > > they will have time to support blk_interposer in device mapper.
> > > 
> > > Excuse me?  I gave you quite a bit of early feedback!  I then went on
> > > PTO for ~10 days, when I returned last week I had to deal with fixing
> > > some 5.10 dm/block bio splitting regressions that only got resolved this
> > > past Saturday in time for 5.10-rc7.
> > 
> > Mike,
> > 
> > I would like to clarify some points that I've made, and also try 
> > to repair the damage from the misunderstandings that I think have occured.
> > 
> > First of all, I actually meant the feedback on Hannes's patch which was
> > sent on the 19th of November:
> > https://lore.kernel.org/linux-block/20201119164924.74401-1-hare@suse.de/
> > 
> > Your feedback from the 18th of November ("[PATCH 4/4] dm_interposer - 
> > Try to use blk_interpose in dm") is very valuable, but I am not sure that
> > I am currently capable of implementing the proposed DM changes.
> > The overall architecture of DM is still not clear to me, and I am studying
> > it right now.
> > 
> > This new patch (the one that Hannes sent on the 19th of November) is also
> > compatibile with DM and should not pose any problems - the architecture is
> > the same. There are some changes that make blk_interposer simpler and better,
> > plus the sample is added.
> > 
> > > 
> > > blk_interposer was/is on my short list to review closer (Hannes' version
> > > that refined yours a bit more).. primarily to see if I could avoid the
> > > extra clone and endio hooking.
> > 
> > Glad to hear that! In order to avoid the additional copying one can only
> > change an intercepted bio, which might be dangerous.
> > 
> > > 
> > > The development window for 5.11 is past, so you pushing this without
> > > using the approach discussed (in terms of DM) doesn't help your cause.
> > > 
> > > > And my blk-snap module requires an architecture change to
> > > > support blk_interposer.
> > > > 
> > > > This time I offer it along with a sample.
> > > > Despite the fact that blk_interposer is quite simple,
> > > > there are a few non-obvious points that I would like to clarify.
> > > > 
> > > > However, I suggest the blk_interposer now so that people
> > > > could discuss it and use it in their projects as soon as possible.
> > > 
> > > So you weren't willing to put the work in on something DM oriented
> > > because you expected me to do the work for you?  And now you're looking
> > > to side-step the consensus that was built because I didn't contribute
> > > quick enough?  That's pretty messed up.
> > 
> > I just think that no one can implement integration of DM with
> > blk_interposer better than dm-devel team. I will certainly try my best,
> > but I am afraid that such efforts will only produce incongruous
> > DM patches that will be a waste of time (yours and mine).
> > 
> > > 
> > > In the time-scale that is Linux kernel development.. you've really
> > > jumped the gun and undercut my enthusiasm to work through the details.
> > > I'd rather not read into your actions more than I already have here, but
> > > I'm not liking what you've done here.  Kind of left in dismay with how
> > > you decided to go down this path without a followup note to me or others
> > > (that I'm aware of).
> > 
> > I am very sorry that I undercut your enthusiasm, but, as you rightly
> > pointed out, another development windows is closing, and my product
> > is still not able to work on newer Linux versions starting from 5.8.
> > That fact makes me particularly sad and forces me to search for different
> > means to draw some attention to blk_interposer. I've seen an RHEL 8.4
> > alpha recently, all looks very cool there but made me even more sad ...
> 
> Made you more sad because you don't have a working solution for upstream
> or RHEL 8.4?
> 
> I may have missed it in your past emails but how were you able to
> provide blk-snap support for kernels prior to 5.8?

I now clearly understand that the 5.8 block changes to do away with
->make_request_fn in favor of a more optimized ->submit_bio (that isn't
applicable for all devices) is why you're pursuing a "fix" so urgently.

> > > But I'm still willing to make blk_interposer work as we all discussed
> > > (in terms of DM).
> > 
> > I want it too. However, there is a certain difficulty with usage of DM
> > for backup copying. For instance, there is no changed block tracking (CBT)
> > and right now I don't have any clue how it could be implemented
> > considering the existing architecture. I still hope that sometime
> > in future I could be offer my blk-snap module which was specifically
> > created for backup copying purposes.
> > 
> > I apologize for causing all that confusion and mess and making you upset.
> > I hope that all of the above makes sense to you and you will not think
> > that I was slacking and tried to offload all the work to your team.
> 
> My primary concern is that blk_interposer be correct from the start.  To
> validate its correctness it needs to be fully implemented and vetted in
> terms on upstream Linux kernel code.  DM can easily serve as the primary
> upstream consumer until if/when your blk-snap module is proposed for
> upstream inclusion.
> 
> But short of having an actual upstream consumer of blk_interposer (not
> just samples/ code) it cannot go upstream.  Otherwise there are too many
> risks of misuse and problems in the longrun.  That and it'd be baggage
> block core would need to carry for no upstream Linux benefit.

As I shared in private: I have some urgent Red Hat business critical
work I need to do and unfortunately cannot put my near-term focus to
implementing a fully baked blk_interposer for DM.  But I can come back
to it (sadly unlikely to do so until the new year though).

While I still think there needs to be a proper _upstream_ consumer of
blk_interposer as a condition of it going in.. I'll let others make the
call.

As such, I'll defer to Jens, Christoph and others on whether your
minimalist blk_interposer hook is acceptable in the near-term.

Mike

Powered by blists - more mailing lists