[Qemu-devel] Cutting a new QEMU release

[Qemu-devel] Cutting a new QEMU release

[Anthony Liguori]

What do people think?  TCG seems to be in a good place.  We've got 
virtio, KVM, live migration, tons of new devices, bsd-user, etc.

We could decide to cut one by the end of the month.  I'm already doing 
some test work in QEMU so I can follow up with some more detailed notes 
about what is working and what isn't working.  That gives us some time 
to decide if there's anything we need to fix before a release.

Regards,

Anthony Liguori

Re: [Qemu-devel] Cutting a new QEMU release

[Glauber Costa]

On Tue, Feb 3, 2009 at 6:48 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> What do people think?  TCG seems to be in a good place.  We've got virtio,
> KVM, live migration, tons of new devices, bsd-user, etc.
>
> We could decide to cut one by the end of the month.  I'm already doing some
> test work in QEMU so I can follow up with some more detailed notes about
> what is working and what isn't working.  That gives us some time to decide
> if there's anything we need to fix before a release.
>
> Regards,

I'm totally for it.

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Cutting a new QEMU release

[Laurent Desnogues]

On Tue, Feb 3, 2009 at 9:58 PM, Glauber Costa <glommer@gmail.com> wrote:
> On Tue, Feb 3, 2009 at 6:48 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> What do people think?  TCG seems to be in a good place.  We've got virtio,
>> KVM, live migration, tons of new devices, bsd-user, etc.
>>
>> We could decide to cut one by the end of the month.  I'm already doing some
>> test work in QEMU so I can follow up with some more detailed notes about
>> what is working and what isn't working.  That gives us some time to decide
>> if there's anything we need to fix before a release.
>>
>> Regards,
>
> I'm totally for it.

So am I, but who will test user mode and more generally (user and system)
what is the test procedure?

For instance someone (Andzrej?) mentionned  ARM in system mode is half
slower than it was before TCG.  Also the ARM target needs some fixing.

Perhaps doing at least one release candidate to get feedback (and focus on
fixing reported bugs) would be appropriate.

Cheers,

Laurent

Re: [Qemu-devel] Cutting a new QEMU release

[Anthony Liguori]

Laurent Desnogues wrote:
> On Tue, Feb 3, 2009 at 9:58 PM, Glauber Costa <glommer@gmail.com> wrote:
>   
>> I'm totally for it.
>>     
>
> So am I, but who will test user mode and more generally (user and system)
> what is the test procedure?
>   
I'd like to approach this gently.  Historically, there's been no formal 
release process.  I'm not inclined to start out by introducing any sort 
of heavy weight procedure.

I'll poke things as best I can over the next couple weeks.  I encourage 
everyone else to do the same.  I'll keep track of what's working and 
what's broken and make it available publicly.  At some point, we can 
decide as if things are too embarrassing to release or not :-)

> For instance someone (Andzrej?) mentionned  ARM in system mode is half
> slower than it was before TCG.  Also the ARM target needs some fixing.
>
> Perhaps doing at least one release candidate to get feedback (and focus on
> fixing reported bugs) would be appropriate.
>   

A release doesn't have to be perfect to be useful.  I think what matters 
most is whether something is likely to be fixed in the reasonably near 
future.  We're going to have some regressions compared to 0.9.1.  There 
are a number of platforms that are no longer supported (ia64 and s390, 
for instance) but we could wait another year and I doubt these features 
would appear.

Regards,

Anthony Liguori

> Cheers,
>
> Laurent
>
>
>   

Re: [Qemu-devel] Cutting a new QEMU release

[Laurent Desnogues]

On Tue, Feb 3, 2009 at 10:50 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Laurent Desnogues wrote:
[...]
>> So am I, but who will test user mode and more generally (user and system)
>> what is the test procedure?
>>
>
> I'd like to approach this gently.  Historically, there's been no formal
> release process.  I'm not inclined to start out by introducing any sort of
> heavy weight procedure.

Don't take me wrong, I am not for formal processes at all :)  I just want
to be sure user mode won't be forgotten due to the lack of a maintainer.

> I'll poke things as best I can over the next couple weeks.  I encourage
> everyone else to do the same.  I'll keep track of what's working and what's
> broken and make it available publicly.  At some point, we can decide as if
> things are too embarrassing to release or not :-)

I intend on testing various things on my side too.  Be sure I'll let you
know of problems and also will provide patches.

>> For instance someone (Andzrej?) mentionned  ARM in system mode is half
>> slower than it was before TCG.  Also the ARM target needs some fixing.
>>
>> Perhaps doing at least one release candidate to get feedback (and focus on
>> fixing reported bugs) would be appropriate.
>>
>
> A release doesn't have to be perfect to be useful.  I think what matters
> most is whether something is likely to be fixed in the reasonably near
> future.  We're going to have some regressions compared to 0.9.1.  There are
> a number of platforms that are no longer supported (ia64 and s390, for
> instance) but we could wait another year and I doubt these features would
> appear.

I agree we should not care now about targets that are not here anymore.
But things that are important for the community should be taken with
care (and arm linux user mode is certainly a very important target).


Laurent

Re: [Qemu-devel] Cutting a new QEMU release

[Anthony Liguori]

Laurent Desnogues wrote:
> On Tue, Feb 3, 2009 at 10:50 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>   
>> Laurent Desnogues wrote:
>>     
>>> For instance someone (Andzrej?) mentionned  ARM in system mode is half
>>> slower than it was before TCG.  Also the ARM target needs some fixing.
>>>
>>> Perhaps doing at least one release candidate to get feedback (and focus on
>>> fixing reported bugs) would be appropriate.
>>>
>>>       
>> A release doesn't have to be perfect to be useful.  I think what matters
>> most is whether something is likely to be fixed in the reasonably near
>> future.  We're going to have some regressions compared to 0.9.1.  There are
>> a number of platforms that are no longer supported (ia64 and s390, for
>> instance) but we could wait another year and I doubt these features would
>> appear.
>>     
>
> I agree we should not care now about targets that are not here anymore.
> But things that are important for the community should be taken with
> care (and arm linux user mode is certainly a very important target).
>   

If someone is actively fixing it, then I'm perfectly happy to wait.  If 
it's a known issue that noone is resolving, I don't think delaying a 
release helps anyone.  However, documenting all of these things 
somewhere so that they are clearly visible may make it easier for 
someone to fix so the process of going through a release would probably 
be helpful in general.

Regards,

Anthony Liguori

> Laurent
>
>
>   

Re: [Qemu-devel] Cutting a new QEMU release

[Glauber Costa]

>>
>> I agree we should not care now about targets that are not here anymore.
>> But things that are important for the community should be taken with
>> care (and arm linux user mode is certainly a very important target).
>>
>
> If someone is actively fixing it, then I'm perfectly happy to wait.  If it's
> a known issue that noone is resolving, I don't think delaying a release
> helps anyone.  However, documenting all of these things somewhere so that
> they are clearly visible may make it easier for someone to fix so the
> process of going through a release would probably be helpful in general.
>

We also have to take into account that making a release is likely to increase
the quality of qemu code base as a whole. Just because any user getting into
qemu site today, or getting code from a distro, is likely to be using
something very
old. Old enough that any bug reports will be probably useless to us.

Bug reports against 0.9.1 has happened many times already in the list.


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Cutting a new QEMU release

[Ulrich Hecht]

On Tuesday 03 February 2009, Anthony Liguori wrote:
> There are a number of platforms that are no longer 
> supported (ia64 and s390, for instance) but we could wait another year
> and I doubt these features would appear.

I am working on S/390 host support. Currently, it's good enough to show 
the PC BIOS startup screen (softmmu) and to run an i386 shell 
(linux-user). ATM I cannot give an estimate when it will be good enough 
for a release, though.

CU
Uli

-- 
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] Cutting a new QEMU release

[David Turner]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Tue, Feb 3, 2009 at 10:35 PM, Laurent Desnogues <
laurent.desnogues@gmail.com> wrote:

>
> For instance someone (Andzrej?) mentionned  ARM in system mode is half
> slower than it was before TCG.  Also the ARM target needs some fixing.
>

I have integrated the TCG ARM backend in the Android emulator, and my
measurements
show an improvement in performance, when running various Android performance
tests,
between x1.10 and x1.90 compared to the old dyngen based translator. To be
honest,
the improvements are not consistent, there are a few rare tests
that run at x0.89, but they're not critical to me).

Note that the TCG binary is compiled with GCC 4.2, while the old one was
built with
GCC 3.3 (fo rthe usual ugly dyngen reasons).

This is only when comparing the same ARMv5 binaries, but it sounds good
enough for me.

An official release would be very welcomed at this point. The amount of
changes since
the last one has been dramatic. Morever, this will allow everyone to reset
the clock on
their forks and more easily share patches with upstream.

Just my 2 cents


>
> Perhaps doing at least one release candidate to get feedback (and focus on
> fixing reported bugs) would be appropriate.
>
> Cheers,
>
> Laurent
>
>
>

[-- Attachment #2: Type: text/html, Size: 1796 bytes --]

Re: [Qemu-devel] Cutting a new QEMU release

[C.W. Betts]

I am all for releasing a release candidate.  This signals everyone  
that qemu is thinking of releasing a stable version and people will  
try to find bugs.
On Feb 3, 2009, at 2:35 PM, Laurent Desnogues wrote:

> Perhaps doing at least one release candidate to get feedback (and  
> focus on
> fixing reported bugs) would be appropriate.
>
> Cheers,
>
> Laurent
>
>

Re: [Qemu-devel] Cutting a new QEMU release

[Rick Vernam]

On Tuesday 03 February 2009 2:48:22 pm Anthony Liguori wrote:
> What do people think?  TCG seems to be in a good place.  We've got
> virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>
> We could decide to cut one by the end of the month.  I'm already doing
> some test work in QEMU so I can follow up with some more detailed notes
> about what is working and what isn't working.  That gives us some time
> to decide if there's anything we need to fix before a release.
>
> Regards,
>
> Anthony Liguori

-vga vmware doesn't work on either of my wxp guests, nor my w2k guest.

I continue to get triple faults from any Windows XP guest at a particular 
point while booting, when invoked with qemu-system-x86_64.
When invoked with plain qemu, it seems to work just fine.
This is with or without -kqemu or -kernel-kqemu (my athlon 3700+ doesn't do 
kvm).

I've posted about these in the past, and I don't have any additional 
information to add to those (ancient) discussions.  I don't intend to start a 
discussion about them now, this is just a friendly reminder about some 
unresolved issues.

Thanks
-Rick

Re: [Qemu-devel] Cutting a new QEMU release

[Daniel P. Berrange]

On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
> What do people think?  TCG seems to be in a good place.  We've got 
> virtio, KVM, live migration, tons of new devices, bsd-user, etc.

I'd like to see a new release if at all practical. For Fedora there is a
push to ship KVM and QEMU packages based off the same source tree to make
patching security flaws more pratical. Given that KVM ships off a QEMU
SVN snapshot, having a single source tree would mean shipping our full
multi-arch QEMU package off a SVN snapshot too. I don't find this a
particularly appealing thing - if CVS snapshot is stable enough for it
to be exposed to Fedora users, I'd like to think QEMU developers would
be happy with a official release. If the QEMU dev community considers 
the code too unstable to release, then exposing it to Fedora users seem 
sub-optimal. 

Personally I test & use the i386 and x86_64 system emulator parts of
QEMU, and those seem generally stable enough to base a new release off.
So I'd welcome a new release from that POV. I'll leave others to
comment on quality of the other arch targets. 

> We could decide to cut one by the end of the month.  I'm already doing 
> some test work in QEMU so I can follow up with some more detailed notes 
> about what is working and what isn't working.  That gives us some time 
> to decide if there's anything we need to fix before a release.

A QEMU release by the end of the month would work pretty well for the 
time scale we're working on to get stuff into the Fedora 11 release 
too. 

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Cutting a new QEMU release

[Aurelien Jarno]

On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
> What do people think?  TCG seems to be in a good place.  We've got  
> virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>
> We could decide to cut one by the end of the month.  I'm already doing  
> some test work in QEMU so I can follow up with some more detailed notes  
> about what is working and what isn't working.  That gives us some time  
> to decide if there's anything we need to fix before a release.
>

That's a really good idea.

I would like to see the switch of the remaining PowerPC machine from
OpenHackware to OpenBIOS. We don't have the sources of the current
ppc_rom.bin binary, and I don't feel comfortable making a release with
it. We probably have the sources of an older version.

This at least concerned ppc_chrp.c (ppc_prep.c could probably simply
be dropped).

I have no idea about how long it would take.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Cutting a new QEMU release

[Tristan Gingold]

On Feb 4, 2009, at 3:50 PM, Aurelien Jarno wrote:
>
> This at least concerned ppc_chrp.c (ppc_prep.c could probably simply
> be dropped).

Please, don't drop ppc_prep.c.  Even if it is not supported by  
OpenBios I know several uses of prep
with direct images.

Tristan.

Re: [Qemu-devel] Cutting a new QEMU release

[Lennart Sorensen]

On Wed, Feb 04, 2009 at 04:23:10PM +0100, Tristan Gingold wrote:
> Please, don't drop ppc_prep.c.  Even if it is not supported by  
> OpenBios I know several uses of prep
> with direct images.

Who still support it?

The linux kernel seems to have thrown away the prep support code as of
2.6.27.

-- 
Len Sorensen

Re: [Qemu-devel] Cutting a new QEMU release

[Tristan Gingold]

On Feb 4, 2009, at 4:43 PM, Lennart Sorensen wrote:

> On Wed, Feb 04, 2009 at 04:23:10PM +0100, Tristan Gingold wrote:
>> Please, don't drop ppc_prep.c.  Even if it is not supported by
>> OpenBios I know several uses of prep
>> with direct images.
>
> Who still support it?

I know at least one non-free OS that runs on prep.
We also create raw programs that runs on prep (yes we could switch to  
chrp)

> The linux kernel seems to have thrown away the prep support code as of
> 2.6.27.

Yes, but the world is not only linux 2.6.27+ :-)

[Qemu-devel] Re: Cutting a new QEMU release

[Consul]

> 
> Yes, but the world is not only linux 2.6.27+ :-)
> 

Of course not. All the world's a VAX!

Re: [Qemu-devel] Cutting a new QEMU release

[Blue Swirl]

On 2/4/09, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
>
> > What do people think?  TCG seems to be in a good place.  We've got
>  > virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>  >
>  > We could decide to cut one by the end of the month.  I'm already doing
>  > some test work in QEMU so I can follow up with some more detailed notes
>  > about what is working and what isn't working.  That gives us some time
>  > to decide if there's anything we need to fix before a release.
>  >
>
>
> That's a really good idea.
>
>  I would like to see the switch of the remaining PowerPC machine from
>  OpenHackware to OpenBIOS. We don't have the sources of the current
>  ppc_rom.bin binary, and I don't feel comfortable making a release with
>  it. We probably have the sources of an older version.
>
>  This at least concerned ppc_chrp.c (ppc_prep.c could probably simply
>  be dropped).
>
>  I have no idea about how long it would take.

PPC development on OpenBIOS side is taking quick leaps, I'd hate to
rush a release just now when we are very close to a fully working
system.

On Sparc32/64 side, things are moving more slowly. Sparc32 is pretty
much release quality with support for Linux, OpenBSD and NetBSD boot
and a lot of boards. Sparc64 is still unusable, but it's not worth
waiting for.

Re: [Qemu-devel] Cutting a new QEMU release

[Jonathan Kalbfeld]

Are the host details fixed on Solaris/SPARC?

Does anyone want access to my build environment to try and make it work?

I haven't gotten anything since the January 13, 2008 release to work
without SIGSEGV on a sparc.

jonathan

On Wed, Feb 4, 2009 at 9:39 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On 2/4/09, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
>>
>> > What do people think?  TCG seems to be in a good place.  We've got
>>  > virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>>  >
>>  > We could decide to cut one by the end of the month.  I'm already doing
>>  > some test work in QEMU so I can follow up with some more detailed notes
>>  > about what is working and what isn't working.  That gives us some time
>>  > to decide if there's anything we need to fix before a release.
>>  >
>>
>>
>> That's a really good idea.
>>
>>  I would like to see the switch of the remaining PowerPC machine from
>>  OpenHackware to OpenBIOS. We don't have the sources of the current
>>  ppc_rom.bin binary, and I don't feel comfortable making a release with
>>  it. We probably have the sources of an older version.
>>
>>  This at least concerned ppc_chrp.c (ppc_prep.c could probably simply
>>  be dropped).
>>
>>  I have no idea about how long it would take.
>
> PPC development on OpenBIOS side is taking quick leaps, I'd hate to
> rush a release just now when we are very close to a fully working
> system.
>
> On Sparc32/64 side, things are moving more slowly. Sparc32 is pretty
> much release quality with support for Linux, OpenBSD and NetBSD boot
> and a lot of boards. Sparc64 is still unusable, but it's not worth
> waiting for.
>
>
>



-- 
--
Jonathan Kalbfeld
ThoughtWave Technologies LLC
www.thoughtwave.com

"Yes, we did!"

Learn UNIX For Free at unixlessons.com

Re: [Qemu-devel] Cutting a new QEMU release

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On 2/4/09, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
>
> > What do people think?  TCG seems to be in a good place.  We've got
>  > virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>  >
>  > We could decide to cut one by the end of the month.  I'm already doing
>  > some test work in QEMU so I can follow up with some more detailed notes
>  > about what is working and what isn't working.  That gives us some time
>  > to decide if there's anything we need to fix before a release.
>  >
>
>
> That's a really good idea.
>
>  I would like to see the switch of the remaining PowerPC machine from
>  OpenHackware to OpenBIOS. We don't have the sources of the current
>  ppc_rom.bin binary, and I don't feel comfortable making a release with
>  it. We probably have the sources of an older version.
>
>  This at least concerned ppc_chrp.c (ppc_prep.c could probably simply
>  be dropped).
>
>  I have no idea about how long it would take.

15 minutes :-), though for Qemu side only. The attached patch switches
CHRP to OpenBIOS. The screen flashes with something, some more stuff
is needed on OpenBIOS side.

[-- Attachment #2: chrp_use_openbios.diff --]
[-- Type: plain/text, Size: 2327 bytes --]

Re: [Qemu-devel] Cutting a new QEMU release

[Stuart Brady]

On Wed, Feb 04, 2009 at 03:50:52PM +0100, Aurelien Jarno wrote:
> We don't have the sources of the current ppc_rom.bin binary, and I
> don't feel comfortable making a release with it. We probably have the
> sources of an older version.

Ouch! :(

I noticed that the upstream site for Open Hack'Ware disappeared a while
ago...  Various distros still have the source (and QEMU has a patch
against it, pc-bios/ohw.diff), and I thought that it had not been
modified in quite a while...

I'm glad that OpenBIOS is doing well, but if we really have lost some of
the Open Hack'Ware source, that's slightly disconcerting (even if it's
not really needed any longer.)

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] Cutting a new QEMU release

[Glauber Costa]

On Tue, Feb 3, 2009 at 6:48 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> What do people think?  TCG seems to be in a good place.  We've got virtio,
> KVM, live migration, tons of new devices, bsd-user, etc.
>
> We could decide to cut one by the end of the month.  I'm already doing some
> test work in QEMU so I can follow up with some more detailed notes about
> what is working and what isn't working.  That gives us some time to decide
> if there's anything we need to fix before a release.

As a curiosity, what would be the preferred version number? 0.9.2?
0.10 ? 1.0 ? "Phoenix"?
"Crisis Release" ?


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Cutting a new QEMU release

[Shin-ichiro KAWASAKI]

Anthony Liguori wrote:
> What do people think?  TCG seems to be in a good place.  We've got 
> virtio, KVM, live migration, tons of new devices, bsd-user, etc.

The development of sh4 is rather slow, you know. Then there is no need to
think about it to decide when to cut the next version. 

>From the point of view from sh4 system emulation, that's a good news. 
It is a good way to provide current features for sh4 developers.
But before release, I hope these two points would handled by anyone.

[1] USB support

Current sh4 system emulation (r2d board) does not support USB host.
Without it, the graphic console does not receive any key input
via USB keyboard emulation.  Then, graphics console is not available
now.  I guess sh4 developers would feel it strange.

Following patch adds usb host.

    http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01620.html

It does not apply to current svn head, because of line mismatch.
I'm willing to post new version, if the patch is OK.
Could anyone review it?


[2] sh4 kernel & disk image on QEMU's download page

It's a bothering work to make kernel & disk image for sh4.  Not to obstruct
sh4 developers with it, I provide a small (4MB) set at following URL.

http://www.assembla.com/spaces/qemu-sh4/documents/b18oeq850r3AhNab7jnrAJ/download?filename=sh-test-0.1.tar.bz2

Could anyone put the image at QEMU's download page?
  http://bellard.org/qemu/download.html

I think it is the best place to provide it.


Regards,
Shin-ichiro KAWASAKI

Re: [Qemu-devel] Cutting a new QEMU release

[Rob Landley]

On Saturday 07 February 2009 09:29:42 Shin-ichiro KAWASAKI wrote:
> Anthony Liguori wrote:
> > What do people think?  TCG seems to be in a good place.  We've got
> > virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>
> The development of sh4 is rather slow, you know. Then there is no need to
> think about it to decide when to cut the next version.
>
> From the point of view from sh4 system emulation, that's a good news.
> It is a good way to provide current features for sh4 developers.
> But before release, I hope these two points would handled by anyone.
>
> [1] USB support
>
> Current sh4 system emulation (r2d board) does not support USB host.
> Without it, the graphic console does not receive any key input
> via USB keyboard emulation.  Then, graphics console is not available
> now.  I guess sh4 developers would feel it strange.
>
> Following patch adds usb host.
>
>     http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01620.html
>
> It does not apply to current svn head, because of line mismatch.
> I'm willing to post new version, if the patch is OK.
> Could anyone review it?
>
>
> [2] sh4 kernel & disk image on QEMU's download page
>
> It's a bothering work to make kernel & disk image for sh4.  Not to obstruct
> sh4 developers with it, I provide a small (4MB) set at following URL.
>
> http://www.assembla.com/spaces/qemu-sh4/documents/b18oeq850r3AhNab7jnrAJ/do
>wnload?filename=sh-test-0.1.tar.bz2

I downloaded this and tried it out with an svn snapshot from today (svn 6613), 
built on Ubuntu 8.10 with default "./configure; make; sudo make install".


Your README has a typo, it says "-kernel r2d_zImage" but the one you've 
packaged is just "zImage".  Once that's worked around, it pops up a qemu 
window within which it boots to a login prompt, but I can't type anything.  
This is the USB issue you mentioned, confirmed by the stdout from qemu:

  char device redirected to /dev/pts/14
  Warning: could not add USB device keyboard
  long read to SH7750_WCR1_A7 (0x000000001f800008) ignored
  long read to SH7750_WCR2_A7 (0x000000001f80000c) ignored
  long read to SH7750_WCR3_A7 (0x000000001f800010) ignored
  long read to SH7750_MCR_A7 (0x000000001f800014) ignored
  long read to SH7750_MCR_A7 (0x000000001f800014) ignored

I thought I'd work around that with a serial console (which is what I actually 
use for in my FWL project anyway), but I can't get that to work either.  Your 
command line -appends "console=ttySC0,115200" and "early_printk=serial" but 
ctrl-alt-2 shows nothing, and booting with
-nographic gives no output.

