Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Max Reitz]

On 29.07.21 18:29, Vladimir Sementsov-Ogievskiy wrote:
> 29.07.2021 16:47, Max Reitz wrote:
> > On 29.07.21 13:35, Vladimir Sementsov-Ogievskiy wrote:
> > > 29.07.2021 13:38, Max Reitz wrote:
> > > > On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
> > > > > 28.07.2021 10:00, Max Reitz wrote:
> > > > > > On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > > Hi all!
> > > > > > >
> > > > > > > That's an alternative to (part of) Max's
> > > > > > > "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
> > > > > > > and shows' my idea of handling soft-cancelling READY mirror case
> > > > > > > directly in qmp_block_job_cancel. And cleanup all other job 
> > > > > > > cancelling
> > > > > > > functions.
> > > > > > >
> > > > > > > That's untested draft, don't take it to heart :)
> > > > > >
> > > > > > Well, I would have preferred it if you’d rebased this on top of 
> > > > > > that series, precisely because it’s an alternative to only part 
> > > > > > of it. And if it’s just an untested draft, that would have been 
> > > > > > even better, because it would’ve given a better idea on what the 
> > > > > > cleanup looks like.
> > > > > >
> > > > > > There are also things like this series making cancel internally 
> > > > > > always a force-cancel, where I’m not sure whether we want that in 
> > > > > > the replication driver or not[1].  With my series, we add an 
> > > > > > explicit parameter, so we’re forced to think about it, and then 
> > > > > > in this series on top we can just drop the parameter for all 
> > > > > > force-cancel invocations again, and for all non-force-cancel 
> > > > > > invocations we would have to think a bit more.
> > > > >
> > > > > I now don't sure that patch 5 of your series is correct (see my 
> > > > > last answer to it), that's why I decided to not base on it.
> > > >
> > > > Well, we can always take patch 5 from v1.  (Where I changed any 
> > > > job_is_cancelled() to job_cancel_requested() when it influenced the 
> > > > external interface.)
> > > >
> > > > > My series has the benefit of handling soft-mirror-cancel case the 
> > > > > other way and handles mirror finalization in case of soft-cancel 
> > > > > properly.
> > > > >
> > > > > > Specifically as for this series, I don’t like job_complete_ex() 
> > > > > > very much, I think the parameter should be part of job_complete() 
> > > > > > itself.
> > > > >
> > > > > That was my idea. But job_complete is passed as function pointer, 
> > > > > so changing its prototype would be more work.. But I think it's 
> > > > > possible.
> > > > >
> > > > > >   If we think that’s too specific of a mirror parameter to 
> > > > > > include in normal job_complete(), well, then there shouldn’t be a 
> > > > > > job_complete_ex() either, and do_graph_change should be a 
> > > > > > property of the mirror job (perhaps as pivot_on_completion) 
> > > > > > that’s cleared by qmp_block_job_cancel() before invoking 
> > > > > > job_complete().
> > > > >
> > > > > This way users will lose a way to make a decision during job 
> > > > > running..
> > > >
> > > > On the contrary, it would be a precursor to letting the user change 
> > > > this property explicitly with a new QMP command.
> > > >
> > > > > But probably they don't need actually. Moving the option to mirror 
> > > > > job parameter seems a good option to me.
> > > > >
> > > > > > Max
> > > > > >
> > > > > > [1] Although looking at it again now, it probably wants 
> > > > > > force-cancel.
> > > > >
> > > > >
> > > > > What do you think of my idea to keep old bugs as is and just 
> > > > > deprecate block-job-cancel and add a new interface for 
> > > > > "no-graph-change mirror" case?
> > > >
> > > > I don’t see a reason for that.  The fix isn’t that complicated.
> > > >
> > > > Also, honestly, I don’t see a good reason for deprecating anything.
> > >
> > > Current interface lead to mess in the code, that's bad. Cancellation 
> > > mode that is actually a kind of completion (and having comments in 
> > > many places about that) - that shows for me that interface is not 
> > > good.. It's a question of terminology, what to call "cancel". Also, 
> > > that's not the first time this question arise. Remember my recent 
> > > cancel-in-flight-requests series, when I thought that "cancel is 
> > > cancel" and didn't consider soft-cancel of mirror.. And reviewers 
> > > didn't caught it. I don't think that interface is good, it will 
> > > always confuse new developers and users. But that's just my opinion, 
> > > I don't impose it )
> > >
> > > If not deprecate, i.e. if we consider old interface to be good, than 
> > > no reason for this my series and for introducing new interface :)
> >
> > I’m not against a better interface, I’m against using this current 
> > bug as an excuse to improve the interface.  We’ve known we want to 
> > improve the interface for quite a long time now, we don’t need an 
> > excuse for that.
> >
> > If we use this bug as an excuse, I’m afraid of becoming hung up on 
> > interface discussions instead of just getting the bug fixed. And we 
> > must get the bug fixed, it’s real, it’s kind of bad, and saying “it 
> > won’t appear with the new interface, let’s not worry about the old 
> > one” is not something I like.
> >
> > OTOH, if we use this bug as an excuse, I’m also afraid of trying to 
> > rush the design instead of actually implementing the interface that 
> > we’ve always desired, i.e. where the user gets to choose the 
> > completion mode via yet-to-be-implemented some job property setter 
> > function.
> >
> > As a final note (but this is precisely the interface discussion that 
> > I want to avoid for now), I said I don’t see a good reason for 
> > deprecating anything, because `job-cancel force=false` can just 
> > internally do `set-job-property .pivot_on_completion=false; 
> > job-complete`.  From an implementation perspective, that should be 
> > simple.
> >
> > I understand that for users the existence of the `force` flag may 
> > still be confusing and so we might want to deprecate it for that 
> > reason, but again, this is the interface discussion that I think we 
> > don’t actually need right now.
> >
> > And that’s why I’d prefer for any clean-up to come on top of fixing 
> > the existing bug, so we can have separate discussions and take our 
> > time with the interface without feeling driven by the need to fix a bug.
>
> OK, I see your point. Let's just fix a bug with current interface and 
> anything else is a separate step. Are you going to send v3?

Yep, sure!

(Still want to look into the exact job->ret details, though. Perhaps I 
can do better than just to keep it in the v1 state.)

Max