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

[Max Reitz]

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.

Max