Also, going back to ctrl-alt-1 doesn't redraw the vga screen.  (The screen 
will partially redraw itself if it's still producing output, but it never 
redraws the penguin logo at the top of the frame buffer, and if the console 
has finished producing output it just stays black from then on.)

This is more functionality than I've ever gotten out of sh4, and I really look 
forward to adding sh4 support to http://impactlinux.com/fwl .  If I can get a 
serial console working I'm probably good to go, but right now it's not quite 
working for me yet...

Your README says that you can extract the config.gz from the kernel with 
linux/scripts/extract-ikconfig, but when I tried it it said:

  ERROR: Unable to extract kernel configuration information.
         This kernel image may not have the config info.

Obviously, I can't get it by logging in and catting /proc/config.gz without a 
working keyboard or serial console...

Rob

Re: [Qemu-devel] Cutting a new QEMU release

[Shin-ichiro KAWASAKI]

Rob Landley wrote:
> On Saturday 07 February 2009 09:29:42 Shin-ichiro KAWASAKI wrote:
>> Anthony Liguori wrote:
>>> What do people think?  TCG seems to be in a good place.  We've got
>>> virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>> The development of sh4 is rather slow, you know. Then there is no need to
>> think about it to decide when to cut the next version.
>>
>> From the point of view from sh4 system emulation, that's a good news.
>> It is a good way to provide current features for sh4 developers.
>> But before release, I hope these two points would handled by anyone.
>>
>> [1] USB support
>>
>> Current sh4 system emulation (r2d board) does not support USB host.
>> Without it, the graphic console does not receive any key input
>> via USB keyboard emulation.  Then, graphics console is not available
>> now.  I guess sh4 developers would feel it strange.
>>
>> Following patch adds usb host.
>>
>>     http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01620.html
>>
>> It does not apply to current svn head, because of line mismatch.
>> I'm willing to post new version, if the patch is OK.
>> Could anyone review it?
>>
>>
>> [2] sh4 kernel & disk image on QEMU's download page
>>
>> It's a bothering work to make kernel & disk image for sh4.  Not to obstruct
>> sh4 developers with it, I provide a small (4MB) set at following URL.
>>
>> http://www.assembla.com/spaces/qemu-sh4/documents/b18oeq850r3AhNab7jnrAJ/do
>> wnload?filename=sh-test-0.1.tar.bz2
> 
> I downloaded this and tried it out with an svn snapshot from today (svn 6613), 
> built on Ubuntu 8.10 with default "./configure; make; sudo make install".

Thank you Rob for trying, and sorry for my mistakes in the package.
I thought again about the contents again, and have uploaded it on the same
URL as before.


> Your README has a typo, it says "-kernel r2d_zImage" but the one you've 
> packaged is just "zImage".  Once that's worked around, it pops up a qemu 
> window within which it boots to a login prompt, but I can't type anything.  
> This is the USB issue you mentioned, confirmed by the stdout from qemu:
> 
>   char device redirected to /dev/pts/14
>   Warning: could not add USB device keyboard
>   long read to SH7750_WCR1_A7 (0x000000001f800008) ignored
>   long read to SH7750_WCR2_A7 (0x000000001f80000c) ignored
>   long read to SH7750_WCR3_A7 (0x000000001f800010) ignored
>   long read to SH7750_MCR_A7 (0x000000001f800014) ignored
>   long read to SH7750_MCR_A7 (0x000000001f800014) ignored
> 
> I thought I'd work around that with a serial console (which is what I actually 
> use for in my FWL project anyway), but I can't get that to work either.  Your 
> command line -appends "console=ttySC0,115200" and "early_printk=serial" but 
> ctrl-alt-2 shows nothing, and booting with
> -nographic gives no output.
> 
> Also, going back to ctrl-alt-1 doesn't redraw the vga screen.  (The screen 
> will partially redraw itself if it's still producing output, but it never 
> redraws the penguin logo at the top of the frame buffer, and if the console 
> has finished producing output it just stays black from then on.)

Sorry to say, I completely missed to add '-serial null -serial stdio' in the
command line example.  Could you try following line again?

  % ./qemu-system-sh4 -M r2d -kernel zImage -hda sh-linux-mini.img -serial null -serial stdio -nographic

I hope you'll see the shell prompt.


> This is more functionality than I've ever gotten out of sh4, and I really look 
> forward to adding sh4 support to http://impactlinux.com/fwl .  If I can get a 
> serial console working I'm probably good to go, but right now it's not quite 
> working for me yet...
> 
> Your README says that you can extract the config.gz from the kernel with 
> linux/scripts/extract-ikconfig, but when I tried it it said:
> 
>   ERROR: Unable to extract kernel configuration information.
>          This kernel image may not have the config info.
> 
> Obviously, I can't get it by logging in and catting /proc/config.gz without a 
> working keyboard or serial console...

On my Ubuntu 8.04 env, I can't get config with scripts/extract-ikconfig.
On the other hand, with Ubuntu 8.10 env, I can.  I'm not sure about the reason.
Anyway, I removed the explanation about the way to get config before booting.

Additionally, I modified the default kernel boot options slightly,
and ran fsck.ext2 on the disk image before packing.
I hope the package is appropriate for qemu-sh users, now.

I'm looking forward the fwl system image.  I've been struggling
to get stable userland on which gcc is available.


Regards,
Shin-ichiro KAWASAKI

Re: [Qemu-devel] Cutting a new QEMU release

[Rob Landley]

On Thursday 12 February 2009 08:44:48 Shin-ichiro KAWASAKI wrote:
> Sorry to say, I completely missed to add '-serial null -serial stdio' in
> the command line example.  Could you try following line again?
>
>   % ./qemu-system-sh4 -M r2d -kernel zImage -hda sh-linux-mini.img -serial
> null -serial stdio -nographic
>
> I hope you'll see the shell prompt.

Yes I did!  Cool!

Thanks, that's what I needed.

> I'm looking forward the fwl system image.  I've been struggling
> to get stable userland on which gcc is available.

I'll try to make it work this evening.

Thank you very much,

Rob

Re: [Qemu-devel] Cutting a new QEMU release

[Rob Landley]

On Thursday 12 February 2009 08:44:48 Shin-ichiro KAWASAKI wrote:
> Rob Landley wrote:
> > On Saturday 07 February 2009 09:29:42 Shin-ichiro KAWASAKI wrote:

>   % ./qemu-system-sh4 -M r2d -kernel zImage -hda sh-linux-mini.img -serial
> null -serial stdio -nographic
>
> I hope you'll see the shell prompt.

FYI, here's something I type from that shell prompt which made qemu-system-sh4 
unhappy:

# reboot
The system is going down NOW!
Sending SIGTERM to all processes
Sending SIGKILL to all processes
Requesting system reboot
Restarting system.
Unauthorized access
qemu: fatal: Trying to execute code outside RAM or ROM at 0xa0000000

pc=0xa0000000 sr=0x700000f0 pr=0x8c03864c fpscr=0x00080000
spc=0x8c0126a6 ssr=0x10000000 gbr=0x2975b450 vbr=0x8c018000
sgr=0x8f989e8c dbr=0x00000000 delayed_pc=0x8c0126a0 fpul=0x00000000
r0=0x00000016 r1=0x80000001 r2=0x10000000 r3=0x0000198e
r4=0x00000000 r5=0x0000198e r6=0xffffffff r7=0xffffffff
r8=0x28121969 r9=0xfee1dead r10=0x000001a0 r11=0x01234567
r12=0x297577b8 r13=0x004c2c10 r14=0x7bea3aa0 r15=0x8f989e8c
r16=0x00000000 r17=0xffffff0f r18=0xffffffff r19=0x40008000
r20=0x8f989e08 r21=0x00000000 r22=0x00000000 r23=0x8f988000


Just FYI. :)

Rob

Re: [Qemu-devel] Cutting a new QEMU release

[Mark McLoughlin]

On Tue, 2009-02-03 at 14:48 -0600, Anthony Liguori wrote:
> What do people think?  TCG seems to be in a good place.  We've got 
> virtio, KVM, live migration, tons of new devices, bsd-user, etc.
> 
> We could decide to cut one by the end of the month.  I'm already doing 
> some test work in QEMU so I can follow up with some more detailed notes 
> about what is working and what isn't working.  That gives us some time 
> to decide if there's anything we need to fix before a release.

Sounds great to me.

>From a Fedora perspective, qemu-0.9.1 is a year old and upstream has
moved on a lot. As a package maintainer, it's hard to justify caring too
much about bugs reported against 0.9.1, since the bug is likely to have
very little relevance to the latest upstream.

Also, it would be really nice to have a kvm-userspace based off a solid
qemu release ... qemu moving so fast is great, but it means it's hard to
predict the stability of a given kvm-userspace release.

Some questions:

  - Will there be a period before the release when only bug fixes are 
    merged?

  - Will there be a release candidate?

  - Is there any missing features that we might push out the release
    date for?

  - Post-release, is there any interest in maintaining a stable branch 
    until the next release?

  - The plan for the next release is roughly 6 months, yes?

Thanks,
Mark.

Re: [Qemu-devel] Cutting a new QEMU release

[Anthony Liguori]

Mark McLoughlin wrote:
> On Tue, 2009-02-03 at 14:48 -0600, Anthony Liguori wrote:
>   
>> What do people think?  TCG seems to be in a good place.  We've got 
>> virtio, KVM, live migration, tons of new devices, bsd-user, etc.
>>
>> We could decide to cut one by the end of the month.  I'm already doing 
>> some test work in QEMU so I can follow up with some more detailed notes 
>> about what is working and what isn't working.  That gives us some time 
>> to decide if there's anything we need to fix before a release.
>>     
>
> Sounds great to me.
>
> >From a Fedora perspective, qemu-0.9.1 is a year old and upstream has
> moved on a lot. As a package maintainer, it's hard to justify caring too
> much about bugs reported against 0.9.1, since the bug is likely to have
> very little relevance to the latest upstream.
>
> Also, it would be really nice to have a kvm-userspace based off a solid
> qemu release ... qemu moving so fast is great, but it means it's hard to
> predict the stability of a given kvm-userspace release.
>
> Some questions:
>
>   - Will there be a period before the release when only bug fixes are 
>     merged?
>   

It's a good idea, but it may be hard to pull off practically speaking 
for the first release.  Let's see how it works out.

>   - Will there be a release candidate?
>   

Sometime this week, I'll try to post something summarizing our current 
state and anything outstanding.  If there's time to put out an -rc, I'll 
try to make one available.  Things may hiccup a bit.

>   - Is there any missing features that we might push out the release
>     date for?
>   

Personally, I don't think so.  I think openbios was the biggest issue 
because we don't have the code for the current firmware.  It looks like 
that's been almost resolved.  I'm more interested in getting a release 
out in a timely manner than holding up for any particular feature.

If we have lots of features going in, I'd rather do more frequent 
releases than hold up releases.

>   - Post-release, is there any interest in maintaining a stable branch 
>     until the next release?
>   

I am tempted to try it out.  Let's see how it goes.

>   - The plan for the next release is roughly 6 months, yes?
>   

Yup.

Regards,

Anthony Liguori

> Thanks,
> Mark.
>
>
>
>   

Re: [Qemu-devel] Cutting a new QEMU release

[Rob Landley]

On Monday 09 February 2009 06:43:34 Mark McLoughlin wrote:
> On Tue, 2009-02-03 at 14:48 -0600, Anthony Liguori wrote:
> > What do people think?  TCG seems to be in a good place.  We've got
> > virtio, KVM, live migration, tons of new devices, bsd-user, etc.
> >
> > We could decide to cut one by the end of the month.  I'm already doing
> > some test work in QEMU so I can follow up with some more detailed notes
> > about what is working and what isn't working.  That gives us some time
> > to decide if there's anything we need to fix before a release.
>
> Sounds great to me.
>
> From a Fedora perspective, qemu-0.9.1 is a year old and upstream has
> moved on a lot. As a package maintainer, it's hard to justify caring too
> much about bugs reported against 0.9.1, since the bug is likely to have
> very little relevance to the latest upstream.
>
> Also, it would be really nice to have a kvm-userspace based off a solid
> qemu release ... qemu moving so fast is great, but it means it's hard to
> predict the stability of a given kvm-userspace release.

I'd like to point out a relevant Google tech talk video:

http://video.google.com/videoplay?docid=-5503858974016723264

April 19, 2007 Release Management in Large Free Software Projects - Martin 
Michlmayr (Debian)

ABSTRACT: Time based releases are made according to a specific time interval, 
instead of making a release when a particular functionality or set of features 
have been implemented. This talk argues that time based release management 
acts as an effective coordination mechanism in large volunteer projects and 
shows examples from seven projects that have moved to time based releases: 
Debian, GCC, GNOME, Linux, OpenOffice, Plone, and X.org.

> Some questions:
>
>   - Will there be a period before the release when only bug fixes are
>     merged?
>
>   - Will there be a release candidate?

Those two answer each other.  If your 0.9.2 release turns out to have bugs, 
you can trivially cut a bugfix-only 0.9.2.1, 0.9.2.2, 0.9.2.3... as needed.  
Weekly even.  So 0.9.2 being bug-free isn't that important.

And it's actually just about impossible for you .0 to be bug-free, because you 
get 20 times as many testers for an actual release as you get for any 
snapshot, so they _will_ find new bugs.  It's just about guaranteed.  Also 
unless your stabilization period is a hard freeze preventing new development 
from going into the repository, then you'll be introducing new bugs while you 
try to fix 'em...

>   - Post-release, is there any interest in maintaining a stable branch
>     until the next release?

That's kind of necessary for the previous two, but as long as it's clearly 
bugfix-only then it should have zero impact on new development, and can be 
done in a completely separate repository by a different maintainer.  (That's 
how the linux kernel does things.)

>   - Is there any missing features that we might push out the release
>     date for?

Defeats the purpose of time based releases: it's ok to bump things from this 
release if the next release is a finite amount of time away.  If you have no 
idea when the next release will be, then getting every last feature into this 
release (and holding up the release for it) is a big deal, and thus you have 
endless delays, feature creep, a rush to merge things that aren't quite ready 
when a release is floated...

>   - The plan for the next release is roughly 6 months, yes?

The general theory of having regular scheduled releases is that bumping stuff 
until the next release is no longer the end of the world, because there _will_ 
be a next release, and this has lots and lots of positive side effects, as 
described in the video.

> Thanks,
> Mark.

Rob

Re: [Qemu-devel] Cutting a new QEMU release

[M. Warner Losh]

Re Time based releases:

You need to have someone drive the time based releases long term,
otherwise you'll slide back into the feature based release mode.  And
since there's always another feature, that, as has been pointed out,
tends to stretch out things a very long time.

Warner

Re: [Qemu-devel] Cutting a new QEMU release

[Riku Voipio]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
> We could decide to cut one by the end of the month.

This would indeed be really cool.

> .. to decide if there's anything we need to fix before a release.

At least the OS X (cocoa) host is broken, which is IMHO pretty
bad regression. Apart from that I'm not aware of any major regression
(wearing arm-linux-user, some arm-softmmu and debian hats).

-- 
"rm -rf" only sounds scary if you don't have backups

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] Cutting a new QEMU release

[Stefano Stabellini]

Riku Voipio wrote:

>> .. to decide if there's anything we need to fix before a release.
> 
> At least the OS X (cocoa) host is broken, which is IMHO pretty
> bad regression. Apart from that I'm not aware of any major regression
> (wearing arm-linux-user, some arm-softmmu and debian hats).
> 



There was a patch laying around few weeks ago that was OK for most cases
but didn't work in some.
If we don't have anything better I think we could accept the patch and
add a few #ifdef in vga.c to make sure the patch does not break.

Re: [Qemu-devel] Cutting a new QEMU release

[Jamie Lokier]

Riku Voipio wrote:
> On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
> > We could decide to cut one by the end of the month.
> 
> This would indeed be really cool.
> 
> > .. to decide if there's anything we need to fix before a release.
> 
> At least the OS X (cocoa) host is broken, which is IMHO pretty
> bad regression. Apart from that I'm not aware of any major regression
> (wearing arm-linux-user, some arm-softmmu and debian hats).

I'd say the two qcow2 data corruption bugs are a major regression.
(Both reported in in another thread).

qemu 0.9.1 has the qcow2 code from kvm-72, which doesn't exhibit
either of those corruption bugs.  A new release based on current kvm
userspace would introduce those bugs.  One of the bugs (reported by
Marc) corrupts a qcow2 image so you can't use it even if you revert to
an older qemu/kvm.  It's not clear if the other bug causes permanent
corruption itself, but anything which causes a guest to see the wrong
data can lead to the guest writing corrupt data elsewhere later on.

Simply reverting the qcow2 code appears to fix those problems, so it
needn't hold up cutting a release.  That's what I recommend.

-- Jamie

Re: [Qemu-devel] Cutting a new QEMU release

[Anthony Liguori]

Jamie Lokier wrote:
> Riku Voipio wrote:
>   
>> On Tue, Feb 03, 2009 at 02:48:22PM -0600, Anthony Liguori wrote:
>>     
>>> We could decide to cut one by the end of the month.
>>>       
>> This would indeed be really cool.
>>
>>     
>>> .. to decide if there's anything we need to fix before a release.
>>>       
>> At least the OS X (cocoa) host is broken, which is IMHO pretty
>> bad regression. Apart from that I'm not aware of any major regression
>> (wearing arm-linux-user, some arm-softmmu and debian hats).
>>     
>
> I'd say the two qcow2 data corruption bugs are a major regression.
> (Both reported in in another thread).
>
> qemu 0.9.1 has the qcow2 code from kvm-72, which doesn't exhibit
> either of those corruption bugs.  A new release based on current kvm
> userspace would introduce those bugs.  One of the bugs (reported by
> Marc) corrupts a qcow2 image so you can't use it even if you revert to
> an older qemu/kvm.  It's not clear if the other bug causes permanent
> corruption itself, but anything which causes a guest to see the wrong
> data can lead to the guest writing corrupt data elsewhere later on.
>
> Simply reverting the qcow2 code appears to fix those problems, so it
> needn't hold up cutting a release.  That's what I recommend.
>   

Send some patches.

Regards,

Anthony Liguori

> -- Jamie
>
>
>   
 

[Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Jamie Lokier]

Anthony Liguori wrote:
> >Simply reverting the qcow2 code appears to fix those problems, so it
> >needn't hold up cutting a release.  That's what I recommend.
> 
> Send some patches.

I did already.

Here it is again.  This should fix my bug and Marc's bug according to
his report that reverting qcow2.c fixes it.

-- Jamie


Subject: Revert block-qcow2.c to kvm-72 version due to corruption reports

This fixes two kinds of qcow2 corruption observed in kvm-83 (actually
kvm-73 and later), from three bug reports.


Bug 1: Windows 2000 guests complain of corrupt registry.

Many Windows 2000 guests which boot and runs fine in kvm-72, fail with
a blue-screen indicating file corruption errors in kvm-73 through to
kvm-83 (the latest), and succeed if we replace block-qcow2.c with the
version from kvm-72.

The blue screen appears towards the end of the boot sequence, and
shows only briefly before rebooting.  It says:

    STOP: c0000218 (Registry File Failure)
    The registry cannot load the hive (file):
    \SystemRoot\System32\Config\SOFTWARE
    or its log or alternate.
    It is corrupt, absent, or not writable.

    Beginning dump of physical memory
    Physical memory dump complete. Contact your system administrator or
    technical support [...?]

This is narrowed down to the difference in block-qcow2.c between
kvm-72 and kvm-73 (not -83).  From kvm-73 to kvm-83, there have been
more changes block-qcow2.c, but the observed corruption still occurs.

The bug isn't evident when only reading.  When using "qemu-img
convert" to convert a qcow2 file to a raw file, with broken and fixed
versions of block-qcow2.c it produces the same raw file.  Also, when
using "-snapshot" with qemu, the blue screen doesn't occur.

This bug was observed by Jamie Lokier <jamie@shareable.org> and 
confirmed for multiple Windows 2000 guests by
Marc Bevand <m.bevand@gmail.com>.


Bug 2: Windows 2003 guests complain of corrupt registry.

According to
http://sourceforge.net/tracker/?func=detail&atid=893831&aid=2001452&group_id=180599

Windows 2003 32-bit guests randomly spew disk corruption messages
like this:

    Windows – Registry Hive Recovered
    Registry hive (file): SOFTWARE was corrupted and it has
    been recovered. Some data might have been lost.

and

    The system cannot log on due to the following error:
    Unable to complete the requested operation because of
    either a catastrophic media failure or a data structure
    corruption on the disk.

