[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror
Date: Thu, 29 Jul 2021 19:29:15 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0

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

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 

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 

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.


[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?

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]