Re: [PATCH 0/2] block-copy: small fix and refactor

[Emanuele Giuseppe Esposito]

On 07/06/2021 18:18, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2021 18:16, Emanuele Giuseppe Esposito wrote:
> > On 07/06/2021 17:10, Kevin Wolf wrote:
> > > Am 03.06.2021 um 09:38 hat Paolo Bonzini geschrieben:
> > > > On 02/06/21 14:21, Kevin Wolf wrote:
> > > > > Am 02.06.2021 um 11:13 hat Stefan Hajnoczi geschrieben:
> > > > > > On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir 
> > > > > > Sementsov-Ogievskiy wrote:
> > > > > > > Hi all!
> > > > > > >
> > > > > > > This is my suggestion how to refactor block-copy to avoid extra 
> > > > > > > atomic
> > > > > > > operations in
> > > > > > > "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
> > > > > > >
> > > > > > > Vladimir Sementsov-Ogievskiy (2):
> > > > > > >     block-copy: fix block_copy_task_entry() progress update
> > > > > > >     block-copy: refactor copy_range handling
> > > > > > >
> > > > > > >    block/block-copy.c | 79 
> > > > > > > +++++++++++++++++++++++++++++++---------------
> > > > > > >    1 file changed, 53 insertions(+), 26 deletions(-)
> > > > > >
> > > > > > I posted suggestions for the doc comment on Patch 2, otherwise:
> > > > > >
> > > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > >
> > > > > Thanks, fixed up the comment accordingly and applied to the block
> > > > > branch.
> > > >
> > > > I'm a bit confused.  Vladimir said in his review of Emanuele's patches
> > > > that he was okay with patch 7 and that he would rebase this
> > > > refactoring on top of it.
> > > >
> > > > Vladimir's main complaint for the s->method state machine was the
> > > > extra lines of code.  Here we have just as many new lines of code and
> > > > new parameters that are passed by reference.  Kevin, can you please
> > > > look at Emanuele's patches and possibly unqueue the second patch here?
> > > > It seems to me that it should have been tagged as RFC.
> > >
> > > Sorry, I was not aware that Vladimir intended to rebase this one. This
> > > has already landed in master, so if rebasing the other patch is a real
> > > problem, we'd have to revert this one first.
> > It shouldn't be a problem, I have already rebased on top of it. I will 
> > re-spin a new series with this and other minor (and hopefully final) 
> > fixes soon.
>
> Thanks, and sorry for the mess!
>
> Hmm, actually, I said
>
> > OK, I'm OK with patch as is. Finally I can refactor it later on top if 
> > needed.. I'll try now do some refactoring, you'll probably want to 
> > base on it, or vise-versa, I'll rebase it later on top of these patches. 
>
> So, I considered both variants. Then I sent patches, everybody in CC, 
> everybody were silent.
>
>
> Honestly, I'm a bit confused too. I find my complains valid 
> (independently of me being "I'm OK and can refactor later") and you 
> agreed with them in general. I'm an author and maintainer of the 
> component. I do refactoring that makes it simple to follow my 
> suggestion. So for me it's a bit like doing your work for you. And you 
> ask to roll-back it.

I think it's useless to discuss about these things now. I rebased, all 
is clear and I am positive that in the next version we will have 
something that makes everyone happy :) and if not, feel free to comment it!

Emanuele

> Still, misunderstanding and the mess with two parallel conflicting 
> series is my fault, sorry for this. At least I should have answered to 
> your series when Stefan gave an r-b to my series.