This bug was reported by <gerdwachs@users.sourceforge.net> and
confirmed by Marc Bevand, noting:

    kvm-73+ also causes some of my Windows 2003 guests to exhibit this
    exact registry corruption error.  [...]  This bug is also fixed by
    reverting block-qcow2.c to the version from kvm-72.

Worryingly, gerdwachs' bug report says it's for kvm-70, implying this
patch may not fix all the Windows 2003 guest corruption problems.

At least Marc says his observed problem goes away with kvm-72's qcow2.


Bug 3: Corruption of qcow2 index rendering the file unusable.

Marc Bevand writes:

    I tested kvm-81 and kvm-83 as well (can't test kvm-80 or older because
    of the qcow2 performance regression caused by the default writethrough
    caching policy) but it randomly triggers an even worse bug: the moment
    I shut down a guest by typing "quit" in the monitor, it sometimes
    overwrite the first 4kB of the disk image with mostly NUL bytes (!)
    which completely destroys it. I am familiar with the qcow2 format and
    apparently this 4kB block seems to be an L2 table with most entries
    set to zero. I have had to restore at least 6 or 7 disk images from
    backup after occurences of that bug. My intuition tells me this may be
    the qcow2 code trying to allocate a cluster to write a new L2 table,
    but not noticing the allocation failed (represented by a 0 offset),
    and writing the L2 table at that 0 offset, overwriting the qcow2
    header.

    Fortunately this bug is also fixed by running kvm-75 with
    block-qcow2.c reverted to its kvm-72 version.

    Basically qcow2 in kvm-73 or newer is completely unreliable.


Reverting block-qcow2.c to the version in kvm-72 appears to fix the
corruption symptoms reported by Marc and Jamie, although gerdwachs'
related bug is against kvm-70 so it may not fix that.

Unfortunately this reverts some optimisations, but fixing corruption
is more important until the new code is reliable.

This patch reverts block-qcow2.c in kvm-83 to the version in kvm-72,
except the "cache=writeback" default performance tweak is retained and
there's no need to define "offsetof".

Signed-Off-By: Jamie Lokier <jamie@shareable.org>


--- kvm-83-real/qemu/block-qcow2.c	2009-01-13 13:29:42.000000000 +0000
+++ kvm-83/qemu/block-qcow2.c	2009-02-13 18:51:12.000000000 +0000
@@ -52,8 +52,6 @@
 #define QCOW_CRYPT_NONE 0
 #define QCOW_CRYPT_AES  1
 
-#define QCOW_MAX_CRYPT_CLUSTERS 32
-
 /* indicate that the refcount of the referenced cluster is exactly one. */
 #define QCOW_OFLAG_COPIED     (1LL << 63)
 /* indicate that the cluster is compressed (they never have the copied flag) */
@@ -269,8 +267,7 @@
     if (!s->cluster_cache)
         goto fail;
     /* one more sector for decompressed data alignment */
-    s->cluster_data = qemu_malloc(QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
-                                  + 512);
+    s->cluster_data = qemu_malloc(s->cluster_size + 512);
     if (!s->cluster_data)
         goto fail;
     s->cluster_cache_offset = -1;
@@ -437,7 +434,8 @@
     int new_l1_size, new_l1_size2, ret, i;
     uint64_t *new_l1_table;
     uint64_t new_l1_table_offset;
-    uint8_t data[12];
+    uint64_t data64;
+    uint32_t data32;
 
     new_l1_size = s->l1_size;
     if (min_size <= new_l1_size)
@@ -467,10 +465,13 @@
         new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
 
     /* set new table */
-    cpu_to_be32w((uint32_t*)data, new_l1_size);
-    cpu_to_be64w((uint64_t*)(data + 4), new_l1_table_offset);
-    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,
-                sizeof(data)) != sizeof(data))
+    data64 = cpu_to_be64(new_l1_table_offset);
+    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_table_offset),
+                    &data64, sizeof(data64)) != sizeof(data64))
+        goto fail;
+    data32 = cpu_to_be32(new_l1_size);
+    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size),
+                    &data32, sizeof(data32)) != sizeof(data32))
         goto fail;
     qemu_free(s->l1_table);
     free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t));
@@ -483,549 +484,169 @@
     return -EIO;
 }
 
-/*
- * seek_l2_table
+/* 'allocate' is:
  *
- * seek l2_offset in the l2_cache table
- * if not found, return NULL,
- * if found,
- *   increments the l2 cache hit count of the entry,
- *   if counter overflow, divide by two all counters
- *   return the pointer to the l2 cache entry
+ * 0 not to allocate.
  *
- */
-
-static uint64_t *seek_l2_table(BDRVQcowState *s, uint64_t l2_offset)
-{
-    int i, j;
-
-    for(i = 0; i < L2_CACHE_SIZE; i++) {
-        if (l2_offset == s->l2_cache_offsets[i]) {
-            /* increment the hit count */
-            if (++s->l2_cache_counts[i] == 0xffffffff) {
-                for(j = 0; j < L2_CACHE_SIZE; j++) {
-                    s->l2_cache_counts[j] >>= 1;
-                }
-            }
-            return s->l2_cache + (i << s->l2_bits);
-        }
-    }
-    return NULL;
-}
-
-/*
- * l2_load
+ * 1 to allocate a normal cluster (for sector indexes 'n_start' to
+ * 'n_end')
  *
- * Loads a L2 table into memory. If the table is in the cache, the cache
- * is used; otherwise the L2 table is loaded from the image file.
+ * 2 to allocate a compressed cluster of size
+ * 'compressed_size'. 'compressed_size' must be > 0 and <
+ * cluster_size
  *
- * Returns a pointer to the L2 table on success, or NULL if the read from
- * the image file failed.
+ * return 0 if not allocated.
  */
-
-static uint64_t *l2_load(BlockDriverState *bs, uint64_t l2_offset)
-{
-    BDRVQcowState *s = bs->opaque;
-    int min_index;
-    uint64_t *l2_table;
-
-    /* seek if the table for the given offset is in the cache */
-
-    l2_table = seek_l2_table(s, l2_offset);
-    if (l2_table != NULL)
-        return l2_table;
-
-    /* not found: load a new entry in the least used one */
-
-    min_index = l2_cache_new_entry(bs);
-    l2_table = s->l2_cache + (min_index << s->l2_bits);
-    if (bdrv_pread(s->hd, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
-        s->l2_size * sizeof(uint64_t))
-        return NULL;
-    s->l2_cache_offsets[min_index] = l2_offset;
-    s->l2_cache_counts[min_index] = 1;
-
-    return l2_table;
-}
-
-/*
- * l2_allocate
- *
- * Allocate a new l2 entry in the file. If l1_index points to an already
- * used entry in the L2 table (i.e. we are doing a copy on write for the L2
- * table) copy the contents of the old L2 table into the newly allocated one.
- * Otherwise the new table is initialized with zeros.
- *
- */
-
-static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
-{
-    BDRVQcowState *s = bs->opaque;
-    int min_index;
-    uint64_t old_l2_offset, tmp;
-    uint64_t *l2_table, l2_offset;
-
-    old_l2_offset = s->l1_table[l1_index];
-
-    /* allocate a new l2 entry */
-
-    l2_offset = alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
-
-    /* update the L1 entry */
-
-    s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
-
-    tmp = cpu_to_be64(l2_offset | QCOW_OFLAG_COPIED);
-    if (bdrv_pwrite(s->hd, s->l1_table_offset + l1_index * sizeof(tmp),
-                    &tmp, sizeof(tmp)) != sizeof(tmp))
-        return NULL;
-
-    /* allocate a new entry in the l2 cache */
-
-    min_index = l2_cache_new_entry(bs);
-    l2_table = s->l2_cache + (min_index << s->l2_bits);
-
-    if (old_l2_offset == 0) {
-        /* if there was no old l2 table, clear the new table */
-        memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
-    } else {
-        /* if there was an old l2 table, read it from the disk */
-        if (bdrv_pread(s->hd, old_l2_offset,
-                       l2_table, s->l2_size * sizeof(uint64_t)) !=
-            s->l2_size * sizeof(uint64_t))
-            return NULL;
-    }
-    /* write the l2 table to the file */
-    if (bdrv_pwrite(s->hd, l2_offset,
-                    l2_table, s->l2_size * sizeof(uint64_t)) !=
-        s->l2_size * sizeof(uint64_t))
-        return NULL;
-
-    /* update the l2 cache entry */
-
-    s->l2_cache_offsets[min_index] = l2_offset;
-    s->l2_cache_counts[min_index] = 1;
-
-    return l2_table;
-}
-
-static int size_to_clusters(BDRVQcowState *s, int64_t size)
-{
-    return (size + (s->cluster_size - 1)) >> s->cluster_bits;
-}
-
-static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
-        uint64_t *l2_table, uint64_t start, uint64_t mask)
-{
-    int i;
-    uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
-
-    if (!offset)
-        return 0;
-
-    for (i = start; i < start + nb_clusters; i++)
-        if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) & ~mask))
-            break;
-
-	return (i - start);
-}
-
-static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t *l2_table)
-{
-    int i = 0;
-
-    while(nb_clusters-- && l2_table[i] == 0)
-        i++;
-
-    return i;
-}
-
-/*
- * get_cluster_offset
- *
- * For a given offset of the disk image, return cluster offset in
- * qcow2 file.
- *
- * on entry, *num is the number of contiguous clusters we'd like to
- * access following offset.
- *
- * on exit, *num is the number of contiguous clusters we can read.
- *
- * Return 1, if the offset is found
- * Return 0, otherwise.
- *
- */
-
 static uint64_t get_cluster_offset(BlockDriverState *bs,
-                                   uint64_t offset, int *num)
-{
-    BDRVQcowState *s = bs->opaque;
-    int l1_index, l2_index;
-    uint64_t l2_offset, *l2_table, cluster_offset;
-    int l1_bits, c;
-    int index_in_cluster, nb_available, nb_needed, nb_clusters;
-
-    index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
-    nb_needed = *num + index_in_cluster;
-
-    l1_bits = s->l2_bits + s->cluster_bits;
-
-    /* compute how many bytes there are between the offset and
-     * the end of the l1 entry
-     */
-
-    nb_available = (1 << l1_bits) - (offset & ((1 << l1_bits) - 1));
-
-    /* compute the number of available sectors */
-
-    nb_available = (nb_available >> 9) + index_in_cluster;
-
-    cluster_offset = 0;
-
-    /* seek the the l2 offset in the l1 table */
-
-    l1_index = offset >> l1_bits;
-    if (l1_index >= s->l1_size)
-        goto out;
-
-    l2_offset = s->l1_table[l1_index];
-
-    /* seek the l2 table of the given l2 offset */
-
-    if (!l2_offset)
-        goto out;
-
-    /* load the l2 table in memory */
-
-    l2_offset &= ~QCOW_OFLAG_COPIED;
-    l2_table = l2_load(bs, l2_offset);
-    if (l2_table == NULL)
-        return 0;
-
-    /* find the cluster offset for the given disk offset */
-
-    l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
-    cluster_offset = be64_to_cpu(l2_table[l2_index]);
-    nb_clusters = size_to_clusters(s, nb_needed << 9);
-
-    if (!cluster_offset) {
-        /* how many empty clusters ? */
-        c = count_contiguous_free_clusters(nb_clusters, &l2_table[l2_index]);
-    } else {
-        /* how many allocated clusters ? */
-        c = count_contiguous_clusters(nb_clusters, s->cluster_size,
-                &l2_table[l2_index], 0, QCOW_OFLAG_COPIED);
-    }
-
-   nb_available = (c * s->cluster_sectors);
-out:
-    if (nb_available > nb_needed)
-        nb_available = nb_needed;
-
-    *num = nb_available - index_in_cluster;
-
-    return cluster_offset & ~QCOW_OFLAG_COPIED;
-}
-
-/*
- * free_any_clusters
- *
- * free clusters according to its type: compressed or not
- *
- */
-
-static void free_any_clusters(BlockDriverState *bs,
-                              uint64_t cluster_offset, int nb_clusters)
-{
-    BDRVQcowState *s = bs->opaque;
-
-    /* free the cluster */
-
-    if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
-        int nb_csectors;
-        nb_csectors = ((cluster_offset >> s->csize_shift) &
-                       s->csize_mask) + 1;
-        free_clusters(bs, (cluster_offset & s->cluster_offset_mask) & ~511,
-                      nb_csectors * 512);
-        return;
-    }
-
-    free_clusters(bs, cluster_offset, nb_clusters << s->cluster_bits);
-
-    return;
-}
-
-/*
- * get_cluster_table
- *
- * for a given disk offset, load (and allocate if needed)
- * the l2 table.
- *
- * the l2 table offset in the qcow2 file and the cluster index
- * in the l2 table are given to the caller.
- *
- */
-
-static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
-                             uint64_t **new_l2_table,
-                             uint64_t *new_l2_offset,
-                             int *new_l2_index)
+                                   uint64_t offset, int allocate,
+                                   int compressed_size,
+                                   int n_start, int n_end)
 {
     BDRVQcowState *s = bs->opaque;
-    int l1_index, l2_index, ret;
-    uint64_t l2_offset, *l2_table;
-
-    /* seek the the l2 offset in the l1 table */
+    int min_index, i, j, l1_index, l2_index, ret;
+    uint64_t l2_offset, *l2_table, cluster_offset, tmp, old_l2_offset;
 
     l1_index = offset >> (s->l2_bits + s->cluster_bits);
     if (l1_index >= s->l1_size) {
-        ret = grow_l1_table(bs, l1_index + 1);
-        if (ret < 0)
+        /* outside l1 table is allowed: we grow the table if needed */
+        if (!allocate)
+            return 0;
+        if (grow_l1_table(bs, l1_index + 1) < 0)
             return 0;
     }
     l2_offset = s->l1_table[l1_index];
+    if (!l2_offset) {
+        if (!allocate)
+            return 0;
+    l2_allocate:
+        old_l2_offset = l2_offset;
+        /* allocate a new l2 entry */
+        l2_offset = alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
+        /* update the L1 entry */
+        s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
+        tmp = cpu_to_be64(l2_offset | QCOW_OFLAG_COPIED);
+        if (bdrv_pwrite(s->hd, s->l1_table_offset + l1_index * sizeof(tmp),
+                        &tmp, sizeof(tmp)) != sizeof(tmp))
+            return 0;
+        min_index = l2_cache_new_entry(bs);
+        l2_table = s->l2_cache + (min_index << s->l2_bits);
 
-    /* seek the l2 table of the given l2 offset */
-
-    if (l2_offset & QCOW_OFLAG_COPIED) {
-        /* load the l2 table in memory */
-        l2_offset &= ~QCOW_OFLAG_COPIED;
-        l2_table = l2_load(bs, l2_offset);
-        if (l2_table == NULL)
+        if (old_l2_offset == 0) {
+            memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
+        } else {
+            if (bdrv_pread(s->hd, old_l2_offset,
+                           l2_table, s->l2_size * sizeof(uint64_t)) !=
+                s->l2_size * sizeof(uint64_t))
+                return 0;
+        }
+        if (bdrv_pwrite(s->hd, l2_offset,
+                        l2_table, s->l2_size * sizeof(uint64_t)) !=
+            s->l2_size * sizeof(uint64_t))
             return 0;
     } else {
-        if (l2_offset)
-            free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
-        l2_table = l2_allocate(bs, l1_index);
-        if (l2_table == NULL)
+        if (!(l2_offset & QCOW_OFLAG_COPIED)) {
+            if (allocate) {
+                free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
+                goto l2_allocate;
+            }
+        } else {
+            l2_offset &= ~QCOW_OFLAG_COPIED;
+        }
+        for(i = 0; i < L2_CACHE_SIZE; i++) {
+            if (l2_offset == s->l2_cache_offsets[i]) {
+                /* increment the hit count */
+                if (++s->l2_cache_counts[i] == 0xffffffff) {
+                    for(j = 0; j < L2_CACHE_SIZE; j++) {
+                        s->l2_cache_counts[j] >>= 1;
+                    }
+                }
+                l2_table = s->l2_cache + (i << s->l2_bits);
+                goto found;
+            }
+        }
+        /* not found: load a new entry in the least used one */
+        min_index = l2_cache_new_entry(bs);
+        l2_table = s->l2_cache + (min_index << s->l2_bits);
+        if (bdrv_pread(s->hd, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
+            s->l2_size * sizeof(uint64_t))
             return 0;
-        l2_offset = s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED;
     }
-
-    /* find the cluster offset for the given disk offset */
-
+    s->l2_cache_offsets[min_index] = l2_offset;
+    s->l2_cache_counts[min_index] = 1;
+ found:
     l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
-
-    *new_l2_table = l2_table;
-    *new_l2_offset = l2_offset;
-    *new_l2_index = l2_index;
-
-    return 1;
-}
-
-/*
- * alloc_compressed_cluster_offset
- *
- * For a given offset of the disk image, return cluster offset in
- * qcow2 file.
- *
- * If the offset is not found, allocate a new compressed cluster.
- *
- * Return the cluster offset if successful,
- * Return 0, otherwise.
- *
- */
-
-static uint64_t alloc_compressed_cluster_offset(BlockDriverState *bs,
-                                                uint64_t offset,
-                                                int compressed_size)
-{
-    BDRVQcowState *s = bs->opaque;
-    int l2_index, ret;
-    uint64_t l2_offset, *l2_table, cluster_offset;
-    int nb_csectors;
-
-    ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
-    if (ret == 0)
-        return 0;
-
     cluster_offset = be64_to_cpu(l2_table[l2_index]);
-    if (cluster_offset & QCOW_OFLAG_COPIED)
-        return cluster_offset & ~QCOW_OFLAG_COPIED;
-
-    if (cluster_offset)
-        free_any_clusters(bs, cluster_offset, 1);
-
-    cluster_offset = alloc_bytes(bs, compressed_size);
-    nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
-                  (cluster_offset >> 9);
-
-    cluster_offset |= QCOW_OFLAG_COMPRESSED |
-                      ((uint64_t)nb_csectors << s->csize_shift);
-
-    /* update L2 table */
-
-    /* compressed clusters never have the copied flag */
-
-    l2_table[l2_index] = cpu_to_be64(cluster_offset);
-    if (bdrv_pwrite(s->hd,
-                    l2_offset + l2_index * sizeof(uint64_t),
-                    l2_table + l2_index,
-                    sizeof(uint64_t)) != sizeof(uint64_t))
-        return 0;
-
-    return cluster_offset;
-}
-
-typedef struct QCowL2Meta
-{
-    uint64_t offset;
-    int n_start;
-    int nb_available;
-    int nb_clusters;
-} QCowL2Meta;
-
-static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
-        QCowL2Meta *m)
-{
-    BDRVQcowState *s = bs->opaque;
-    int i, j = 0, l2_index, ret;
-    uint64_t *old_cluster, start_sect, l2_offset, *l2_table;
-
-    if (m->nb_clusters == 0)
-        return 0;
-
-    if (!(old_cluster = qemu_malloc(m->nb_clusters * sizeof(uint64_t))))
-        return -ENOMEM;
-
-    /* copy content of unmodified sectors */
-    start_sect = (m->offset & ~(s->cluster_size - 1)) >> 9;
-    if (m->n_start) {
-        ret = copy_sectors(bs, start_sect, cluster_offset, 0, m->n_start);
-        if (ret < 0)
-            goto err;
+    if (!cluster_offset) {
+        if (!allocate)
+            return cluster_offset;
+    } else if (!(cluster_offset & QCOW_OFLAG_COPIED)) {
+        if (!allocate)
+            return cluster_offset;
+        /* free the cluster */
+        if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
+            int nb_csectors;
+            nb_csectors = ((cluster_offset >> s->csize_shift) &
+                           s->csize_mask) + 1;
+            free_clusters(bs, (cluster_offset & s->cluster_offset_mask) & ~511,
+                          nb_csectors * 512);
+        } else {
+            free_clusters(bs, cluster_offset, s->cluster_size);
+        }
+    } else {
+        cluster_offset &= ~QCOW_OFLAG_COPIED;
+        return cluster_offset;
     }
-
-    if (m->nb_available & (s->cluster_sectors - 1)) {
-        uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1);
-        ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9),
-                m->nb_available - end, s->cluster_sectors);
-        if (ret < 0)
-            goto err;
+    if (allocate == 1) {
+        /* allocate a new cluster */
+        cluster_offset = alloc_clusters(bs, s->cluster_size);
+
+        /* we must initialize the cluster content which won't be
+           written */
+        if ((n_end - n_start) < s->cluster_sectors) {
+            uint64_t start_sect;
+
+            start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
+            ret = copy_sectors(bs, start_sect,
+                               cluster_offset, 0, n_start);
+            if (ret < 0)
+                return 0;
+            ret = copy_sectors(bs, start_sect,
+                               cluster_offset, n_end, s->cluster_sectors);
+            if (ret < 0)
+                return 0;
+        }
+        tmp = cpu_to_be64(cluster_offset | QCOW_OFLAG_COPIED);
+    } else {
+        int nb_csectors;
+        cluster_offset = alloc_bytes(bs, compressed_size);
+        nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
+            (cluster_offset >> 9);
+        cluster_offset |= QCOW_OFLAG_COMPRESSED |
+            ((uint64_t)nb_csectors << s->csize_shift);
+        /* compressed clusters never have the copied flag */
+        tmp = cpu_to_be64(cluster_offset);
     }
-
-    ret = -EIO;
     /* update L2 table */
