Re: [Qemu-devel] [PATCH v5 0/8] Add metadata overlap checks

[Max Reitz]

On 2013-09-12 16:57, Eric Blake wrote:
> On 09/02/2013 01:25 AM, Max Reitz wrote:
> > If a qcow2 image file becomes corrupted, any write may inadvertently
> > overwrite important metadata structures such as the L1 table. This
> > series adds functionality for detecting, preventing and (to some extent)
> > repairing such collisions.
> >
> > v5:
> >   - fixed patch 6 (forgot to update the event_names array for the new
> >     event BLKDBG_REFTABLE_UPDATE); no other changes
> >
> > v4:
> >   - fixed handling of preallocated zero clusters in patch 4
> >   - moved OFLAG_COPIED checks into a separate function (this affects
> >     patches 4 and 5); functionality remains unchanged
> >   - patches 1, 2, 3, 6, 7 and 8 remain unmodified (except for line
> >     numbers in block/qcow2-refcount.c)
> Just now looking at this series, and I have several questions.
>
> It looks like Kevin applied v4 rather than v5; have we fixed that up?
Seems like it – I have the change (from v4 to v5) in master (see 
block/blkdebug.c:171).

> Next, what sort of overhead do these new checks add to the write case?
> Is it something that would be a noticeable slowdown?  I'd love to see
> some benchmark numbers (hopefully, the default set of checks are in the
> noise compared to the overhead of actual I/O).
I'll do my best to get some benchmarking done.

> Also, is there a way to tune the set of checks used at runtime, or are
> we stuck with the compiled-in default?  That is, can a user opt in to
> more expensive tests for robustness, or opt out of default tests for
> speed, via a runtime command, or is it something where they have to
> recompile to choose a different QCOW2_OL_DEFAULT value?
Right now, it's obviously a compile-time constant. However, I think we 
could also define QCOW2_OL_DEFAULT to be a runtime variable, however, 
that variable would most probably be stored in the BDRVQcowState 
structure. We could take advantage of that structure's instance being 
named "s" almost universally, therefore we could define QCOW2_OL_DEFAULT 
to be s->default_overlap_check or something like that. It would 
obviously be kind of ugly, but it should work pretty well.

On the other hand, we could replace QCOW2_OL_DEFAULT manually everywhere 
by s->default_overlap_check; or, we change the macro to take s as a 
parameter.

Either way, I see three answers to your question:

First, right now, we're basically stuck with a compile time constant.

Second, if a user really wants to, he could always define the macro to 
represent some variable. This should work pretty well already, although 
this variable has to be defined first, of course.

Third, it wouldn't be too much of a problem to write a follow-up patch 
manually replacing every QCOW2_OL_DEFAULT occurrence by a true variable 
(such as default_overlap_check from the current BDRVQcowState 
structure). I'd just undefine the macro and work down every compiler 
error. ;-)

On the other hand, now, that I think about it, we could also invert the 
current program logic: qcow2_check_metadata_overlap would then no longer 
receive a bitmask of structures to check for overlaps, but rather a 
bitmask of structures to ignore, since it can figure out 
s->default_overlap_check by itself.


Max