Re: [RFC 0/4] mirror: implement incremental and bitmap modes

[Vladimir Sementsov-Ogievskiy]

On 29.02.24 17:50, Fiona Ebner wrote:
> Am 29.02.24 um 13:00 schrieb Vladimir Sementsov-Ogievskiy:
> > But anyway, this all could be simply achieved with
> > bitmap-copying/merging API, if we allow to pass user-given bitmap to the
> > mirror as working bitmap.
> >
> > > I see, I'll drop the 'bitmap-mode' in the next version if nobody
> > > complains :)
> >
> > Good. It's a golden rule: never make public interfaces which you don't
> > actually need for production. I myself sometimes violate it and spend
> > extra time on developing features, which we later have to just drop as
> > "not needed downstream, no sense in upstreaming".
>
> Just wondering which new mode I should allow for the @MirrorSyncMode
> then? The documentation states:
>
> > # @incremental: only copy data described by the dirty bitmap.
> > #     (since: 2.4)
> > #
> > # @bitmap: only copy data described by the dirty bitmap.  (since: 4.2)
> > #     Behavior on completion is determined by the BitmapSyncMode.
>
> For backup, do_backup_common() just maps @incremental to @bitmap +
> @bitmap-mode == @on-success.
>
> Using @bitmap for mirror would lead to being at odds with the
> documentation, because it mentions the BitmapSyncMode, which mirror
> won't have.
>
> Using @incremental for mirror would be consistent with the
> documentation, but behave a bit differently from backup.
>
> Opinions?

Good question.

As we already understood, (block-)job-api needs some spring-cleaning. 
Unfortunately I don't have much time on it, but still I decided to start from 
finally depreacting block-job-* API and moving to job-*.. Probably 
bitmap/bitmap-mode/sync APIs also need some optimization, keeping in mind new 
block-dirty-bitmap-merge api.

So, what I could advice in this situation for newc interfaces:

1. be minimalistic
2. add `x-` prefix when unsure

So, following these two rules, what about x-bitmap field, which may be combined 
only with 'full' mode, and do what you need?

About documentation: actually, I never liked that we use for backup job "MirrorSyncMode". 
Now it looks more like "BackupSyncMode", having two values supported only by backup.

I'm also unsure how mode=full&bitmap=some_bitmap differs from 
mode=bitmap&bitmap=some_bitmap..

So, I'd suggest simply rename MirrorSyncMode to BackupSyncMode, and add separate MirrorSyncMode with only 
"full", "top" and "none" values.

--
Best regards,
Vladimir