-    if (!get_cluster_table(bs, m->offset, &l2_table, &l2_offset, &l2_index))
-        goto err;
-
-    for (i = 0; i < m->nb_clusters; i++) {
-        if(l2_table[l2_index + i] != 0)
-            old_cluster[j++] = l2_table[l2_index + i];
-
-        l2_table[l2_index + i] = cpu_to_be64((cluster_offset +
-                    (i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
-     }
-
-    if (bdrv_pwrite(s->hd, l2_offset + l2_index * sizeof(uint64_t),
-                l2_table + l2_index, m->nb_clusters * sizeof(uint64_t)) !=
-            m->nb_clusters * sizeof(uint64_t))
-        goto err;
-
-    for (i = 0; i < j; i++)
-        free_any_clusters(bs, old_cluster[i], 1);
-
-    ret = 0;
-err:
-    qemu_free(old_cluster);
-    return ret;
- }
-
-/*
- * alloc_cluster_offset
- *
- * For a given offset of the disk image, return cluster offset in
- * qcow2 file.
- *
- * If the offset is not found, allocate a new cluster.
- *
- * Return the cluster offset if successful,
- * Return 0, otherwise.
- *
- */
-
-static uint64_t alloc_cluster_offset(BlockDriverState *bs,
-                                     uint64_t offset,
-                                     int n_start, int n_end,
-                                     int *num, QCowL2Meta *m)
-{
-    BDRVQcowState *s = bs->opaque;
-    int l2_index, ret;
-    uint64_t l2_offset, *l2_table, cluster_offset;
-    int nb_clusters, i = 0;
-
-    ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
-    if (ret == 0)
+    l2_table[l2_index] = tmp;
+    if (bdrv_pwrite(s->hd,
+                    l2_offset + l2_index * sizeof(tmp), &tmp, sizeof(tmp)) != sizeof(tmp))
         return 0;
-
-    nb_clusters = size_to_clusters(s, n_end << 9);
-
-    nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
-
-    cluster_offset = be64_to_cpu(l2_table[l2_index]);
-
-    /* We keep all QCOW_OFLAG_COPIED clusters */
-
-    if (cluster_offset & QCOW_OFLAG_COPIED) {
-        nb_clusters = count_contiguous_clusters(nb_clusters, s->cluster_size,
-                &l2_table[l2_index], 0, 0);
-
-        cluster_offset &= ~QCOW_OFLAG_COPIED;
-        m->nb_clusters = 0;
-
-        goto out;
-    }
-
-    /* for the moment, multiple compressed clusters are not managed */
-
-    if (cluster_offset & QCOW_OFLAG_COMPRESSED)
-        nb_clusters = 1;
-
-    /* how many available clusters ? */
-
-    while (i < nb_clusters) {
-        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
-                &l2_table[l2_index], i, 0);
-
-        if(be64_to_cpu(l2_table[l2_index + i]))
-            break;
-
-        i += count_contiguous_free_clusters(nb_clusters - i,
-                &l2_table[l2_index + i]);
-
-        cluster_offset = be64_to_cpu(l2_table[l2_index + i]);
-
-        if ((cluster_offset & QCOW_OFLAG_COPIED) ||
-                (cluster_offset & QCOW_OFLAG_COMPRESSED))
-            break;
-    }
-    nb_clusters = i;
-
-    /* allocate a new cluster */
-
-    cluster_offset = alloc_clusters(bs, nb_clusters * s->cluster_size);
-
-    /* save info needed for meta data update */
-    m->offset = offset;
-    m->n_start = n_start;
-    m->nb_clusters = nb_clusters;
-
-out:
-    m->nb_available = MIN(nb_clusters << (s->cluster_bits - 9), n_end);
-
-    *num = m->nb_available - n_start;
-
     return cluster_offset;
 }
 
 static int qcow_is_allocated(BlockDriverState *bs, int64_t sector_num,
                              int nb_sectors, int *pnum)
 {
+    BDRVQcowState *s = bs->opaque;
+    int index_in_cluster, n;
     uint64_t cluster_offset;
 
-    *pnum = nb_sectors;
-    cluster_offset = get_cluster_offset(bs, sector_num << 9, pnum);
-
+    cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
+    index_in_cluster = sector_num & (s->cluster_sectors - 1);
+    n = s->cluster_sectors - index_in_cluster;
+    if (n > nb_sectors)
+        n = nb_sectors;
+    *pnum = n;
     return (cluster_offset != 0);
 }
 
@@ -1102,9 +723,11 @@
     uint64_t cluster_offset;
 
     while (nb_sectors > 0) {
-        n = nb_sectors;
-        cluster_offset = get_cluster_offset(bs, sector_num << 9, &n);
+        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
+        n = s->cluster_sectors - index_in_cluster;
+        if (n > nb_sectors)
+            n = nb_sectors;
         if (!cluster_offset) {
             if (bs->backing_hd) {
                 /* read from the base image */
@@ -1143,18 +766,15 @@
     BDRVQcowState *s = bs->opaque;
     int ret, index_in_cluster, n;
     uint64_t cluster_offset;
-    int n_end;
-    QCowL2Meta l2meta;
 
     while (nb_sectors > 0) {
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
-        n_end = index_in_cluster + nb_sectors;
-        if (s->crypt_method &&
-            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
-            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
-        cluster_offset = alloc_cluster_offset(bs, sector_num << 9,
-                                              index_in_cluster,
-                                              n_end, &n, &l2meta);
+        n = s->cluster_sectors - index_in_cluster;
+        if (n > nb_sectors)
+            n = nb_sectors;
+        cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
+                                            index_in_cluster,
+                                            index_in_cluster + n);
         if (!cluster_offset)
             return -1;
         if (s->crypt_method) {
@@ -1165,10 +785,8 @@
         } else {
             ret = bdrv_pwrite(s->hd, cluster_offset + index_in_cluster * 512, buf, n * 512);
         }
-        if (ret != n * 512 || alloc_cluster_link_l2(bs, cluster_offset, &l2meta) < 0) {
-            free_any_clusters(bs, cluster_offset, l2meta.nb_clusters);
+        if (ret != n * 512)
             return -1;
-        }
         nb_sectors -= n;
         sector_num += n;
         buf += n * 512;
@@ -1186,33 +804,8 @@
     uint64_t cluster_offset;
     uint8_t *cluster_data;
     BlockDriverAIOCB *hd_aiocb;
-    QEMUBH *bh;
-    QCowL2Meta l2meta;
 } QCowAIOCB;
 
-static void qcow_aio_read_cb(void *opaque, int ret);
-static void qcow_aio_read_bh(void *opaque)
-{
-    QCowAIOCB *acb = opaque;
-    qemu_bh_delete(acb->bh);
-    acb->bh = NULL;
-    qcow_aio_read_cb(opaque, 0);
-}
-
-static int qcow_schedule_bh(QEMUBHFunc *cb, QCowAIOCB *acb)
-{
-    if (acb->bh)
-        return -EIO;
-
-    acb->bh = qemu_bh_new(cb, acb);
-    if (!acb->bh)
-        return -EIO;
-
-    qemu_bh_schedule(acb->bh);
-
-    return 0;
-}
-
 static void qcow_aio_read_cb(void *opaque, int ret)
 {
     QCowAIOCB *acb = opaque;
@@ -1222,12 +815,13 @@
 
     acb->hd_aiocb = NULL;
     if (ret < 0) {
-fail:
+    fail:
         acb->common.cb(acb->common.opaque, ret);
         qemu_aio_release(acb);
         return;
     }
 
+ redo:
     /* post process the read buffer */
     if (!acb->cluster_offset) {
         /* nothing to do */
@@ -1253,9 +847,12 @@
     }
 
     /* prepare next AIO request */
-    acb->n = acb->nb_sectors;
-    acb->cluster_offset = get_cluster_offset(bs, acb->sector_num << 9, &acb->n);
+    acb->cluster_offset = get_cluster_offset(bs, acb->sector_num << 9,
+                                             0, 0, 0, 0);
     index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
+    acb->n = s->cluster_sectors - index_in_cluster;
+    if (acb->n > acb->nb_sectors)
+        acb->n = acb->nb_sectors;
 
     if (!acb->cluster_offset) {
         if (bs->backing_hd) {
@@ -1268,16 +865,12 @@
                 if (acb->hd_aiocb == NULL)
                     goto fail;
             } else {
-                ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
-                if (ret < 0)
-                    goto fail;
+                goto redo;
             }
         } else {
             /* Note: in this case, no need to wait */
             memset(acb->buf, 0, 512 * acb->n);
-            ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
-            if (ret < 0)
-                goto fail;
+            goto redo;
         }
     } else if (acb->cluster_offset & QCOW_OFLAG_COMPRESSED) {
         /* add AIO support for compressed blocks ? */
@@ -1285,9 +878,7 @@
             goto fail;
         memcpy(acb->buf,
                s->cluster_cache + index_in_cluster * 512, 512 * acb->n);
-        ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
-        if (ret < 0)
-            goto fail;
+        goto redo;
     } else {
         if ((acb->cluster_offset & 511) != 0) {
             ret = -EIO;
@@ -1316,7 +907,6 @@
     acb->nb_sectors = nb_sectors;
     acb->n = 0;
     acb->cluster_offset = 0;
-    acb->l2meta.nb_clusters = 0;
     return acb;
 }
 
@@ -1340,8 +930,8 @@
     BlockDriverState *bs = acb->common.bs;
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
+    uint64_t cluster_offset;
     const uint8_t *src_buf;
-    int n_end;
 
     acb->hd_aiocb = NULL;
 
@@ -1352,11 +942,6 @@
         return;
     }
 
-    if (alloc_cluster_link_l2(bs, acb->cluster_offset, &acb->l2meta) < 0) {
-        free_any_clusters(bs, acb->cluster_offset, acb->l2meta.nb_clusters);
-        goto fail;
-    }
-
     acb->nb_sectors -= acb->n;
     acb->sector_num += acb->n;
     acb->buf += acb->n * 512;
@@ -1369,22 +954,19 @@
     }
 
     index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
-    n_end = index_in_cluster + acb->nb_sectors;
-    if (s->crypt_method &&
-        n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
-        n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
-
-    acb->cluster_offset = alloc_cluster_offset(bs, acb->sector_num << 9,
-                                          index_in_cluster,
-                                          n_end, &acb->n, &acb->l2meta);
-    if (!acb->cluster_offset || (acb->cluster_offset & 511) != 0) {
+    acb->n = s->cluster_sectors - index_in_cluster;
+    if (acb->n > acb->nb_sectors)
+        acb->n = acb->nb_sectors;
+    cluster_offset = get_cluster_offset(bs, acb->sector_num << 9, 1, 0,
+                                        index_in_cluster,
+                                        index_in_cluster + acb->n);
+    if (!cluster_offset || (cluster_offset & 511) != 0) {
         ret = -EIO;
         goto fail;
     }
     if (s->crypt_method) {
         if (!acb->cluster_data) {
-            acb->cluster_data = qemu_mallocz(QCOW_MAX_CRYPT_CLUSTERS *
-                                             s->cluster_size);
+            acb->cluster_data = qemu_mallocz(s->cluster_size);
             if (!acb->cluster_data) {
                 ret = -ENOMEM;
                 goto fail;
@@ -1397,7 +979,7 @@
         src_buf = acb->buf;
     }
     acb->hd_aiocb = bdrv_aio_write(s->hd,
-                                   (acb->cluster_offset >> 9) + index_in_cluster,
+                                   (cluster_offset >> 9) + index_in_cluster,
                                    src_buf, acb->n,
                                    qcow_aio_write_cb, acb);
     if (acb->hd_aiocb == NULL)
@@ -1571,7 +1153,7 @@
 
     memset(s->l1_table, 0, l1_length);
     if (bdrv_pwrite(s->hd, s->l1_table_offset, s->l1_table, l1_length) < 0)
-        return -1;
+	return -1;
     ret = bdrv_truncate(s->hd, s->l1_table_offset + l1_length);
     if (ret < 0)
         return ret;
@@ -1637,10 +1219,8 @@
         /* could not compress: write normal cluster */
         qcow_write(bs, sector_num, buf, s->cluster_sectors);
     } else {
-        cluster_offset = alloc_compressed_cluster_offset(bs, sector_num << 9,
-                                              out_len);
-        if (!cluster_offset)
-            return -1;
+        cluster_offset = get_cluster_offset(bs, sector_num << 9, 2,
+                                            out_len, 0, 0);
         cluster_offset &= s->cluster_offset_mask;
         if (bdrv_pwrite(s->hd, cluster_offset, out_buf, out_len) != out_len) {
             qemu_free(out_buf);
@@ -2225,19 +1805,26 @@
     BDRVQcowState *s = bs->opaque;
     int i, nb_clusters;
 
-    nb_clusters = size_to_clusters(s, size);
-retry:
-    for(i = 0; i < nb_clusters; i++) {
-        int64_t i = s->free_cluster_index++;
-        if (get_refcount(bs, i) != 0)
-            goto retry;
-    }
+    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
+    for(;;) {
+        if (get_refcount(bs, s->free_cluster_index) == 0) {
+            s->free_cluster_index++;
+            for(i = 1; i < nb_clusters; i++) {
+                if (get_refcount(bs, s->free_cluster_index) != 0)
+                    goto not_found;
+                s->free_cluster_index++;
+            }
 #ifdef DEBUG_ALLOC2
-    printf("alloc_clusters: size=%lld -> %lld\n",
-            size,
-            (s->free_cluster_index - nb_clusters) << s->cluster_bits);
+            printf("alloc_clusters: size=%lld -> %lld\n",
+                   size,
+                   (s->free_cluster_index - nb_clusters) << s->cluster_bits);
 #endif
-    return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
+            return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
+        } else {
+        not_found:
+            s->free_cluster_index++;
+        }
+    }
 }
 
 static int64_t alloc_clusters(BlockDriverState *bs, int64_t size)
@@ -2301,7 +1888,8 @@
     int new_table_size, new_table_size2, refcount_table_clusters, i, ret;
     uint64_t *new_table;
     int64_t table_offset;
-    uint8_t data[12];
+    uint64_t data64;
+    uint32_t data32;
     int old_table_size;
     int64_t old_table_offset;
 
@@ -2340,10 +1928,13 @@
     for(i = 0; i < s->refcount_table_size; i++)
         be64_to_cpus(&new_table[i]);
 
-    cpu_to_be64w((uint64_t*)data, table_offset);
-    cpu_to_be32w((uint32_t*)(data + 8), refcount_table_clusters);
+    data64 = cpu_to_be64(table_offset);
     if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
-                    data, sizeof(data)) != sizeof(data))
+                    &data64, sizeof(data64)) != sizeof(data64))
+        goto fail;
+    data32 = cpu_to_be32(refcount_table_clusters);
+    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_clusters),
+                    &data32, sizeof(data32)) != sizeof(data32))
         goto fail;
     qemu_free(s->refcount_table);
     old_table_offset = s->refcount_table_offset;
@@ -2572,7 +2163,7 @@
     uint16_t *refcount_table;
 
     size = bdrv_getlength(s->hd);
-    nb_clusters = size_to_clusters(s, size);
+    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
     refcount_table = qemu_mallocz(nb_clusters * sizeof(uint16_t));
 
     /* header */
@@ -2624,7 +2215,7 @@
     int refcount;
 
     size = bdrv_getlength(s->hd);
-    nb_clusters = size_to_clusters(s, size);
+    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
     for(k = 0; k < nb_clusters;) {
         k1 = k;
         refcount = get_refcount(bs, k);

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Dor Laor]

