Re: [PATCH for-6.0? 1/3] job: Add job_wait_unpaused() for block-job-complete

[Max Reitz <mreitz@redhat.com>]

On 08.04.21 19:26, Vladimir Sementsov-Ogievskiy wrote:
> 08.04.2021 20:04, John Snow wrote:
>> On 4/8/21 12:58 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> job-complete command is async. Can we instead just add a boolean like 
>>> job->completion_requested, and set it if job-complete called in 
>>> STANDBY state, and on job_resume job_complete will be called 
>>> automatically if this boolean is true?
>>
>> job_complete has a synchronous setup, though -- we lose out on a lot 
>> of synchronous error checking in that circumstance.
> 
> yes, that's a problem..
> 
>>
>> I was not able to audit it to determine that it'd be safe to attempt 
>> that setup during a drained section -- I imagine it won't work and 
>> will fail, though.
>>
>> So I thought we'd have to signal completion and run the setup *later*, 
>> but what do we do if we get an error then? Does the entire job fail? 
>> Do we emit some new event? ("BLOCK_JOB_COMPLETION_FAILED" ?) Is it 
>> recoverable?
>>
> 
> Isn't it possible even now, that after successful job-complete job still 
> fails and we report BLOCK_JOB_COMPLETED with error?
> 
> And actually, how much benefit user get from the fact that job-complete 
> may fail?
> 
> We can make job-complete a simple always-success boolean flag setter 
> like job-pause.

I wanted to say the following:

   But job-pause does always succeed, in contrast to block-job-complete.

   block-job-complete is more akin to job-finalize, which too is a
   synchronous operation.

But when I wrote that last sentence, I asked myself whether what 
mirror_complete() does isn’t actually a remnant of what we had to do 
when we didn’t have job-finalize yet.  Shouldn’t that all be in 
mirror_exit_common()?  What’s the advantage of opening the backing chain 
or putting blockers on the to-replace node in block-job-complete? 
Aren’t that all graph-changing operation, basically, i.e. stuff that 
should be done in job-finalize?

If we move everything to mirror_exit_common(), all that remains to do is 
basically set some should_complete flag (could even be part of the Job 
struct), and then the whole problem disappears.

Thoughts?

Max

> And actual completion will be done in background, when possible. And if 
> it fail, job just fails, like it does for any background io error. And 
> user have to check error/success status of final BLOCK_JOB_COMPLETED 
> anyway.
>