[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 7/8] block/backup: support bitmap sync modes for
From: |
John Snow |
Subject: |
Re: [Qemu-block] [PATCH 7/8] block/backup: support bitmap sync modes for non-bitmap backups |
Date: |
Wed, 10 Jul 2019 14:32:54 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
On 7/10/19 12:48 PM, Max Reitz wrote:
> On 10.07.19 03:05, John Snow wrote:
>> Accept bitmaps and sync policies for the other backup modes.
>> This allows us to do things like create a bitmap synced to a full backup
>> without a transaction, or start a resumable backup process.
>>
>> Some combinations don't make sense, though:
>>
>> - NEVER policy combined with any non-BITMAP mode doesn't do anything,
>> because the bitmap isn't used for input or output.
>> It's harmless, but is almost certainly never what the user wanted.
>>
>> - sync=NONE is more questionable. It can't use on-success because this
>> job never completes with success anyway, and the resulting artifact
>> of 'always' is suspect: because we start with a full bitmap and only
>> copy out segments that get written to, the final output bitmap will
>> always be ... a fully set bitmap.
>>
>> Maybe there's contexts in which bitmaps make sense for sync=none,
>> but not without more severe changes to the current job, and omitting
>> it here doesn't prevent us from adding it later.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> block/backup.c | 8 +-------
>> blockdev.c | 22 ++++++++++++++++++++++
>> qapi/block-core.json | 6 ++++--
>> 3 files changed, 27 insertions(+), 9 deletions(-)
>
> [...]
>
>> diff --git a/blockdev.c b/blockdev.c
>> index f0b7da53b0..bc152f8e0d 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>
> [...]
>
>> + if (!backup->has_bitmap && backup->has_bitmap_mode) {
>> + error_setg(errp, "Cannot specify Bitmap sync mode without a
>> bitmap");
>
> Any reason for capitalizing the first “Bitmap”?
>
> With a reason or it fixed:
>
> Reviewed-by: Max Reitz <address@hidden>
>
Hanging around Germans too much?
Actually, I can explain why: because a "bitmap" is a generic term, but
whenever I capitalize it as "Bitmap" I am referring to a Block Dirty
Bitmap which is a specific sort of thing. I do this unconsciously.
But in that case, I should actually be consistent in the interface (and
docstrings and docs and error strings) and always call it that specific
thing, which I don't.
"bitmap" is probably more correct for now, but I ought to go through all
the interface and make it consistent in some way or another.
(Actually: I'd like to see if I can't rename the "BdrvDirtyBitmap"
object to something more generic and shorter so I can rename many of the
functions we have something shorter.
Because the structure is "BdrvDirtyBitmap", there's some confusion when
we name functions like bdrv_dirty_bitmap_{verb} because it's not clear
if this is a bdrv function that does something with dirty bitmaps, or if
it's a "BdrvDirtyBitmap" function that does something with that object.
I guess that seems like a subtle point, but it's why the naming
conventions in dirty-bitmap.c are all over the place. I think at one
point, the idea was that:
bdrv_{verb}_dirty_bitmap was an action applied to a BDS that happened to
do something with dirty bitmaps. bdrv_dirty_bitmap_{verb} was an action
that applied to a "BdrvDirtyBitmap". Crystal clear and not confusing at
all, right?
It'd be nice to have functions that operate on a node be named
bdrv_dbitmap_foo() and functions that operate on the bitmap structure
itself named just dbitmap_bar().
Would it be okay if I named them such a thing, I wonder?
we have "bitmap" and "hbitmap" already, I could do something like
"dbitmap" for "dirty bitmap" or some such. Kind of an arbitrary change I
admit, but I'm itching to do a big spring cleaning in dirty-bitmap.c
right after this series is done.)
[Qemu-block] [PATCH 3/8] iotests/257: Refactor backup helpers, John Snow, 2019/07/09
[Qemu-block] [PATCH 8/8] iotests/257: test traditional sync modes, John Snow, 2019/07/09