[-- Attachment #1: Type: text/plain, Size: 42670 bytes --]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>>> Simply reverting the qcow2 code appears to fix those problems, so it
>>> needn't hold up cutting a release.  That's what I recommend.
>>>       
>> Send some patches.
>>     
>
> I did already.
>
> Here it is again.  This should fix my bug and Marc's bug according to
> his report that reverting qcow2.c fixes it.
>   
Going back to kvm-72 is not good also.
First, there were qcow2 corruptions before it, they were very rare but 
still exist. Not long
ago we did not know even that qcow2 is the faulty.
In addition, Gleb fixed some qcow2 meta data ordering writes. We need to 
keep them in.
The solution is to find the real cause to the corruption.
> -- Jamie
>
>
> Subject: Revert block-qcow2.c to kvm-72 version due to corruption reports
>
> This fixes two kinds of qcow2 corruption observed in kvm-83 (actually
> kvm-73 and later), from three bug reports.
>
>
> Bug 1: Windows 2000 guests complain of corrupt registry.
>
> Many Windows 2000 guests which boot and runs fine in kvm-72, fail with
> a blue-screen indicating file corruption errors in kvm-73 through to
> kvm-83 (the latest), and succeed if we replace block-qcow2.c with the
> version from kvm-72.
>
> The blue screen appears towards the end of the boot sequence, and
> shows only briefly before rebooting.  It says:
>
>     STOP: c0000218 (Registry File Failure)
>     The registry cannot load the hive (file):
>     \SystemRoot\System32\Config\SOFTWARE
>     or its log or alternate.
>     It is corrupt, absent, or not writable.
>
>     Beginning dump of physical memory
>     Physical memory dump complete. Contact your system administrator or
>     technical support [...?]
>
> This is narrowed down to the difference in block-qcow2.c between
> kvm-72 and kvm-73 (not -83).  From kvm-73 to kvm-83, there have been
> more changes block-qcow2.c, but the observed corruption still occurs.
>
> The bug isn't evident when only reading.  When using "qemu-img
> convert" to convert a qcow2 file to a raw file, with broken and fixed
> versions of block-qcow2.c it produces the same raw file.  Also, when
> using "-snapshot" with qemu, the blue screen doesn't occur.
>
> This bug was observed by Jamie Lokier <jamie@shareable.org> and 
> confirmed for multiple Windows 2000 guests by
> Marc Bevand <m.bevand@gmail.com>.
>
>
> Bug 2: Windows 2003 guests complain of corrupt registry.
>
> According to
> http://sourceforge.net/tracker/?func=detail&atid=893831&aid=2001452&group_id=180599
>
> Windows 2003 32-bit guests randomly spew disk corruption messages
> like this:
>
>     Windows – Registry Hive Recovered
>     Registry hive (file): SOFTWARE was corrupted and it has
>     been recovered. Some data might have been lost.
>
> and
>
>     The system cannot log on due to the following error:
>     Unable to complete the requested operation because of
>     either a catastrophic media failure or a data structure
>     corruption on the disk.
>
> This bug was reported by <gerdwachs@users.sourceforge.net> and
> confirmed by Marc Bevand, noting:
>
>     kvm-73+ also causes some of my Windows 2003 guests to exhibit this
>     exact registry corruption error.  [...]  This bug is also fixed by
>     reverting block-qcow2.c to the version from kvm-72.
>
> Worryingly, gerdwachs' bug report says it's for kvm-70, implying this
> patch may not fix all the Windows 2003 guest corruption problems.
>
> At least Marc says his observed problem goes away with kvm-72's qcow2.
>
>
> Bug 3: Corruption of qcow2 index rendering the file unusable.
>
> Marc Bevand writes:
>
>     I tested kvm-81 and kvm-83 as well (can't test kvm-80 or older because
>     of the qcow2 performance regression caused by the default writethrough
>     caching policy) but it randomly triggers an even worse bug: the moment
>     I shut down a guest by typing "quit" in the monitor, it sometimes
>     overwrite the first 4kB of the disk image with mostly NUL bytes (!)
>     which completely destroys it. I am familiar with the qcow2 format and
>     apparently this 4kB block seems to be an L2 table with most entries
>     set to zero. I have had to restore at least 6 or 7 disk images from
>     backup after occurences of that bug. My intuition tells me this may be
>     the qcow2 code trying to allocate a cluster to write a new L2 table,
>     but not noticing the allocation failed (represented by a 0 offset),
>     and writing the L2 table at that 0 offset, overwriting the qcow2
>     header.
>
>     Fortunately this bug is also fixed by running kvm-75 with
>     block-qcow2.c reverted to its kvm-72 version.
>
>     Basically qcow2 in kvm-73 or newer is completely unreliable.
>
>
> Reverting block-qcow2.c to the version in kvm-72 appears to fix the
> corruption symptoms reported by Marc and Jamie, although gerdwachs'
> related bug is against kvm-70 so it may not fix that.
>
> Unfortunately this reverts some optimisations, but fixing corruption
> is more important until the new code is reliable.
>
> This patch reverts block-qcow2.c in kvm-83 to the version in kvm-72,
> except the "cache=writeback" default performance tweak is retained and
> there's no need to define "offsetof".
>
> Signed-Off-By: Jamie Lokier <jamie@shareable.org>
>
>
> --- kvm-83-real/qemu/block-qcow2.c	2009-01-13 13:29:42.000000000 +0000
> +++ kvm-83/qemu/block-qcow2.c	2009-02-13 18:51:12.000000000 +0000
> @@ -52,8 +52,6 @@
>  #define QCOW_CRYPT_NONE 0
>  #define QCOW_CRYPT_AES  1
>  
> -#define QCOW_MAX_CRYPT_CLUSTERS 32
> -
>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1LL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) */
> @@ -269,8 +267,7 @@
>      if (!s->cluster_cache)
>          goto fail;
>      /* one more sector for decompressed data alignment */
> -    s->cluster_data = qemu_malloc(QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
> -                                  + 512);
> +    s->cluster_data = qemu_malloc(s->cluster_size + 512);
>      if (!s->cluster_data)
>          goto fail;
>      s->cluster_cache_offset = -1;
> @@ -437,7 +434,8 @@
>      int new_l1_size, new_l1_size2, ret, i;
>      uint64_t *new_l1_table;
>      uint64_t new_l1_table_offset;
> -    uint8_t data[12];
> +    uint64_t data64;
> +    uint32_t data32;
>  
>      new_l1_size = s->l1_size;
>      if (min_size <= new_l1_size)
> @@ -467,10 +465,13 @@
>          new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
>  
>      /* set new table */
> -    cpu_to_be32w((uint32_t*)data, new_l1_size);
> -    cpu_to_be64w((uint64_t*)(data + 4), new_l1_table_offset);
> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,
> -                sizeof(data)) != sizeof(data))
> +    data64 = cpu_to_be64(new_l1_table_offset);
> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_table_offset),
> +                    &data64, sizeof(data64)) != sizeof(data64))
> +        goto fail;
> +    data32 = cpu_to_be32(new_l1_size);
> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size),
> +                    &data32, sizeof(data32)) != sizeof(data32))
>          goto fail;
>      qemu_free(s->l1_table);
>      free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t));
> @@ -483,549 +484,169 @@
>      return -EIO;
>  }
>  
> -/*
> - * seek_l2_table
> +/* 'allocate' is:
>   *
> - * seek l2_offset in the l2_cache table
> - * if not found, return NULL,
> - * if found,
> - *   increments the l2 cache hit count of the entry,
> - *   if counter overflow, divide by two all counters
> - *   return the pointer to the l2 cache entry
> + * 0 not to allocate.
>   *
> - */
> -
> -static uint64_t *seek_l2_table(BDRVQcowState *s, uint64_t l2_offset)
> -{
> -    int i, j;
> -
> -    for(i = 0; i < L2_CACHE_SIZE; i++) {
> -        if (l2_offset == s->l2_cache_offsets[i]) {
> -            /* increment the hit count */
> -            if (++s->l2_cache_counts[i] == 0xffffffff) {
> -                for(j = 0; j < L2_CACHE_SIZE; j++) {
> -                    s->l2_cache_counts[j] >>= 1;
> -                }
> -            }
> -            return s->l2_cache + (i << s->l2_bits);
> -        }
> -    }
> -    return NULL;
> -}
> -
> -/*
> - * l2_load
> + * 1 to allocate a normal cluster (for sector indexes 'n_start' to
> + * 'n_end')
>   *
> - * Loads a L2 table into memory. If the table is in the cache, the cache
> - * is used; otherwise the L2 table is loaded from the image file.
> + * 2 to allocate a compressed cluster of size
> + * 'compressed_size'. 'compressed_size' must be > 0 and <
> + * cluster_size
>   *
> - * Returns a pointer to the L2 table on success, or NULL if the read from
> - * the image file failed.
> + * return 0 if not allocated.
>   */
> -
> -static uint64_t *l2_load(BlockDriverState *bs, uint64_t l2_offset)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int min_index;
> -    uint64_t *l2_table;
> -
> -    /* seek if the table for the given offset is in the cache */
> -
> -    l2_table = seek_l2_table(s, l2_offset);
> -    if (l2_table != NULL)
> -        return l2_table;
> -
> -    /* not found: load a new entry in the least used one */
> -
> -    min_index = l2_cache_new_entry(bs);
> -    l2_table = s->l2_cache + (min_index << s->l2_bits);
> -    if (bdrv_pread(s->hd, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
> -        s->l2_size * sizeof(uint64_t))
> -        return NULL;
> -    s->l2_cache_offsets[min_index] = l2_offset;
> -    s->l2_cache_counts[min_index] = 1;
> -
> -    return l2_table;
> -}
> -
> -/*
> - * l2_allocate
> - *
> - * Allocate a new l2 entry in the file. If l1_index points to an already
> - * used entry in the L2 table (i.e. we are doing a copy on write for the L2
> - * table) copy the contents of the old L2 table into the newly allocated one.
> - * Otherwise the new table is initialized with zeros.
> - *
> - */
> -
> -static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int min_index;
> -    uint64_t old_l2_offset, tmp;
> -    uint64_t *l2_table, l2_offset;
> -
> -    old_l2_offset = s->l1_table[l1_index];
> -
> -    /* allocate a new l2 entry */
> -
> -    l2_offset = alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
> -
> -    /* update the L1 entry */
> -
> -    s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
> -
> -    tmp = cpu_to_be64(l2_offset | QCOW_OFLAG_COPIED);
> -    if (bdrv_pwrite(s->hd, s->l1_table_offset + l1_index * sizeof(tmp),
> -                    &tmp, sizeof(tmp)) != sizeof(tmp))
> -        return NULL;
> -
> -    /* allocate a new entry in the l2 cache */
> -
> -    min_index = l2_cache_new_entry(bs);
> -    l2_table = s->l2_cache + (min_index << s->l2_bits);
> -
> -    if (old_l2_offset == 0) {
> -        /* if there was no old l2 table, clear the new table */
> -        memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
> -    } else {
> -        /* if there was an old l2 table, read it from the disk */
> -        if (bdrv_pread(s->hd, old_l2_offset,
> -                       l2_table, s->l2_size * sizeof(uint64_t)) !=
> -            s->l2_size * sizeof(uint64_t))
> -            return NULL;
> -    }
> -    /* write the l2 table to the file */
> -    if (bdrv_pwrite(s->hd, l2_offset,
> -                    l2_table, s->l2_size * sizeof(uint64_t)) !=
> -        s->l2_size * sizeof(uint64_t))
> -        return NULL;
> -
> -    /* update the l2 cache entry */
> -
> -    s->l2_cache_offsets[min_index] = l2_offset;
> -    s->l2_cache_counts[min_index] = 1;
> -
> -    return l2_table;
> -}
> -
> -static int size_to_clusters(BDRVQcowState *s, int64_t size)
> -{
> -    return (size + (s->cluster_size - 1)) >> s->cluster_bits;
> -}
> -
> -static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
> -        uint64_t *l2_table, uint64_t start, uint64_t mask)
> -{
> -    int i;
> -    uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
> -
> -    if (!offset)
> -        return 0;
> -
> -    for (i = start; i < start + nb_clusters; i++)
> -        if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) & ~mask))
> -            break;
> -
> -	return (i - start);
> -}
> -
> -static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t *l2_table)
> -{
> -    int i = 0;
> -
> -    while(nb_clusters-- && l2_table[i] == 0)
> -        i++;
> -
> -    return i;
> -}
> -
> -/*
> - * get_cluster_offset
> - *
> - * For a given offset of the disk image, return cluster offset in
> - * qcow2 file.
> - *
> - * on entry, *num is the number of contiguous clusters we'd like to
> - * access following offset.
> - *
> - * on exit, *num is the number of contiguous clusters we can read.
> - *
> - * Return 1, if the offset is found
> - * Return 0, otherwise.
> - *
> - */
> -
>  static uint64_t get_cluster_offset(BlockDriverState *bs,
> -                                   uint64_t offset, int *num)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int l1_index, l2_index;
> -    uint64_t l2_offset, *l2_table, cluster_offset;
> -    int l1_bits, c;
> -    int index_in_cluster, nb_available, nb_needed, nb_clusters;
> -
> -    index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
> -    nb_needed = *num + index_in_cluster;
> -
> -    l1_bits = s->l2_bits + s->cluster_bits;
> -
> -    /* compute how many bytes there are between the offset and
> -     * the end of the l1 entry
> -     */
> -
> -    nb_available = (1 << l1_bits) - (offset & ((1 << l1_bits) - 1));
> -
> -    /* compute the number of available sectors */
> -
> -    nb_available = (nb_available >> 9) + index_in_cluster;
> -
> -    cluster_offset = 0;
> -
> -    /* seek the the l2 offset in the l1 table */
> -
> -    l1_index = offset >> l1_bits;
> -    if (l1_index >= s->l1_size)
> -        goto out;
> -
> -    l2_offset = s->l1_table[l1_index];
> -
> -    /* seek the l2 table of the given l2 offset */
> -
> -    if (!l2_offset)
> -        goto out;
> -
> -    /* load the l2 table in memory */
> -
> -    l2_offset &= ~QCOW_OFLAG_COPIED;
> -    l2_table = l2_load(bs, l2_offset);
> -    if (l2_table == NULL)
> -        return 0;
> -
> -    /* find the cluster offset for the given disk offset */
> -
> -    l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
> -    cluster_offset = be64_to_cpu(l2_table[l2_index]);
> -    nb_clusters = size_to_clusters(s, nb_needed << 9);
> -
> -    if (!cluster_offset) {
> -        /* how many empty clusters ? */
> -        c = count_contiguous_free_clusters(nb_clusters, &l2_table[l2_index]);
> -    } else {
> -        /* how many allocated clusters ? */
> -        c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> -                &l2_table[l2_index], 0, QCOW_OFLAG_COPIED);
> -    }
> -
> -   nb_available = (c * s->cluster_sectors);
> -out:
> -    if (nb_available > nb_needed)
> -        nb_available = nb_needed;
> -
> -    *num = nb_available - index_in_cluster;
> -
> -    return cluster_offset & ~QCOW_OFLAG_COPIED;
> -}
> -
> -/*
> - * free_any_clusters
> - *
> - * free clusters according to its type: compressed or not
> - *
> - */
> -
> -static void free_any_clusters(BlockDriverState *bs,
> -                              uint64_t cluster_offset, int nb_clusters)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -
> -    /* free the cluster */
> -
> -    if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
> -        int nb_csectors;
> -        nb_csectors = ((cluster_offset >> s->csize_shift) &
> -                       s->csize_mask) + 1;
> -        free_clusters(bs, (cluster_offset & s->cluster_offset_mask) & ~511,
> -                      nb_csectors * 512);
> -        return;
> -    }
> -
> -    free_clusters(bs, cluster_offset, nb_clusters << s->cluster_bits);
> -
> -    return;
> -}
> -
> -/*
> - * get_cluster_table
> - *
> - * for a given disk offset, load (and allocate if needed)
> - * the l2 table.
> - *
> - * the l2 table offset in the qcow2 file and the cluster index
> - * in the l2 table are given to the caller.
> - *
> - */
> -
> -static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
> -                             uint64_t **new_l2_table,
> -                             uint64_t *new_l2_offset,
> -                             int *new_l2_index)
> +                                   uint64_t offset, int allocate,
> +                                   int compressed_size,
> +                                   int n_start, int n_end)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int l1_index, l2_index, ret;
> -    uint64_t l2_offset, *l2_table;
> -
> -    /* seek the the l2 offset in the l1 table */
> +    int min_index, i, j, l1_index, l2_index, ret;
> +    uint64_t l2_offset, *l2_table, cluster_offset, tmp, old_l2_offset;
>  
>      l1_index = offset >> (s->l2_bits + s->cluster_bits);
>      if (l1_index >= s->l1_size) {
> -        ret = grow_l1_table(bs, l1_index + 1);
> -        if (ret < 0)
> +        /* outside l1 table is allowed: we grow the table if needed */
> +        if (!allocate)
> +            return 0;
> +        if (grow_l1_table(bs, l1_index + 1) < 0)
>              return 0;
>      }
>      l2_offset = s->l1_table[l1_index];
> +    if (!l2_offset) {
> +        if (!allocate)
> +            return 0;
> +    l2_allocate:
> +        old_l2_offset = l2_offset;
> +        /* allocate a new l2 entry */
> +        l2_offset = alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
> +        /* update the L1 entry */
> +        s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
> +        tmp = cpu_to_be64(l2_offset | QCOW_OFLAG_COPIED);
> +        if (bdrv_pwrite(s->hd, s->l1_table_offset + l1_index * sizeof(tmp),
> +                        &tmp, sizeof(tmp)) != sizeof(tmp))
> +            return 0;
> +        min_index = l2_cache_new_entry(bs);
> +        l2_table = s->l2_cache + (min_index << s->l2_bits);
>  
> -    /* seek the l2 table of the given l2 offset */
> -
> -    if (l2_offset & QCOW_OFLAG_COPIED) {
> -        /* load the l2 table in memory */
> -        l2_offset &= ~QCOW_OFLAG_COPIED;
> -        l2_table = l2_load(bs, l2_offset);
> -        if (l2_table == NULL)
> +        if (old_l2_offset == 0) {
> +            memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
> +        } else {
> +            if (bdrv_pread(s->hd, old_l2_offset,
> +                           l2_table, s->l2_size * sizeof(uint64_t)) !=
> +                s->l2_size * sizeof(uint64_t))
> +                return 0;
> +        }
> +        if (bdrv_pwrite(s->hd, l2_offset,
> +                        l2_table, s->l2_size * sizeof(uint64_t)) !=
> +            s->l2_size * sizeof(uint64_t))
>              return 0;
>      } else {
> -        if (l2_offset)
> -            free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
> -        l2_table = l2_allocate(bs, l1_index);
> -        if (l2_table == NULL)
> +        if (!(l2_offset & QCOW_OFLAG_COPIED)) {
> +            if (allocate) {
> +                free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
> +                goto l2_allocate;
> +            }
> +        } else {
> +            l2_offset &= ~QCOW_OFLAG_COPIED;
> +        }
> +        for(i = 0; i < L2_CACHE_SIZE; i++) {
> +            if (l2_offset == s->l2_cache_offsets[i]) {
> +                /* increment the hit count */
> +                if (++s->l2_cache_counts[i] == 0xffffffff) {
> +                    for(j = 0; j < L2_CACHE_SIZE; j++) {
> +                        s->l2_cache_counts[j] >>= 1;
> +                    }
> +                }
> +                l2_table = s->l2_cache + (i << s->l2_bits);
> +                goto found;
> +            }
> +        }
> +        /* not found: load a new entry in the least used one */
> +        min_index = l2_cache_new_entry(bs);
> +        l2_table = s->l2_cache + (min_index << s->l2_bits);
> +        if (bdrv_pread(s->hd, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
> +            s->l2_size * sizeof(uint64_t))
>              return 0;
> -        l2_offset = s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED;
>      }
> -
> -    /* find the cluster offset for the given disk offset */
> -
> +    s->l2_cache_offsets[min_index] = l2_offset;
> +    s->l2_cache_counts[min_index] = 1;
> + found:
>      l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
> -
> -    *new_l2_table = l2_table;
> -    *new_l2_offset = l2_offset;
> -    *new_l2_index = l2_index;
> -
> -    return 1;
> -}
> -
> -/*
> - * alloc_compressed_cluster_offset
> - *
> - * For a given offset of the disk image, return cluster offset in
> - * qcow2 file.
> - *
> - * If the offset is not found, allocate a new compressed cluster.
> - *
> - * Return the cluster offset if successful,
> - * Return 0, otherwise.
> - *
> - */
> -
> -static uint64_t alloc_compressed_cluster_offset(BlockDriverState *bs,
> -                                                uint64_t offset,
> -                                                int compressed_size)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int l2_index, ret;
> -    uint64_t l2_offset, *l2_table, cluster_offset;
> -    int nb_csectors;
> -
> -    ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
> -    if (ret == 0)
> -        return 0;
> -
>      cluster_offset = be64_to_cpu(l2_table[l2_index]);
> -    if (cluster_offset & QCOW_OFLAG_COPIED)
> -        return cluster_offset & ~QCOW_OFLAG_COPIED;
> -
> -    if (cluster_offset)
> -        free_any_clusters(bs, cluster_offset, 1);
> -
> -    cluster_offset = alloc_bytes(bs, compressed_size);
> -    nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
> -                  (cluster_offset >> 9);
> -
> -    cluster_offset |= QCOW_OFLAG_COMPRESSED |
> -                      ((uint64_t)nb_csectors << s->csize_shift);
> -
> -    /* update L2 table */
> -
> -    /* compressed clusters never have the copied flag */
> -
> -    l2_table[l2_index] = cpu_to_be64(cluster_offset);
> -    if (bdrv_pwrite(s->hd,
> -                    l2_offset + l2_index * sizeof(uint64_t),
> -                    l2_table + l2_index,
> -                    sizeof(uint64_t)) != sizeof(uint64_t))
> -        return 0;
> -
> -    return cluster_offset;
> -}
> -
> -typedef struct QCowL2Meta
> -{
> -    uint64_t offset;
> -    int n_start;
> -    int nb_available;
> -    int nb_clusters;
> -} QCowL2Meta;
> -
> -static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
> -        QCowL2Meta *m)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int i, j = 0, l2_index, ret;
> -    uint64_t *old_cluster, start_sect, l2_offset, *l2_table;
> -
> -    if (m->nb_clusters == 0)
> -        return 0;
> -
> -    if (!(old_cluster = qemu_malloc(m->nb_clusters * sizeof(uint64_t))))
> -        return -ENOMEM;
> -
> -    /* copy content of unmodified sectors */
> -    start_sect = (m->offset & ~(s->cluster_size - 1)) >> 9;
> -    if (m->n_start) {
> -        ret = copy_sectors(bs, start_sect, cluster_offset, 0, m->n_start);
> -        if (ret < 0)
> -            goto err;
> +    if (!cluster_offset) {
> +        if (!allocate)
> +            return cluster_offset;
> +    } else if (!(cluster_offset & QCOW_OFLAG_COPIED)) {
> +        if (!allocate)
> +            return cluster_offset;
> +        /* free the cluster */
> +        if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
> +            int nb_csectors;
> +            nb_csectors = ((cluster_offset >> s->csize_shift) &
> +                           s->csize_mask) + 1;
> +            free_clusters(bs, (cluster_offset & s->cluster_offset_mask) & ~511,
> +                          nb_csectors * 512);
> +        } else {
> +            free_clusters(bs, cluster_offset, s->cluster_size);
> +        }
> +    } else {
> +        cluster_offset &= ~QCOW_OFLAG_COPIED;
> +        return cluster_offset;
>      }
> -
> -    if (m->nb_available & (s->cluster_sectors - 1)) {
> -        uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1);
> -        ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9),
> -                m->nb_available - end, s->cluster_sectors);
> -        if (ret < 0)
> -            goto err;
> +    if (allocate == 1) {
> +        /* allocate a new cluster */
> +        cluster_offset = alloc_clusters(bs, s->cluster_size);
> +
> +        /* we must initialize the cluster content which won't be
> +           written */
> +        if ((n_end - n_start) < s->cluster_sectors) {
> +            uint64_t start_sect;
> +
> +            start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> +            ret = copy_sectors(bs, start_sect,
> +                               cluster_offset, 0, n_start);
> +            if (ret < 0)
> +                return 0;
> +            ret = copy_sectors(bs, start_sect,
> +                               cluster_offset, n_end, s->cluster_sectors);
> +            if (ret < 0)
> +                return 0;
> +        }
> +        tmp = cpu_to_be64(cluster_offset | QCOW_OFLAG_COPIED);
> +    } else {
> +        int nb_csectors;
> +        cluster_offset = alloc_bytes(bs, compressed_size);
> +        nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
> +            (cluster_offset >> 9);
> +        cluster_offset |= QCOW_OFLAG_COMPRESSED |
> +            ((uint64_t)nb_csectors << s->csize_shift);
> +        /* compressed clusters never have the copied flag */
> +        tmp = cpu_to_be64(cluster_offset);
>      }
> -
> -    ret = -EIO;
>      /* update L2 table */
> -    if (!get_cluster_table(bs, m->offset, &l2_table, &l2_offset, &l2_index))
> -        goto err;
> -
> -    for (i = 0; i < m->nb_clusters; i++) {
> -        if(l2_table[l2_index + i] != 0)
> -            old_cluster[j++] = l2_table[l2_index + i];
> -
> -        l2_table[l2_index + i] = cpu_to_be64((cluster_offset +
> -                    (i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
> -     }
> -
> -    if (bdrv_pwrite(s->hd, l2_offset + l2_index * sizeof(uint64_t),
> -                l2_table + l2_index, m->nb_clusters * sizeof(uint64_t)) !=
> -            m->nb_clusters * sizeof(uint64_t))
> -        goto err;
> -
> -    for (i = 0; i < j; i++)
> -        free_any_clusters(bs, old_cluster[i], 1);
> -
> -    ret = 0;
> -err:
> -    qemu_free(old_cluster);
> -    return ret;
> - }
> -
> -/*
> - * alloc_cluster_offset
> - *
> - * For a given offset of the disk image, return cluster offset in
> - * qcow2 file.
> - *
> - * If the offset is not found, allocate a new cluster.
> - *
> - * Return the cluster offset if successful,
> - * Return 0, otherwise.
> - *
> - */
> -
> -static uint64_t alloc_cluster_offset(BlockDriverState *bs,
> -                                     uint64_t offset,
> -                                     int n_start, int n_end,
> -                                     int *num, QCowL2Meta *m)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int l2_index, ret;
> -    uint64_t l2_offset, *l2_table, cluster_offset;
> -    int nb_clusters, i = 0;
> -
> -    ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
> -    if (ret == 0)
> +    l2_table[l2_index] = tmp;
> +    if (bdrv_pwrite(s->hd,
> +                    l2_offset + l2_index * sizeof(tmp), &tmp, sizeof(tmp)) != sizeof(tmp))
>          return 0;
> -
> -    nb_clusters = size_to_clusters(s, n_end << 9);
> -
> -    nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
> -
> -    cluster_offset = be64_to_cpu(l2_table[l2_index]);
> -
> -    /* We keep all QCOW_OFLAG_COPIED clusters */
> -
> -    if (cluster_offset & QCOW_OFLAG_COPIED) {
> -        nb_clusters = count_contiguous_clusters(nb_clusters, s->cluster_size,
> -                &l2_table[l2_index], 0, 0);
> -
> -        cluster_offset &= ~QCOW_OFLAG_COPIED;
> -        m->nb_clusters = 0;
> -
> -        goto out;
> -    }
> -
> -    /* for the moment, multiple compressed clusters are not managed */
> -
> -    if (cluster_offset & QCOW_OFLAG_COMPRESSED)
> -        nb_clusters = 1;
> -
> -    /* how many available clusters ? */
> -
> -    while (i < nb_clusters) {
> -        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
> -                &l2_table[l2_index], i, 0);
> -
> -        if(be64_to_cpu(l2_table[l2_index + i]))
> -            break;
> -
> -        i += count_contiguous_free_clusters(nb_clusters - i,
> -                &l2_table[l2_index + i]);
> -
> -        cluster_offset = be64_to_cpu(l2_table[l2_index + i]);
> -
> -        if ((cluster_offset & QCOW_OFLAG_COPIED) ||
> -                (cluster_offset & QCOW_OFLAG_COMPRESSED))
> -            break;
> -    }
> -    nb_clusters = i;
> -
> -    /* allocate a new cluster */
> -
> -    cluster_offset = alloc_clusters(bs, nb_clusters * s->cluster_size);
> -
> -    /* save info needed for meta data update */
> -    m->offset = offset;
> -    m->n_start = n_start;
> -    m->nb_clusters = nb_clusters;
> -
> -out:
> -    m->nb_available = MIN(nb_clusters << (s->cluster_bits - 9), n_end);
> -
> -    *num = m->nb_available - n_start;
> -
>      return cluster_offset;
>  }
>  
>  static int qcow_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>  {
> +    BDRVQcowState *s = bs->opaque;
> +    int index_in_cluster, n;
>      uint64_t cluster_offset;
>  
> -    *pnum = nb_sectors;
> -    cluster_offset = get_cluster_offset(bs, sector_num << 9, pnum);
> -
> +    cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
> +    index_in_cluster = sector_num & (s->cluster_sectors - 1);
> +    n = s->cluster_sectors - index_in_cluster;
> +    if (n > nb_sectors)
> +        n = nb_sectors;
> +    *pnum = n;
>      return (cluster_offset != 0);
>  }
>  
> @@ -1102,9 +723,11 @@
>      uint64_t cluster_offset;
>  
>      while (nb_sectors > 0) {
> -        n = nb_sectors;
> -        cluster_offset = get_cluster_offset(bs, sector_num << 9, &n);
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> +        n = s->cluster_sectors - index_in_cluster;
> +        if (n > nb_sectors)
> +            n = nb_sectors;
>          if (!cluster_offset) {
>              if (bs->backing_hd) {
>                  /* read from the base image */
> @@ -1143,18 +766,15 @@
>      BDRVQcowState *s = bs->opaque;
>      int ret, index_in_cluster, n;
>      uint64_t cluster_offset;
> -    int n_end;
> -    QCowL2Meta l2meta;
>  
>      while (nb_sectors > 0) {
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n_end = index_in_cluster + nb_sectors;
> -        if (s->crypt_method &&
> -            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
> -            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> -        cluster_offset = alloc_cluster_offset(bs, sector_num << 9,
> -                                              index_in_cluster,
> -                                              n_end, &n, &l2meta);
> +        n = s->cluster_sectors - index_in_cluster;
> +        if (n > nb_sectors)
> +            n = nb_sectors;
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
> +                                            index_in_cluster,
> +                                            index_in_cluster + n);
>          if (!cluster_offset)
>              return -1;
>          if (s->crypt_method) {
> @@ -1165,10 +785,8 @@
>          } else {
>              ret = bdrv_pwrite(s->hd, cluster_offset + index_in_cluster * 512, buf, n * 512);
>          }
> -        if (ret != n * 512 || alloc_cluster_link_l2(bs, cluster_offset, &l2meta) < 0) {
> -            free_any_clusters(bs, cluster_offset, l2meta.nb_clusters);
> +        if (ret != n * 512)
>              return -1;
> -        }
>          nb_sectors -= n;
>          sector_num += n;
>          buf += n * 512;
> @@ -1186,33 +804,8 @@
>      uint64_t cluster_offset;
>      uint8_t *cluster_data;
>      BlockDriverAIOCB *hd_aiocb;
> -    QEMUBH *bh;
> -    QCowL2Meta l2meta;
>  } QCowAIOCB;
>  
> -static void qcow_aio_read_cb(void *opaque, int ret);
> -static void qcow_aio_read_bh(void *opaque)
> -{
> -    QCowAIOCB *acb = opaque;
> -    qemu_bh_delete(acb->bh);
> -    acb->bh = NULL;
> -    qcow_aio_read_cb(opaque, 0);
> -}
> -
> -static int qcow_schedule_bh(QEMUBHFunc *cb, QCowAIOCB *acb)
> -{
> -    if (acb->bh)
> -        return -EIO;
> -
> -    acb->bh = qemu_bh_new(cb, acb);
> -    if (!acb->bh)
> -        return -EIO;
> -
> -    qemu_bh_schedule(acb->bh);
> -
> -    return 0;
> -}
> -
>  static void qcow_aio_read_cb(void *opaque, int ret)
>  {
>      QCowAIOCB *acb = opaque;
> @@ -1222,12 +815,13 @@
>  
>      acb->hd_aiocb = NULL;
>      if (ret < 0) {
> -fail:
> +    fail:
>          acb->common.cb(acb->common.opaque, ret);
>          qemu_aio_release(acb);
>          return;
>      }
>  
> + redo:
>      /* post process the read buffer */
>      if (!acb->cluster_offset) {
>          /* nothing to do */
> @@ -1253,9 +847,12 @@
>      }
>  
>      /* prepare next AIO request */
> -    acb->n = acb->nb_sectors;
> -    acb->cluster_offset = get_cluster_offset(bs, acb->sector_num << 9, &acb->n);
> +    acb->cluster_offset = get_cluster_offset(bs, acb->sector_num << 9,
> +                                             0, 0, 0, 0);
>      index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
> +    acb->n = s->cluster_sectors - index_in_cluster;
> +    if (acb->n > acb->nb_sectors)
> +        acb->n = acb->nb_sectors;
>  
>      if (!acb->cluster_offset) {
>          if (bs->backing_hd) {
> @@ -1268,16 +865,12 @@
>                  if (acb->hd_aiocb == NULL)
>                      goto fail;
>              } else {
> -                ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
> -                if (ret < 0)
> -                    goto fail;
> +                goto redo;
>              }
>          } else {
>              /* Note: in this case, no need to wait */
>              memset(acb->buf, 0, 512 * acb->n);
> -            ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
> -            if (ret < 0)
> -                goto fail;
> +            goto redo;
>          }
>      } else if (acb->cluster_offset & QCOW_OFLAG_COMPRESSED) {
>          /* add AIO support for compressed blocks ? */
> @@ -1285,9 +878,7 @@
>              goto fail;
>          memcpy(acb->buf,
>                 s->cluster_cache + index_in_cluster * 512, 512 * acb->n);
> -        ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
> -        if (ret < 0)
> -            goto fail;
> +        goto redo;
>      } else {
>          if ((acb->cluster_offset & 511) != 0) {
>              ret = -EIO;
> @@ -1316,7 +907,6 @@
>      acb->nb_sectors = nb_sectors;
>      acb->n = 0;
>      acb->cluster_offset = 0;
> -    acb->l2meta.nb_clusters = 0;
>      return acb;
>  }
>  
> @@ -1340,8 +930,8 @@
>      BlockDriverState *bs = acb->common.bs;
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
> +    uint64_t cluster_offset;
>      const uint8_t *src_buf;
> -    int n_end;
>  
>      acb->hd_aiocb = NULL;
>  
> @@ -1352,11 +942,6 @@
>          return;
>      }
>  
> -    if (alloc_cluster_link_l2(bs, acb->cluster_offset, &acb->l2meta) < 0) {
> -        free_any_clusters(bs, acb->cluster_offset, acb->l2meta.nb_clusters);
> -        goto fail;
> -    }
> -
>      acb->nb_sectors -= acb->n;
>      acb->sector_num += acb->n;
>      acb->buf += acb->n * 512;
> @@ -1369,22 +954,19 @@
>      }
>  
>      index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
> -    n_end = index_in_cluster + acb->nb_sectors;
> -    if (s->crypt_method &&
> -        n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
> -        n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> -
> -    acb->cluster_offset = alloc_cluster_offset(bs, acb->sector_num << 9,
> -                                          index_in_cluster,
> -                                          n_end, &acb->n, &acb->l2meta);
> -    if (!acb->cluster_offset || (acb->cluster_offset & 511) != 0) {
> +    acb->n = s->cluster_sectors - index_in_cluster;
> +    if (acb->n > acb->nb_sectors)
> +        acb->n = acb->nb_sectors;
> +    cluster_offset = get_cluster_offset(bs, acb->sector_num << 9, 1, 0,
> +                                        index_in_cluster,
> +                                        index_in_cluster + acb->n);
> +    if (!cluster_offset || (cluster_offset & 511) != 0) {
>          ret = -EIO;
>          goto fail;
>      }
>      if (s->crypt_method) {
>          if (!acb->cluster_data) {
> -            acb->cluster_data = qemu_mallocz(QCOW_MAX_CRYPT_CLUSTERS *
> -                                             s->cluster_size);
> +            acb->cluster_data = qemu_mallocz(s->cluster_size);
>              if (!acb->cluster_data) {
>                  ret = -ENOMEM;
>                  goto fail;
> @@ -1397,7 +979,7 @@
>          src_buf = acb->buf;
>      }
>      acb->hd_aiocb = bdrv_aio_write(s->hd,
> -                                   (acb->cluster_offset >> 9) + index_in_cluster,
> +                                   (cluster_offset >> 9) + index_in_cluster,
>                                     src_buf, acb->n,
>                                     qcow_aio_write_cb, acb);
>      if (acb->hd_aiocb == NULL)
> @@ -1571,7 +1153,7 @@
>  
>      memset(s->l1_table, 0, l1_length);
>      if (bdrv_pwrite(s->hd, s->l1_table_offset, s->l1_table, l1_length) < 0)
> -        return -1;
> +	return -1;
>      ret = bdrv_truncate(s->hd, s->l1_table_offset + l1_length);
>      if (ret < 0)
>          return ret;
> @@ -1637,10 +1219,8 @@
>          /* could not compress: write normal cluster */
>          qcow_write(bs, sector_num, buf, s->cluster_sectors);
>      } else {
> -        cluster_offset = alloc_compressed_cluster_offset(bs, sector_num << 9,
> -                                              out_len);
> -        if (!cluster_offset)
> -            return -1;
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 2,
> +                                            out_len, 0, 0);
>          cluster_offset &= s->cluster_offset_mask;
>          if (bdrv_pwrite(s->hd, cluster_offset, out_buf, out_len) != out_len) {
>              qemu_free(out_buf);
> @@ -2225,19 +1805,26 @@
>      BDRVQcowState *s = bs->opaque;
>      int i, nb_clusters;
>  
> -    nb_clusters = size_to_clusters(s, size);
> -retry:
> -    for(i = 0; i < nb_clusters; i++) {
> -        int64_t i = s->free_cluster_index++;
> -        if (get_refcount(bs, i) != 0)
> -            goto retry;
> -    }
> +    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
> +    for(;;) {
> +        if (get_refcount(bs, s->free_cluster_index) == 0) {
> +            s->free_cluster_index++;
> +            for(i = 1; i < nb_clusters; i++) {
> +                if (get_refcount(bs, s->free_cluster_index) != 0)
> +                    goto not_found;
> +                s->free_cluster_index++;
> +            }
>  #ifdef DEBUG_ALLOC2
> -    printf("alloc_clusters: size=%lld -> %lld\n",
> -            size,
> -            (s->free_cluster_index - nb_clusters) << s->cluster_bits);
> +            printf("alloc_clusters: size=%lld -> %lld\n",
> +                   size,
> +                   (s->free_cluster_index - nb_clusters) << s->cluster_bits);
>  #endif
> -    return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
> +            return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
> +        } else {
> +        not_found:
> +            s->free_cluster_index++;
> +        }
> +    }
>  }
>  
>  static int64_t alloc_clusters(BlockDriverState *bs, int64_t size)
> @@ -2301,7 +1888,8 @@
>      int new_table_size, new_table_size2, refcount_table_clusters, i, ret;
>      uint64_t *new_table;
>      int64_t table_offset;
> -    uint8_t data[12];
> +    uint64_t data64;
> +    uint32_t data32;
>      int old_table_size;
>      int64_t old_table_offset;
>  
> @@ -2340,10 +1928,13 @@
>      for(i = 0; i < s->refcount_table_size; i++)
>          be64_to_cpus(&new_table[i]);
>  
> -    cpu_to_be64w((uint64_t*)data, table_offset);
> -    cpu_to_be32w((uint32_t*)(data + 8), refcount_table_clusters);
> +    data64 = cpu_to_be64(table_offset);
>      if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
> -                    data, sizeof(data)) != sizeof(data))
> +                    &data64, sizeof(data64)) != sizeof(data64))
> +        goto fail;
> +    data32 = cpu_to_be32(refcount_table_clusters);
> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_clusters),
> +                    &data32, sizeof(data32)) != sizeof(data32))
>          goto fail;
>      qemu_free(s->refcount_table);
>      old_table_offset = s->refcount_table_offset;
> @@ -2572,7 +2163,7 @@
>      uint16_t *refcount_table;
>  
>      size = bdrv_getlength(s->hd);
> -    nb_clusters = size_to_clusters(s, size);
> +    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
>      refcount_table = qemu_mallocz(nb_clusters * sizeof(uint16_t));
>  
>      /* header */
> @@ -2624,7 +2215,7 @@
>      int refcount;
>  
>      size = bdrv_getlength(s->hd);
> -    nb_clusters = size_to_clusters(s, size);
> +    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
>      for(k = 0; k < nb_clusters;) {
>          k1 = k;
>          refcount = get_refcount(bs, k);
>
>
>   


[-- Attachment #2: Type: text/html, Size: 43068 bytes --]

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Jamie Lokier]

Dor Laor wrote:
> The solution is to find the real cause to the corruption.

I agree, if someone is able to do that, great, but if not and
practical reality results in these choices:

    1. Ship the current code which results in corruption on Windows
       2000 and 2003 guests (and who knows what else), and by the way
       is unlikely to have anything to do with device emulation.

    2. Revert to (nearly) kvm-72 code which appears to fix the
       majority of those corruption cases, although there is still
       something rare, which may be a different bug.

Which is the best choice?

>From a QA POV, I would revert the known bug until someone has a fix,
then reinstate everything after it which is thought to be good.

> Jamie Lokier wrote:
>      Anthony Liguori wrote:
>                Simply reverting the qcow2 code appears to fix
>                those problems, so it
>                needn't hold up cutting a release.  That's what I
>                recommend.
>           Send some patches.
>      I did already.
> 
>      Here it is again.  This should fix my bug and Marc's bug according to
>      his report that reverting qcow2.c fixes it.
> Going back to kvm-72 is not good also.

> First, there were qcow2 corruptions before it, they were very rare but still
> exist.

That's true.  But they were noticably rarer - to the point that people
clearly are using kvm-72 with qcow2 and not reporting many problems.

Ubuntu 8.10 shipped kvm-72, and that coincided with their announcement
that they're supporting KVM as their official virtualisation solution.
I imagine kvm-72 is getting a fair bit of usage because of that.

Of course they could be having rare problems and think it's a bug in
the guest or its applications :-)

