Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 03.11.23 18:56, Markus Armbruster wrote:
> Kevin Wolf<kwolf@redhat.com>  writes:
>
> > Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
> > > Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>  writes:
> > >
> > > > On 11.10.23 13:18, Fiona Ebner wrote:
> > > > > Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
> > > > > > On 09.10.23 12:46, Fiona Ebner wrote:
> > > > > > > Initially, I tried to go for a more general 'job-change' command, but
> > > > > > > I couldn't figure out a way to avoid mutual inclusion between
> > > > > > > block-core.json and job.json.
> > > > > > What is the problem with it? I still think that job-change would be better.
> > > > > If going for job-change in job.json, the dependencies would be
> > > > > job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
> > > > > query-jobs -> JobInfo -> JobInfoMirror
> > > > > and we can't include block-core.json in job.json, because an inclusion
> > > > > loop gives a build error.
> > > Let me try to understand this.
> > >
> > > Command job-change needs its argument type JobChangeOptions.
> > >
> > > JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
> > > branches.
> > >
> > > JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
> > >
> > > block-core.json needs job.json for JobType and JobStatus.
> > >
> > > > > Could be made to work by moving MirrorCopyMode (and
> > > > > JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
> > > > > can be included by both job.json and block-core.json. Moving the
> > > > > type-specific definitions to the general job.json didn't feel right to
> > > > > me. Including another file with type-specific definitions in job.json
> > > > > feels slightly less wrong, but still not quite right and I didn't want
> > > > > to create a new file just for MirrorCopyMode (and
> > > > > JobChangeOptionsMirror, JobInfoMirror).
> > > > > And going further and moving all mirror-related things to a separate
> > > > > file would require moving along things like NewImageMode with it or
> > > > > create yet another file for such general things used by multiple block-jobs.
> > > > > If preferred, I can try and go with some version of the above.
> > > > OK, I see the problem. Seems, that all requires some good refactoring. But 
> > > > that's a preexisting big work, and should not hold up your series. I'm OK to 
> > > > proceed with block-job-change.
> > > Saving ourselves some internal refactoring is a poor excuse for
> > > undesirable external interfaces.
> > I'm not sure how undesirable it is. We have block-job-* commands for
> > pretty much every other operation, so it's only consistent to have
> > block-job-change, too.
> Is the job abstraction a failure?
>
> We have
>
>      block-job- command      since   job- command    since
>      -----------------------------------------------------
>      block-job-set-speed     1.1
>      block-job-cancel        1.1     job-cancel      3.0
>      block-job-pause         1.3     job-pause       3.0
>      block-job-resume        1.3     job-resume      3.0
>      block-job-complete      1.3     job-complete    3.0
>      block-job-dismiss       2.12    job-dismiss     3.0
>      block-job-finalize      2.12    job-finalize    3.0
>      block-job-change        8.2
>      query-block-jobs        1.1     query-jobs
>
> I was under the impression that we added the (more general) job-
> commands to replace the (less general) block-job commands, and we're
> keeping the latter just for compatibility.  Am I mistaken?
>
> Which one should be used?
>
> Why not deprecate the one that shouldn't be used?
>
> The addition of block-job-change without even trying to do job-change
> makes me wonder: have we given up on the job- interface?
>
> I'm okay with giving up on failures.  All I want is clarity.  Right now,
> I feel thoroughly confused about the status block-jobs and jobs, and how
> they're related.

Hi! I didn't notice, that the series was finally merged.

About the APIs, I think, of course we should deprecate block-job-* API, because 
we already have jobs which are not block-jobs, so we can't deprecate job-* API.

So I suggest a plan:

1. Add job-change command simply in block-core.json, as a simple copy of 
block-job-change, to not care with resolving inclusion loops. (ha we could 
simply name our block-job-change to be job-change and place it in 
block-core.json, but now is too late)

2. Support changing speed in a new job-chage command. (or both in 
block-job-change and job-change, keeping them equal)

3. Deprecate block-job-* APIs

4. Wait 3 releases

5. Drop block-job-* APIs

6. Move all job-related stuff to job.json, drop `{ 'include': 'job.json' }` 
from block-core.json, and instead include block-core.json into job.json

If it's OK, I can go through the steps.

--
Best regards,
Vladimir