Re: [Qemu-devel] [BUG] qemu-1.1.2 [FIXED-BY] qcow2: Fix avail_sectors in cluster allocation code

[Kevin Wolf]

Am 12.12.2012 15:09, schrieb Philipp Hahn:
> Hello Kevin, 
> 
> Am Mittwoch 12 Dezember 2012 14:41:49 schrieb Kevin Wolf:
>> As you can see in the commit message of that patch I was convinced that
>> no bug did exist in practice and this was only dangerous with respect to
>> future changes. Therefore my first question is if you're using an
>> unmodified upstream qemu or if some backported patches are applied to
>> it? If it's indeed unmodified, we should probably review the code once
>> again to understand why it makes a difference.
> 
> This were all unmodified versions directly from git between "qemu-kvm-1.1.0" 
> and "qemu-kvm-1.2.0"
> 
> "git checkout b7ab0fea37c15ca9e249c42c46f5c48fd1a0943c" works,
> "git checkout b7ab0fea37c15ca9e249c42c46f5c48fd1a0943c~1" is broken.
> "git checkout qemu-kvm-1.1.2"  is broken,
> "git checkout qemu-kvm-1.1.2 ; git cherry-pick 
> b7ab0fea37c15ca9e249c42c46f5c48fd1a0943c"  works

Ok, thanks for clarifying. Then I must have missed some interesting case
while doing the patch.

Ideally we would find a sequence of qemu-io commands to reliably
reproduce this. First thing worth trying would be running the current
qemu-iotests suite on the old versions. If we don't find it this way, I
guess we need to catch it with code review. I'm not sure if I can get to
it this week, and starting next week I'll be on vacation, so any help
with finding a reproducer would be appreciated.

>> In any case, this is the cluster allocation code. It's probably not
>> related to rereading things from disk, but rather to the writeout of the
>> page cache.
> 
> Yes, the problem is probably write related. But as the write "doens't explode 
> with some spectacular error", I only notice the error on the following read 
> by comparing md5 sums.
> I just re-checked it: After a reboot the md5sums are still invalid, so I 
> guess 
> the data is corrupted on writeout.

Yes, it's really the only thing that makes sense in connection with this
patch.

Kevin