> Not long ago we did not know even that qcow2 is the faulty.

Worrying, isn't it.  Does qcow2 get any rigorous testing?  Should that
be added - a blockdev test suite?

There hasn't been a complete lack of bug reports about qcow2, but
maybe they aren't getting to the right places, and maybe they're too
difficult to reproduce and easy to workaround ("my guest occasionally
shows random corruption", "don't use KVM for that guest", "I switch to
raw and it went away")

I very luckily discovered it prevented one of my VMs from booting, as
soon as I upgraded from kvm-72 (shipped with Ubuntu) to something
newer.  If it hadn't prevented it from booting, just occasional rare
corruption, I might not have realised it was qcow2 at all.  Guest
corruption can occur for many reasons, and -win2k-hack implies that
the IDE emulation is not quite right in some way.

-- Jamie

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>>> Simply reverting the qcow2 code appears to fix those problems, so it
>>> needn't hold up cutting a release.  That's what I recommend.
>>>       
>> Send some patches.
>>     
>
> I did already.
>
> Here it is again.  This should fix my bug and Marc's bug according to
> his report that reverting qcow2.c fixes it.
>   

Well such a large reversion is a bad idea.  Can you git bisect to the 
actual changeset that introduced the bug you see?

You're effectively reverting a very large number of changes whereas only 
one is likely causing your problem

Regards,

Anthony Liguori

> -- Jamie
>
>
> Subject: Revert block-qcow2.c to kvm-72 version due to corruption reports
>
> This fixes two kinds of qcow2 corruption observed in kvm-83 (actually
> kvm-73 and later), from three bug reports.
>
>
> Bug 1: Windows 2000 guests complain of corrupt registry.
>
> Many Windows 2000 guests which boot and runs fine in kvm-72, fail with
> a blue-screen indicating file corruption errors in kvm-73 through to
> kvm-83 (the latest), and succeed if we replace block-qcow2.c with the
> version from kvm-72.
>
> The blue screen appears towards the end of the boot sequence, and
> shows only briefly before rebooting.  It says:
>
>     STOP: c0000218 (Registry File Failure)
>     The registry cannot load the hive (file):
>     \SystemRoot\System32\Config\SOFTWARE
>     or its log or alternate.
>     It is corrupt, absent, or not writable.
>
>     Beginning dump of physical memory
>     Physical memory dump complete. Contact your system administrator or
>     technical support [...?]
>
> This is narrowed down to the difference in block-qcow2.c between
> kvm-72 and kvm-73 (not -83).  From kvm-73 to kvm-83, there have been
> more changes block-qcow2.c, but the observed corruption still occurs.
>
> The bug isn't evident when only reading.  When using "qemu-img
> convert" to convert a qcow2 file to a raw file, with broken and fixed
> versions of block-qcow2.c it produces the same raw file.  Also, when
> using "-snapshot" with qemu, the blue screen doesn't occur.
>
> This bug was observed by Jamie Lokier <jamie@shareable.org> and 
> confirmed for multiple Windows 2000 guests by
> Marc Bevand <m.bevand@gmail.com>.
>
>
> Bug 2: Windows 2003 guests complain of corrupt registry.
>
> According to
> http://sourceforge.net/tracker/?func=detail&atid=893831&aid=2001452&group_id=180599
>
> Windows 2003 32-bit guests randomly spew disk corruption messages
> like this:
>
>     Windows – Registry Hive Recovered
>     Registry hive (file): SOFTWARE was corrupted and it has
>     been recovered. Some data might have been lost.
>
> and
>
>     The system cannot log on due to the following error:
>     Unable to complete the requested operation because of
>     either a catastrophic media failure or a data structure
>     corruption on the disk.
>
> This bug was reported by <gerdwachs@users.sourceforge.net> and
> confirmed by Marc Bevand, noting:
>
>     kvm-73+ also causes some of my Windows 2003 guests to exhibit this
>     exact registry corruption error.  [...]  This bug is also fixed by
>     reverting block-qcow2.c to the version from kvm-72.
>
> Worryingly, gerdwachs' bug report says it's for kvm-70, implying this
> patch may not fix all the Windows 2003 guest corruption problems.
>
> At least Marc says his observed problem goes away with kvm-72's qcow2.
>
>
> Bug 3: Corruption of qcow2 index rendering the file unusable.
>
> Marc Bevand writes:
>
>     I tested kvm-81 and kvm-83 as well (can't test kvm-80 or older because
>     of the qcow2 performance regression caused by the default writethrough
>     caching policy) but it randomly triggers an even worse bug: the moment
>     I shut down a guest by typing "quit" in the monitor, it sometimes
>     overwrite the first 4kB of the disk image with mostly NUL bytes (!)
>     which completely destroys it. I am familiar with the qcow2 format and
>     apparently this 4kB block seems to be an L2 table with most entries
>     set to zero. I have had to restore at least 6 or 7 disk images from
>     backup after occurences of that bug. My intuition tells me this may be
>     the qcow2 code trying to allocate a cluster to write a new L2 table,
>     but not noticing the allocation failed (represented by a 0 offset),
>     and writing the L2 table at that 0 offset, overwriting the qcow2
>     header.
>
>     Fortunately this bug is also fixed by running kvm-75 with
>     block-qcow2.c reverted to its kvm-72 version.
>
>     Basically qcow2 in kvm-73 or newer is completely unreliable.
>
>
> Reverting block-qcow2.c to the version in kvm-72 appears to fix the
> corruption symptoms reported by Marc and Jamie, although gerdwachs'
> related bug is against kvm-70 so it may not fix that.
>
> Unfortunately this reverts some optimisations, but fixing corruption
> is more important until the new code is reliable.
>
> This patch reverts block-qcow2.c in kvm-83 to the version in kvm-72,
> except the "cache=writeback" default performance tweak is retained and
> there's no need to define "offsetof".
>
> Signed-Off-By: Jamie Lokier <jamie@shareable.org>
>
>
> --- kvm-83-real/qemu/block-qcow2.c	2009-01-13 13:29:42.000000000 +0000
> +++ kvm-83/qemu/block-qcow2.c	2009-02-13 18:51:12.000000000 +0000
> @@ -52,8 +52,6 @@
>  #define QCOW_CRYPT_NONE 0
>  #define QCOW_CRYPT_AES  1
>  
> -#define QCOW_MAX_CRYPT_CLUSTERS 32
> -
>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1LL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) */
> @@ -269,8 +267,7 @@
>      if (!s->cluster_cache)
>          goto fail;
>      /* one more sector for decompressed data alignment */
> -    s->cluster_data = qemu_malloc(QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
> -                                  + 512);
> +    s->cluster_data = qemu_malloc(s->cluster_size + 512);
>      if (!s->cluster_data)
>          goto fail;
>      s->cluster_cache_offset = -1;
> @@ -437,7 +434,8 @@
>      int new_l1_size, new_l1_size2, ret, i;
>      uint64_t *new_l1_table;
>      uint64_t new_l1_table_offset;
> -    uint8_t data[12];
> +    uint64_t data64;
> +    uint32_t data32;
>  
>      new_l1_size = s->l1_size;
>      if (min_size <= new_l1_size)
> @@ -467,10 +465,13 @@
>          new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
>  
>      /* set new table */
> -    cpu_to_be32w((uint32_t*)data, new_l1_size);
> -    cpu_to_be64w((uint64_t*)(data + 4), new_l1_table_offset);
> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,
> -                sizeof(data)) != sizeof(data))
> +    data64 = cpu_to_be64(new_l1_table_offset);
> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_table_offset),
> +                    &data64, sizeof(data64)) != sizeof(data64))
> +        goto fail;
> +    data32 = cpu_to_be32(new_l1_size);
> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size),
> +                    &data32, sizeof(data32)) != sizeof(data32))
>          goto fail;
>      qemu_free(s->l1_table);
>      free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t));
> @@ -483,549 +484,169 @@
>      return -EIO;
>  }
>  
> -/*
> - * seek_l2_table
> +/* 'allocate' is:
>   *
> - * seek l2_offset in the l2_cache table
> - * if not found, return NULL,
> - * if found,
> - *   increments the l2 cache hit count of the entry,
> - *   if counter overflow, divide by two all counters
> - *   return the pointer to the l2 cache entry
> + * 0 not to allocate.
>   *
> - */
> -
> -static uint64_t *seek_l2_table(BDRVQcowState *s, uint64_t l2_offset)
> -{
> -    int i, j;
> -
> -    for(i = 0; i < L2_CACHE_SIZE; i++) {
> -        if (l2_offset == s->l2_cache_offsets[i]) {
> -            /* increment the hit count */
> -            if (++s->l2_cache_counts[i] == 0xffffffff) {
> -                for(j = 0; j < L2_CACHE_SIZE; j++) {
> -                    s->l2_cache_counts[j] >>= 1;
> -                }
> -            }
> -            return s->l2_cache + (i << s->l2_bits);
> -        }
> -    }
> -    return NULL;
> -}
> -
> -/*
> - * l2_load
> + * 1 to allocate a normal cluster (for sector indexes 'n_start' to
> + * 'n_end')
>   *
> - * Loads a L2 table into memory. If the table is in the cache, the cache
> - * is used; otherwise the L2 table is loaded from the image file.
> + * 2 to allocate a compressed cluster of size
> + * 'compressed_size'. 'compressed_size' must be > 0 and <
> + * cluster_size
>   *
> - * Returns a pointer to the L2 table on success, or NULL if the read from
> - * the image file failed.
> + * return 0 if not allocated.
>   */
> -
> -static uint64_t *l2_load(BlockDriverState *bs, uint64_t l2_offset)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int min_index;
> -    uint64_t *l2_table;
> -
> -    /* seek if the table for the given offset is in the cache */
> -
> -    l2_table = seek_l2_table(s, l2_offset);
> -    if (l2_table != NULL)
> -        return l2_table;
> -
> -    /* not found: load a new entry in the least used one */
> -
> -    min_index = l2_cache_new_entry(bs);
> -    l2_table = s->l2_cache + (min_index << s->l2_bits);
> -    if (bdrv_pread(s->hd, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
> -        s->l2_size * sizeof(uint64_t))
> -        return NULL;
> -    s->l2_cache_offsets[min_index] = l2_offset;
> -    s->l2_cache_counts[min_index] = 1;
> -
> -    return l2_table;
> -}
> -
> -/*
> - * l2_allocate
> - *
> - * Allocate a new l2 entry in the file. If l1_index points to an already
> - * used entry in the L2 table (i.e. we are doing a copy on write for the L2
> - * table) copy the contents of the old L2 table into the newly allocated one.
> - * Otherwise the new table is initialized with zeros.
> - *
> - */
> -
> -static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int min_index;
> -    uint64_t old_l2_offset, tmp;
> -    uint64_t *l2_table, l2_offset;
> -
> -    old_l2_offset = s->l1_table[l1_index];
> -
> -    /* allocate a new l2 entry */
> -
> -    l2_offset = alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
> -
> -    /* update the L1 entry */
> -
> -    s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
> -
> -    tmp = cpu_to_be64(l2_offset | QCOW_OFLAG_COPIED);
> -    if (bdrv_pwrite(s->hd, s->l1_table_offset + l1_index * sizeof(tmp),
> -                    &tmp, sizeof(tmp)) != sizeof(tmp))
> -        return NULL;
> -
> -    /* allocate a new entry in the l2 cache */
> -
> -    min_index = l2_cache_new_entry(bs);
> -    l2_table = s->l2_cache + (min_index << s->l2_bits);
> -
> -    if (old_l2_offset == 0) {
> -        /* if there was no old l2 table, clear the new table */
> -        memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
> -    } else {
> -        /* if there was an old l2 table, read it from the disk */
> -        if (bdrv_pread(s->hd, old_l2_offset,
> -                       l2_table, s->l2_size * sizeof(uint64_t)) !=
> -            s->l2_size * sizeof(uint64_t))
> -            return NULL;
> -    }
> -    /* write the l2 table to the file */
> -    if (bdrv_pwrite(s->hd, l2_offset,
> -                    l2_table, s->l2_size * sizeof(uint64_t)) !=
> -        s->l2_size * sizeof(uint64_t))
> -        return NULL;
> -
> -    /* update the l2 cache entry */
> -
> -    s->l2_cache_offsets[min_index] = l2_offset;
> -    s->l2_cache_counts[min_index] = 1;
> -
> -    return l2_table;
> -}
> -
> -static int size_to_clusters(BDRVQcowState *s, int64_t size)
> -{
> -    return (size + (s->cluster_size - 1)) >> s->cluster_bits;
> -}
> -
> -static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
> -        uint64_t *l2_table, uint64_t start, uint64_t mask)
> -{
> -    int i;
> -    uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
> -
> -    if (!offset)
> -        return 0;
> -
> -    for (i = start; i < start + nb_clusters; i++)
> -        if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) & ~mask))
> -            break;
> -
> -	return (i - start);
> -}
> -
> -static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t *l2_table)
> -{
> -    int i = 0;
> -
> -    while(nb_clusters-- && l2_table[i] == 0)
> -        i++;
> -
> -    return i;
> -}
> -
> -/*
> - * get_cluster_offset
> - *
> - * For a given offset of the disk image, return cluster offset in
> - * qcow2 file.
> - *
> - * on entry, *num is the number of contiguous clusters we'd like to
> - * access following offset.
> - *
> - * on exit, *num is the number of contiguous clusters we can read.
> - *
> - * Return 1, if the offset is found
> - * Return 0, otherwise.
> - *
> - */
> -
>  static uint64_t get_cluster_offset(BlockDriverState *bs,
> -                                   uint64_t offset, int *num)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int l1_index, l2_index;
> -    uint64_t l2_offset, *l2_table, cluster_offset;
> -    int l1_bits, c;
> -    int index_in_cluster, nb_available, nb_needed, nb_clusters;
> -
> -    index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
> -    nb_needed = *num + index_in_cluster;
> -
> -    l1_bits = s->l2_bits + s->cluster_bits;
> -
> -    /* compute how many bytes there are between the offset and
> -     * the end of the l1 entry
> -     */
> -
> -    nb_available = (1 << l1_bits) - (offset & ((1 << l1_bits) - 1));
> -
> -    /* compute the number of available sectors */
> -
> -    nb_available = (nb_available >> 9) + index_in_cluster;
> -
> -    cluster_offset = 0;
> -
> -    /* seek the the l2 offset in the l1 table */
> -
> -    l1_index = offset >> l1_bits;
> -    if (l1_index >= s->l1_size)
> -        goto out;
> -
> -    l2_offset = s->l1_table[l1_index];
> -
> -    /* seek the l2 table of the given l2 offset */
> -
> -    if (!l2_offset)
> -        goto out;
> -
> -    /* load the l2 table in memory */
> -
> -    l2_offset &= ~QCOW_OFLAG_COPIED;
> -    l2_table = l2_load(bs, l2_offset);
> -    if (l2_table == NULL)
> -        return 0;
> -
> -    /* find the cluster offset for the given disk offset */
> -
> -    l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
> -    cluster_offset = be64_to_cpu(l2_table[l2_index]);
> -    nb_clusters = size_to_clusters(s, nb_needed << 9);
> -
> -    if (!cluster_offset) {
> -        /* how many empty clusters ? */
> -        c = count_contiguous_free_clusters(nb_clusters, &l2_table[l2_index]);
> -    } else {
> -        /* how many allocated clusters ? */
> -        c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> -                &l2_table[l2_index], 0, QCOW_OFLAG_COPIED);
> -    }
> -
> -   nb_available = (c * s->cluster_sectors);
> -out:
> -    if (nb_available > nb_needed)
> -        nb_available = nb_needed;
> -
> -    *num = nb_available - index_in_cluster;
> -
> -    return cluster_offset & ~QCOW_OFLAG_COPIED;
> -}
> -
> -/*
> - * free_any_clusters
> - *
> - * free clusters according to its type: compressed or not
> - *
> - */
> -
> -static void free_any_clusters(BlockDriverState *bs,
> -                              uint64_t cluster_offset, int nb_clusters)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -
> -    /* free the cluster */
> -
> -    if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
> -        int nb_csectors;
> -        nb_csectors = ((cluster_offset >> s->csize_shift) &
> -                       s->csize_mask) + 1;
> -        free_clusters(bs, (cluster_offset & s->cluster_offset_mask) & ~511,
> -                      nb_csectors * 512);
> -        return;
> -    }
> -
> -    free_clusters(bs, cluster_offset, nb_clusters << s->cluster_bits);
> -
> -    return;
> -}
> -
> -/*
> - * get_cluster_table
> - *
> - * for a given disk offset, load (and allocate if needed)
> - * the l2 table.
> - *
> - * the l2 table offset in the qcow2 file and the cluster index
> - * in the l2 table are given to the caller.
> - *
> - */
> -
> -static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
> -                             uint64_t **new_l2_table,
> -                             uint64_t *new_l2_offset,
> -                             int *new_l2_index)
> +                                   uint64_t offset, int allocate,
> +                                   int compressed_size,
> +                                   int n_start, int n_end)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int l1_index, l2_index, ret;
> -    uint64_t l2_offset, *l2_table;
> -
> -    /* seek the the l2 offset in the l1 table */
> +    int min_index, i, j, l1_index, l2_index, ret;
> +    uint64_t l2_offset, *l2_table, cluster_offset, tmp, old_l2_offset;
>  
>      l1_index = offset >> (s->l2_bits + s->cluster_bits);
>      if (l1_index >= s->l1_size) {
> -        ret = grow_l1_table(bs, l1_index + 1);
> -        if (ret < 0)
> +        /* outside l1 table is allowed: we grow the table if needed */
> +        if (!allocate)
> +            return 0;
> +        if (grow_l1_table(bs, l1_index + 1) < 0)
>              return 0;
>      }
>      l2_offset = s->l1_table[l1_index];
> +    if (!l2_offset) {
> +        if (!allocate)
> +            return 0;
> +    l2_allocate:
> +        old_l2_offset = l2_offset;
> +        /* allocate a new l2 entry */
> +        l2_offset = alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
> +        /* update the L1 entry */
> +        s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
> +        tmp = cpu_to_be64(l2_offset | QCOW_OFLAG_COPIED);
> +        if (bdrv_pwrite(s->hd, s->l1_table_offset + l1_index * sizeof(tmp),
> +                        &tmp, sizeof(tmp)) != sizeof(tmp))
> +            return 0;
> +        min_index = l2_cache_new_entry(bs);
> +        l2_table = s->l2_cache + (min_index << s->l2_bits);
>  
> -    /* seek the l2 table of the given l2 offset */
> -
> -    if (l2_offset & QCOW_OFLAG_COPIED) {
> -        /* load the l2 table in memory */
> -        l2_offset &= ~QCOW_OFLAG_COPIED;
> -        l2_table = l2_load(bs, l2_offset);
> -        if (l2_table == NULL)
> +        if (old_l2_offset == 0) {
> +            memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
> +        } else {
> +            if (bdrv_pread(s->hd, old_l2_offset,
> +                           l2_table, s->l2_size * sizeof(uint64_t)) !=
> +                s->l2_size * sizeof(uint64_t))
> +                return 0;
> +        }
> +        if (bdrv_pwrite(s->hd, l2_offset,
> +                        l2_table, s->l2_size * sizeof(uint64_t)) !=
> +            s->l2_size * sizeof(uint64_t))
>              return 0;
>      } else {
> -        if (l2_offset)
> -            free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
> -        l2_table = l2_allocate(bs, l1_index);
> -        if (l2_table == NULL)
> +        if (!(l2_offset & QCOW_OFLAG_COPIED)) {
> +            if (allocate) {
> +                free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
> +                goto l2_allocate;
> +            }
> +        } else {
> +            l2_offset &= ~QCOW_OFLAG_COPIED;
> +        }
> +        for(i = 0; i < L2_CACHE_SIZE; i++) {
> +            if (l2_offset == s->l2_cache_offsets[i]) {
> +                /* increment the hit count */
> +                if (++s->l2_cache_counts[i] == 0xffffffff) {
> +                    for(j = 0; j < L2_CACHE_SIZE; j++) {
> +                        s->l2_cache_counts[j] >>= 1;
> +                    }
> +                }
> +                l2_table = s->l2_cache + (i << s->l2_bits);
> +                goto found;
> +            }
> +        }
> +        /* not found: load a new entry in the least used one */
> +        min_index = l2_cache_new_entry(bs);
> +        l2_table = s->l2_cache + (min_index << s->l2_bits);
> +        if (bdrv_pread(s->hd, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
> +            s->l2_size * sizeof(uint64_t))
>              return 0;
> -        l2_offset = s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED;
>      }
> -
> -    /* find the cluster offset for the given disk offset */
> -
> +    s->l2_cache_offsets[min_index] = l2_offset;
> +    s->l2_cache_counts[min_index] = 1;
> + found:
>      l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
> -
> -    *new_l2_table = l2_table;
> -    *new_l2_offset = l2_offset;
> -    *new_l2_index = l2_index;
> -
> -    return 1;
> -}
> -
> -/*
> - * alloc_compressed_cluster_offset
> - *
> - * For a given offset of the disk image, return cluster offset in
> - * qcow2 file.
> - *
> - * If the offset is not found, allocate a new compressed cluster.
> - *
> - * Return the cluster offset if successful,
> - * Return 0, otherwise.
> - *
> - */
> -
> -static uint64_t alloc_compressed_cluster_offset(BlockDriverState *bs,
> -                                                uint64_t offset,
> -                                                int compressed_size)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int l2_index, ret;
> -    uint64_t l2_offset, *l2_table, cluster_offset;
> -    int nb_csectors;
> -
> -    ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
> -    if (ret == 0)
> -        return 0;
> -
>      cluster_offset = be64_to_cpu(l2_table[l2_index]);
> -    if (cluster_offset & QCOW_OFLAG_COPIED)
> -        return cluster_offset & ~QCOW_OFLAG_COPIED;
> -
> -    if (cluster_offset)
> -        free_any_clusters(bs, cluster_offset, 1);
> -
> -    cluster_offset = alloc_bytes(bs, compressed_size);
> -    nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
> -                  (cluster_offset >> 9);
> -
> -    cluster_offset |= QCOW_OFLAG_COMPRESSED |
> -                      ((uint64_t)nb_csectors << s->csize_shift);
> -
> -    /* update L2 table */
> -
> -    /* compressed clusters never have the copied flag */
> -
> -    l2_table[l2_index] = cpu_to_be64(cluster_offset);
> -    if (bdrv_pwrite(s->hd,
> -                    l2_offset + l2_index * sizeof(uint64_t),
> -                    l2_table + l2_index,
> -                    sizeof(uint64_t)) != sizeof(uint64_t))
> -        return 0;
> -
> -    return cluster_offset;
> -}
> -
> -typedef struct QCowL2Meta
> -{
> -    uint64_t offset;
> -    int n_start;
> -    int nb_available;
> -    int nb_clusters;
> -} QCowL2Meta;
> -
> -static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
> -        QCowL2Meta *m)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int i, j = 0, l2_index, ret;
> -    uint64_t *old_cluster, start_sect, l2_offset, *l2_table;
> -
> -    if (m->nb_clusters == 0)
> -        return 0;
> -
> -    if (!(old_cluster = qemu_malloc(m->nb_clusters * sizeof(uint64_t))))
> -        return -ENOMEM;
> -
> -    /* copy content of unmodified sectors */
> -    start_sect = (m->offset & ~(s->cluster_size - 1)) >> 9;
> -    if (m->n_start) {
> -        ret = copy_sectors(bs, start_sect, cluster_offset, 0, m->n_start);
> -        if (ret < 0)
> -            goto err;
> +    if (!cluster_offset) {
> +        if (!allocate)
> +            return cluster_offset;
> +    } else if (!(cluster_offset & QCOW_OFLAG_COPIED)) {
> +        if (!allocate)
> +            return cluster_offset;
> +        /* free the cluster */
> +        if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
> +            int nb_csectors;
> +            nb_csectors = ((cluster_offset >> s->csize_shift) &
> +                           s->csize_mask) + 1;
> +            free_clusters(bs, (cluster_offset & s->cluster_offset_mask) & ~511,
> +                          nb_csectors * 512);
> +        } else {
> +            free_clusters(bs, cluster_offset, s->cluster_size);
> +        }
> +    } else {
> +        cluster_offset &= ~QCOW_OFLAG_COPIED;
> +        return cluster_offset;
>      }
> -
> -    if (m->nb_available & (s->cluster_sectors - 1)) {
> -        uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1);
> -        ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9),
> -                m->nb_available - end, s->cluster_sectors);
> -        if (ret < 0)
> -            goto err;
> +    if (allocate == 1) {
> +        /* allocate a new cluster */
> +        cluster_offset = alloc_clusters(bs, s->cluster_size);
> +
> +        /* we must initialize the cluster content which won't be
> +           written */
> +        if ((n_end - n_start) < s->cluster_sectors) {
> +            uint64_t start_sect;
> +
> +            start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> +            ret = copy_sectors(bs, start_sect,
> +                               cluster_offset, 0, n_start);
> +            if (ret < 0)
> +                return 0;
> +            ret = copy_sectors(bs, start_sect,
> +                               cluster_offset, n_end, s->cluster_sectors);
> +            if (ret < 0)
> +                return 0;
> +        }
> +        tmp = cpu_to_be64(cluster_offset | QCOW_OFLAG_COPIED);
> +    } else {
> +        int nb_csectors;
> +        cluster_offset = alloc_bytes(bs, compressed_size);
> +        nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
> +            (cluster_offset >> 9);
> +        cluster_offset |= QCOW_OFLAG_COMPRESSED |
> +            ((uint64_t)nb_csectors << s->csize_shift);
> +        /* compressed clusters never have the copied flag */
> +        tmp = cpu_to_be64(cluster_offset);
>      }
> -
> -    ret = -EIO;
>      /* update L2 table */
> -    if (!get_cluster_table(bs, m->offset, &l2_table, &l2_offset, &l2_index))
> -        goto err;
> -
> -    for (i = 0; i < m->nb_clusters; i++) {
> -        if(l2_table[l2_index + i] != 0)
> -            old_cluster[j++] = l2_table[l2_index + i];
> -
> -        l2_table[l2_index + i] = cpu_to_be64((cluster_offset +
> -                    (i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
> -     }
> -
> -    if (bdrv_pwrite(s->hd, l2_offset + l2_index * sizeof(uint64_t),
> -                l2_table + l2_index, m->nb_clusters * sizeof(uint64_t)) !=
> -            m->nb_clusters * sizeof(uint64_t))
> -        goto err;
> -
> -    for (i = 0; i < j; i++)
> -        free_any_clusters(bs, old_cluster[i], 1);
> -
> -    ret = 0;
> -err:
> -    qemu_free(old_cluster);
> -    return ret;
> - }
> -
> -/*
> - * alloc_cluster_offset
> - *
> - * For a given offset of the disk image, return cluster offset in
> - * qcow2 file.
> - *
> - * If the offset is not found, allocate a new cluster.
> - *
> - * Return the cluster offset if successful,
> - * Return 0, otherwise.
> - *
> - */
> -
> -static uint64_t alloc_cluster_offset(BlockDriverState *bs,
> -                                     uint64_t offset,
> -                                     int n_start, int n_end,
> -                                     int *num, QCowL2Meta *m)
> -{
> -    BDRVQcowState *s = bs->opaque;
> -    int l2_index, ret;
> -    uint64_t l2_offset, *l2_table, cluster_offset;
> -    int nb_clusters, i = 0;
> -
> -    ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
> -    if (ret == 0)
> +    l2_table[l2_index] = tmp;
> +    if (bdrv_pwrite(s->hd,
> +                    l2_offset + l2_index * sizeof(tmp), &tmp, sizeof(tmp)) != sizeof(tmp))
>          return 0;
> -
> -    nb_clusters = size_to_clusters(s, n_end << 9);
> -
> -    nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
> -
> -    cluster_offset = be64_to_cpu(l2_table[l2_index]);
> -
> -    /* We keep all QCOW_OFLAG_COPIED clusters */
> -
> -    if (cluster_offset & QCOW_OFLAG_COPIED) {
> -        nb_clusters = count_contiguous_clusters(nb_clusters, s->cluster_size,
> -                &l2_table[l2_index], 0, 0);
> -
> -        cluster_offset &= ~QCOW_OFLAG_COPIED;
> -        m->nb_clusters = 0;
> -
> -        goto out;
> -    }
> -
> -    /* for the moment, multiple compressed clusters are not managed */
> -
> -    if (cluster_offset & QCOW_OFLAG_COMPRESSED)
> -        nb_clusters = 1;
> -
> -    /* how many available clusters ? */
> -
> -    while (i < nb_clusters) {
> -        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
> -                &l2_table[l2_index], i, 0);
> -
> -        if(be64_to_cpu(l2_table[l2_index + i]))
> -            break;
> -
> -        i += count_contiguous_free_clusters(nb_clusters - i,
> -                &l2_table[l2_index + i]);
> -
> -        cluster_offset = be64_to_cpu(l2_table[l2_index + i]);
> -
> -        if ((cluster_offset & QCOW_OFLAG_COPIED) ||
> -                (cluster_offset & QCOW_OFLAG_COMPRESSED))
> -            break;
> -    }
> -    nb_clusters = i;
> -
> -    /* allocate a new cluster */
> -
> -    cluster_offset = alloc_clusters(bs, nb_clusters * s->cluster_size);
> -
> -    /* save info needed for meta data update */
> -    m->offset = offset;
> -    m->n_start = n_start;
> -    m->nb_clusters = nb_clusters;
> -
> -out:
> -    m->nb_available = MIN(nb_clusters << (s->cluster_bits - 9), n_end);
> -
> -    *num = m->nb_available - n_start;
> -
>      return cluster_offset;
>  }
>  
>  static int qcow_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>  {
> +    BDRVQcowState *s = bs->opaque;
> +    int index_in_cluster, n;
>      uint64_t cluster_offset;
>  
> -    *pnum = nb_sectors;
> -    cluster_offset = get_cluster_offset(bs, sector_num << 9, pnum);
> -
> +    cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
> +    index_in_cluster = sector_num & (s->cluster_sectors - 1);
> +    n = s->cluster_sectors - index_in_cluster;
> +    if (n > nb_sectors)
> +        n = nb_sectors;
> +    *pnum = n;
>      return (cluster_offset != 0);
>  }
>  
> @@ -1102,9 +723,11 @@
>      uint64_t cluster_offset;
>  
>      while (nb_sectors > 0) {
> -        n = nb_sectors;
> -        cluster_offset = get_cluster_offset(bs, sector_num << 9, &n);
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> +        n = s->cluster_sectors - index_in_cluster;
> +        if (n > nb_sectors)
> +            n = nb_sectors;
>          if (!cluster_offset) {
>              if (bs->backing_hd) {
>                  /* read from the base image */
> @@ -1143,18 +766,15 @@
>      BDRVQcowState *s = bs->opaque;
>      int ret, index_in_cluster, n;
>      uint64_t cluster_offset;
> -    int n_end;
> -    QCowL2Meta l2meta;
>  
>      while (nb_sectors > 0) {
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n_end = index_in_cluster + nb_sectors;
> -        if (s->crypt_method &&
> -            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
> -            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> -        cluster_offset = alloc_cluster_offset(bs, sector_num << 9,
> -                                              index_in_cluster,
> -                                              n_end, &n, &l2meta);
> +        n = s->cluster_sectors - index_in_cluster;
> +        if (n > nb_sectors)
> +            n = nb_sectors;
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
> +                                            index_in_cluster,
> +                                            index_in_cluster + n);
>          if (!cluster_offset)
>              return -1;
>          if (s->crypt_method) {
> @@ -1165,10 +785,8 @@
>          } else {
>              ret = bdrv_pwrite(s->hd, cluster_offset + index_in_cluster * 512, buf, n * 512);
>          }
> -        if (ret != n * 512 || alloc_cluster_link_l2(bs, cluster_offset, &l2meta) < 0) {
> -            free_any_clusters(bs, cluster_offset, l2meta.nb_clusters);
> +        if (ret != n * 512)
>              return -1;
> -        }
>          nb_sectors -= n;
>          sector_num += n;
>          buf += n * 512;
> @@ -1186,33 +804,8 @@
>      uint64_t cluster_offset;
>      uint8_t *cluster_data;
>      BlockDriverAIOCB *hd_aiocb;
> -    QEMUBH *bh;
> -    QCowL2Meta l2meta;
>  } QCowAIOCB;
>  
> -static void qcow_aio_read_cb(void *opaque, int ret);
> -static void qcow_aio_read_bh(void *opaque)
> -{
> -    QCowAIOCB *acb = opaque;
> -    qemu_bh_delete(acb->bh);
> -    acb->bh = NULL;
> -    qcow_aio_read_cb(opaque, 0);
> -}
> -
> -static int qcow_schedule_bh(QEMUBHFunc *cb, QCowAIOCB *acb)
> -{
> -    if (acb->bh)
> -        return -EIO;
> -
> -    acb->bh = qemu_bh_new(cb, acb);
> -    if (!acb->bh)
> -        return -EIO;
> -
> -    qemu_bh_schedule(acb->bh);
> -
> -    return 0;
> -}
> -
>  static void qcow_aio_read_cb(void *opaque, int ret)
>  {
>      QCowAIOCB *acb = opaque;
> @@ -1222,12 +815,13 @@
>  
>      acb->hd_aiocb = NULL;
>      if (ret < 0) {
> -fail:
> +    fail:
>          acb->common.cb(acb->common.opaque, ret);
>          qemu_aio_release(acb);
>          return;
>      }
>  
> + redo:
>      /* post process the read buffer */
>      if (!acb->cluster_offset) {
>          /* nothing to do */
> @@ -1253,9 +847,12 @@
>      }
>  
>      /* prepare next AIO request */
> -    acb->n = acb->nb_sectors;
> -    acb->cluster_offset = get_cluster_offset(bs, acb->sector_num << 9, &acb->n);
> +    acb->cluster_offset = get_cluster_offset(bs, acb->sector_num << 9,
> +                                             0, 0, 0, 0);
>      index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
> +    acb->n = s->cluster_sectors - index_in_cluster;
> +    if (acb->n > acb->nb_sectors)
> +        acb->n = acb->nb_sectors;
>  
>      if (!acb->cluster_offset) {
>          if (bs->backing_hd) {
> @@ -1268,16 +865,12 @@
>                  if (acb->hd_aiocb == NULL)
>                      goto fail;
>              } else {
> -                ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
> -                if (ret < 0)
> -                    goto fail;
> +                goto redo;
>              }
>          } else {
>              /* Note: in this case, no need to wait */
>              memset(acb->buf, 0, 512 * acb->n);
> -            ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
> -            if (ret < 0)
> -                goto fail;
> +            goto redo;
>          }
>      } else if (acb->cluster_offset & QCOW_OFLAG_COMPRESSED) {
>          /* add AIO support for compressed blocks ? */
> @@ -1285,9 +878,7 @@
>              goto fail;
>          memcpy(acb->buf,
>                 s->cluster_cache + index_in_cluster * 512, 512 * acb->n);
> -        ret = qcow_schedule_bh(qcow_aio_read_bh, acb);
> -        if (ret < 0)
> -            goto fail;
> +        goto redo;
>      } else {
>          if ((acb->cluster_offset & 511) != 0) {
>              ret = -EIO;
> @@ -1316,7 +907,6 @@
>      acb->nb_sectors = nb_sectors;
>      acb->n = 0;
>      acb->cluster_offset = 0;
> -    acb->l2meta.nb_clusters = 0;
>      return acb;
>  }
>  
> @@ -1340,8 +930,8 @@
>      BlockDriverState *bs = acb->common.bs;
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
> +    uint64_t cluster_offset;
>      const uint8_t *src_buf;
> -    int n_end;
>  
>      acb->hd_aiocb = NULL;
>  
> @@ -1352,11 +942,6 @@
>          return;
>      }
>  
> -    if (alloc_cluster_link_l2(bs, acb->cluster_offset, &acb->l2meta) < 0) {
> -        free_any_clusters(bs, acb->cluster_offset, acb->l2meta.nb_clusters);
> -        goto fail;
> -    }
> -
>      acb->nb_sectors -= acb->n;
>      acb->sector_num += acb->n;
>      acb->buf += acb->n * 512;
> @@ -1369,22 +954,19 @@
>      }
>  
>      index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
> -    n_end = index_in_cluster + acb->nb_sectors;
> -    if (s->crypt_method &&
> -        n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
> -        n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> -
> -    acb->cluster_offset = alloc_cluster_offset(bs, acb->sector_num << 9,
> -                                          index_in_cluster,
> -                                          n_end, &acb->n, &acb->l2meta);
> -    if (!acb->cluster_offset || (acb->cluster_offset & 511) != 0) {
> +    acb->n = s->cluster_sectors - index_in_cluster;
> +    if (acb->n > acb->nb_sectors)
> +        acb->n = acb->nb_sectors;
> +    cluster_offset = get_cluster_offset(bs, acb->sector_num << 9, 1, 0,
> +                                        index_in_cluster,
> +                                        index_in_cluster + acb->n);
> +    if (!cluster_offset || (cluster_offset & 511) != 0) {
>          ret = -EIO;
>          goto fail;
>      }
>      if (s->crypt_method) {
>          if (!acb->cluster_data) {
> -            acb->cluster_data = qemu_mallocz(QCOW_MAX_CRYPT_CLUSTERS *
> -                                             s->cluster_size);
> +            acb->cluster_data = qemu_mallocz(s->cluster_size);
>              if (!acb->cluster_data) {
>                  ret = -ENOMEM;
>                  goto fail;
> @@ -1397,7 +979,7 @@
>          src_buf = acb->buf;
>      }
>      acb->hd_aiocb = bdrv_aio_write(s->hd,
> -                                   (acb->cluster_offset >> 9) + index_in_cluster,
> +                                   (cluster_offset >> 9) + index_in_cluster,
>                                     src_buf, acb->n,
>                                     qcow_aio_write_cb, acb);
>      if (acb->hd_aiocb == NULL)
> @@ -1571,7 +1153,7 @@
>  
>      memset(s->l1_table, 0, l1_length);
>      if (bdrv_pwrite(s->hd, s->l1_table_offset, s->l1_table, l1_length) < 0)
> -        return -1;
> +	return -1;
>      ret = bdrv_truncate(s->hd, s->l1_table_offset + l1_length);
>      if (ret < 0)
>          return ret;
> @@ -1637,10 +1219,8 @@
>          /* could not compress: write normal cluster */
>          qcow_write(bs, sector_num, buf, s->cluster_sectors);
>      } else {
> -        cluster_offset = alloc_compressed_cluster_offset(bs, sector_num << 9,
> -                                              out_len);
> -        if (!cluster_offset)
> -            return -1;
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 2,
> +                                            out_len, 0, 0);
>          cluster_offset &= s->cluster_offset_mask;
>          if (bdrv_pwrite(s->hd, cluster_offset, out_buf, out_len) != out_len) {
>              qemu_free(out_buf);
> @@ -2225,19 +1805,26 @@
>      BDRVQcowState *s = bs->opaque;
>      int i, nb_clusters;
>  
> -    nb_clusters = size_to_clusters(s, size);
> -retry:
> -    for(i = 0; i < nb_clusters; i++) {
> -        int64_t i = s->free_cluster_index++;
> -        if (get_refcount(bs, i) != 0)
> -            goto retry;
> -    }
> +    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
> +    for(;;) {
> +        if (get_refcount(bs, s->free_cluster_index) == 0) {
> +            s->free_cluster_index++;
> +            for(i = 1; i < nb_clusters; i++) {
> +                if (get_refcount(bs, s->free_cluster_index) != 0)
> +                    goto not_found;
> +                s->free_cluster_index++;
> +            }
>  #ifdef DEBUG_ALLOC2
> -    printf("alloc_clusters: size=%lld -> %lld\n",
> -            size,
> -            (s->free_cluster_index - nb_clusters) << s->cluster_bits);
> +            printf("alloc_clusters: size=%lld -> %lld\n",
> +                   size,
> +                   (s->free_cluster_index - nb_clusters) << s->cluster_bits);
>  #endif
> -    return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
> +            return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
> +        } else {
> +        not_found:
> +            s->free_cluster_index++;
> +        }
> +    }
>  }
>  
>  static int64_t alloc_clusters(BlockDriverState *bs, int64_t size)
> @@ -2301,7 +1888,8 @@
>      int new_table_size, new_table_size2, refcount_table_clusters, i, ret;
>      uint64_t *new_table;
>      int64_t table_offset;
> -    uint8_t data[12];
> +    uint64_t data64;
> +    uint32_t data32;
>      int old_table_size;
>      int64_t old_table_offset;
>  
> @@ -2340,10 +1928,13 @@
>      for(i = 0; i < s->refcount_table_size; i++)
>          be64_to_cpus(&new_table[i]);
>  
> -    cpu_to_be64w((uint64_t*)data, table_offset);
> -    cpu_to_be32w((uint32_t*)(data + 8), refcount_table_clusters);
> +    data64 = cpu_to_be64(table_offset);
>      if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
> -                    data, sizeof(data)) != sizeof(data))
> +                    &data64, sizeof(data64)) != sizeof(data64))
> +        goto fail;
> +    data32 = cpu_to_be32(refcount_table_clusters);
> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_clusters),
> +                    &data32, sizeof(data32)) != sizeof(data32))
>          goto fail;
>      qemu_free(s->refcount_table);
>      old_table_offset = s->refcount_table_offset;
> @@ -2572,7 +2163,7 @@
>      uint16_t *refcount_table;
>  
>      size = bdrv_getlength(s->hd);
> -    nb_clusters = size_to_clusters(s, size);
> +    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
>      refcount_table = qemu_mallocz(nb_clusters * sizeof(uint16_t));
>  
>      /* header */
> @@ -2624,7 +2215,7 @@
>      int refcount;
>  
>      size = bdrv_getlength(s->hd);
> -    nb_clusters = size_to_clusters(s, size);
> +    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
>      for(k = 0; k < nb_clusters;) {
>          k1 = k;
>          refcount = get_refcount(bs, k);
>
>
>   

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Jamie Lokier]

Anthony Liguori wrote:
> Well such a large reversion is a bad idea.  Can you git bisect to the 
> actual changeset that introduced the bug you see?

Have done, did you read the other thread?

Message-ID: <20090211114126.GC31997@shareable.org>
Subject: Re: qcow2 corruption observed, fixed by reverting old change

Jamie Lokier wrote:
> Kevin Wolf wrote:
> > Jamie Lokier schrieb:
> > > Although there are many ways to make Windows blue screen in KVM, in
> > > this case I've narrowed it down to the difference in
> > > qemu/block-qcow2.c between kvm-72 and kvm-73 (not -83).
> >
> > This must be one of SVN revisions 5003 to 5008 in upstream qemu. Can you
> > narrow it down to one of these? I certainly don't feel like reviewing
> > all of them once again.
> 
> It's QEMU SVN delta 5005-5006, copied below.

I don't have time to disentangle the different optimisations done to
qcow2 around that changeset, nor fix the changeset itself, but I can
test proposed patches on my guest VM image, which I've copied aside
because it's consistent about failing or not.

If nobody else has time either, then I think an imminent new QEMU
release, which may get rolled into distros and so on, is better off
with the the changes reverted than corrupting guest images.

I'm not proposing throwing away all the good work done on qcow2, only
that fixing observed corruption is important especially for a major
release, and reverting later changes can be temporary until the bug is
found and fixed.

-- Jamie

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Anthony Liguori]

On Sat, Feb 14, 2009 at 8:01 PM, Jamie Lokier <jamie@shareable.org> wrote:
> Have done, did you read the other thread?

Yes, but your patch confused me (which is admittedly not hard).

>> It's QEMU SVN delta 5005-5006, copied below.

So why such an aggressive revert?  Why not just revert the problematic
changesets?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Jamie Lokier]

Anthony Liguori wrote:
> On Sat, Feb 14, 2009 at 8:01 PM, Jamie Lokier <jamie@shareable.org> wrote:
> > Have done, did you read the other thread?
> 
> Yes, but your patch confused me (which is admittedly not hard).
> 
> >> It's QEMU SVN delta 5005-5006, copied below.
> 
> So why such an aggressive revert?  Why not just revert the problematic
> changesets?

Because most of the following changes look too dependent on it.

I did keep a couple of changes which are trivially independent since
that one - default to "cache=writeback" and eliminating #define
offsetof.

You have a point that QEMU SVN deltas up to 5005 don't need to be
reverted.  Reason for that: I simply don't have time to trim the patch
down to its bare essentials quickly, and being a corruption bug, it
should be dealt with quickly.  This one seems to work; feel free to
improve it by reverting less, or waiting a long time for me to do so :-)

-- Jamie

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>> On Sat, Feb 14, 2009 at 8:01 PM, Jamie Lokier <jamie@shareable.org> wrote:
>>     
>>> Have done, did you read the other thread?
>>>       
>> Yes, but your patch confused me (which is admittedly not hard).
>>
>>     
>>>> It's QEMU SVN delta 5005-5006, copied below.
>>>>         
>> So why such an aggressive revert?  Why not just revert the problematic
>> changesets?
>>     
>
> Because most of the following changes look too dependent on it.
>   

Too dependent on the introduced functionality or too dependent to make 
porting trivial?  My impression upon looking was that it's the later, 
not the former.  If that is the case, then someone needs to do the work 
of properly reverting.

> I did keep a couple of changes which are trivially independent since
> that one - default to "cache=writeback" and eliminating #define
> offsetof.
>
> You have a point that QEMU SVN deltas up to 5005 don't need to be
> reverted.  Reason for that: I simply don't have time to trim the patch
> down to its bare essentials quickly, and being a corruption bug, it
> should be dealt with quickly.  This one seems to work; feel free to
> improve it by reverting less, or waiting a long time for me to do so :-)
>   

But many of the changes since 5005 were also corruption fixes.  And 
let's be clear, your data is *not* safe with qcow2.  So I don't consider 
this to be a show stopping issue.

Regards,

Anthony Liguori

> -- Jamie
>
>
>   

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Anthony Liguori wrote:

> And let's be clear, your data is *not* safe with qcow2.  So I don't 
> consider this to be a show stopping issue.

I beg your pardon?  The one format that was recommended for quite a long 
time now is considered unsafe?

That would not have happened with Fabrice in charge,
Dscho

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Anthony Liguori]

Johannes Schindelin wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Anthony Liguori wrote:
>
>   
>> And let's be clear, your data is *not* safe with qcow2.  So I don't 
>> consider this to be a show stopping issue.
>>     
>
> I beg your pardon?  The one format that was recommended for quite a long 
> time now is considered unsafe?
>   

It's always been that way.  It's unsafe for a number of reasons that 
have been discussed at great length.

Regards,

Anthony LIguori

> That would not have happened with Fabrice in charge,
> Dscho
>
>   

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Jamie Lokier]

Anthony Liguori wrote:
> >>And let's be clear, your data is *not* safe with qcow2.  So I don't 
> >>consider this to be a show stopping issue.
> >
> >I beg your pardon?  The one format that was recommended for quite a long 
> >time now is considered unsafe?
> 
> It's always been that way.  It's unsafe for a number of reasons that 
> have been discussed at great length.

It sure isn't mentioned in the documentation.
If it was, I would never have used it, and I imagine I'm not alone.

QEMU might be an emulator project where people expect quirks, but KVM
and Xen are professional virtualisation platforms competing with
VMware.

It is really not very professional that the documentation places "your
data is not safe" formats on an equal footing with safe formats -
without saying anything about it - and doesn't even recommend one or
the other.

That said, maybe Microsoft is doing the same thing - their
documentation happily recommends their VHD format if you're not
concerned about running out of disk space, and it's maybe VHD has
similar corruption windows.

-- Jamie

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>>>> And let's be clear, your data is *not* safe with qcow2.  So I don't 
>>>> consider this to be a show stopping issue.
>>>>         
>>> I beg your pardon?  The one format that was recommended for quite a long 
>>> time now is considered unsafe?
>>>       
>> It's always been that way.  It's unsafe for a number of reasons that 
>> have been discussed at great length.
>>     
>
> It sure isn't mentioned in the documentation.
> If it was, I would never have used it, and I imagine I'm not alone.
>
> QEMU might be an emulator project where people expect quirks, but KVM
> and Xen are professional virtualisation platforms competing with
> VMware.
>
> It is really not very professional that the documentation places "your
> data is not safe" formats on an equal footing with safe formats -
> without saying anything about it - and doesn't even recommend one or
> the other.
>   

Please submit patches.  I don't disagree with you and that is why I'm 
trying to make this clear now.

> That said, maybe Microsoft is doing the same thing - their
> documentation happily recommends their VHD format if you're not
> concerned about running out of disk space, and it's maybe VHD has
> similar corruption windows.
>   

Yeah, it's hard to make a truly reliable format that isn't raw.  It 
basically is the same problem file systems solve and requires either a 
journal or an fsck step.  I'm thinking that this is a problem for other 
software too.

Regards,

Anthony Liguori

> -- Jamie
>
>
>   

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Anthony Liguori]

Johannes Schindelin wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Anthony Liguori wrote:
>
>   
>> And let's be clear, your data is *not* safe with qcow2.  So I don't 
>> consider this to be a show stopping issue.
>>     
>
> I beg your pardon?  The one format that was recommended for quite a long 
> time now is considered unsafe?
>   

Let me be abundantly clear here.  qcow2 has never been the "recommended" 
format IMHO.  When it was introduced, it was introduced as 
experimental.  This is why block-qcow2.c was forked instead of qcow2 
support being integrated into block-qcow.c.  There's a ton of duplicate 
code between the two.  The plan was to eventually merge the two once 
qcow2 stabilized.  That's not happened.  The code was never improved 
much since it's introduction until recently.

I have concerns about the fundamental design of qcow2.  I believe it 
will be difficult to make it safe while having acceptable performance.  
There has been some work recently by Gleb Natapov to reduce the 
corruption window in qcow2 but it's still there.

I can only recommend qcow2 to casual users.  It should *not* be used in 
production environments.  It pains me to say that because we don't have 
a good alternative but there's no way I could recommend its use.

In particular, Jamie's patch reverts one set of patches that reduces the 
corruption window to "fix" a corruption bug that is now being 
experience.  However, since we don't know exactly what the cause of the 
new bug is, it's not necessarily true that the revert fixes the bug.  It 
may just make it more difficult to expose.

There's really no winning scenario here except finding the root cause of 
the new bug and fixing it.  That still won't make qcow2 safe for 
production data though.  So as far as I'm concerned, until qcow2 is made 
completely safe, it's still an experimental feature.

Regards,

Anthony Liguori

> That would not have happened with Fabrice in charge,
> Dscho
>
>   

Re: [Qemu-devel] [PATCH] Revert block-qcow2.c to kvm-72 version due to corruption reports

[Jamie Lokier]

Anthony Liguori wrote:
> >>>>It's QEMU SVN delta 5005-5006, copied below.
> >>>>        
> >>So why such an aggressive revert?  Why not just revert the problematic
> >>changesets?
> >
> >Because most of the following changes look too dependent on it.
> >  
> 
> Too dependent on the introduced functionality or too dependent to make 
> porting trivial?  My impression upon looking was that it's the later, 

My impression is the former, in that it seems necessary to understand
the changes in 5006 to understand how to rewrite subsequent patches
which use the changed functions.

But I didn't spend a long time on it, as I can't.  Of course all such
things reduce to trivial porting if you have enough time.

> But many of the changes since 5005 were also corruption fixes.  And 
> let's be clear, your data is *not* safe with qcow2.  So I don't consider 
> this to be a show stopping issue.

There's a HUGE difference between "not safe if the host/QEMU crashes"
and "corrupts silently during normal operation with no errors".

The former is a rare event we hope.

Marc's report, based apparently on a big farm of VMs, is that he
observes this corruption a lot with Windows guests.

The scary thing is it looks like it doesn't have anything (directly)
to do with the device emulation, which is more sensitive to guest OS
type.  I wonder if people using kvm >= 73 have silent corruption in
their Linux guests without noticing yet.

-- Jamie