Revert "qcow2: Use bdrv_(p)write_sync for metadata writes"

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

I hope to get some more comments on how to proceed here for 0.13.

Sometimes an improvement has a side effect and it makes sense to hold back the improvement until the side effect can be resolved. The period of time in which users could rely on qcow2 data integrity is small to none, I feel reverting the commit makes sense.

QEMU 0.12.5 has qcow2 sync metadata writes in commit 37060c28e522843fbf6f7e59af745dfcb05b132c. Was the performance regression spotted on 0.12.5 or 0.13?

Quoted 14 lines

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

Quoted 1 line

I hope to get some more comments on how to proceed here for 0.13.

Sometimes an improvement has a side effect and it makes sense to hold back the improvement until the side effect can be resolved. The period of time in which users could rely on qcow2 data integrity is small to none, I feel reverting the commit makes sense.

QEMU 0.12.5 has qcow2 sync metadata writes in commit 37060c28e522843fbf6f7e59af745dfcb05b132c. Was the performance regression spotted on 0.12.5 or 0.13?

Am 24.08.2010 13:02, schrieb Stefan Hajnoczi:

Quoted 18 lines · Kevin Wolf

Quoted 17 lines

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

I hope to get some more comments on how to proceed here for 0.13.

Quoted 5 lines

Sometimes an improvement has a side effect and it makes sense to hold back the improvement until the side effect can be resolved. The period of time in which users could rely on qcow2 data integrity is small to none, I feel reverting the commit makes sense.

Right, that's the vague feeling I have, too.

Quoted 26 lines · Kevin Wolf

Am 24.08.2010 13:02, schrieb Stefan Hajnoczi:

Quoted 23 lines · Kevin Wolf

Quoted 11 lines

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Quoted 6 lines

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

I hope to get some more comments on how to proceed here for 0.13.

Sometimes an improvement has a side effect and it makes sense to hold back the improvement until the side effect can be resolved. The period of time in which users could rely on qcow2 data integrity is small to none, I feel reverting the commit makes sense.

Quoted 2 lines

Right, that's the vague feeling I have, too.

If we don't think of qcow2 as integer format, why don't we just default to cache=unsafe there then? That way you could keep all the syncs in place making it stable with cache=!unsafe, but the default for users would be fast albeit unsafe, which it already is.

Am 24.08.2010 13:56, schrieb Alexander Graf:

Quoted 31 lines

Quoted 29 lines · Kevin Wolf

Am 24.08.2010 13:02, schrieb Stefan Hajnoczi:

Quoted 26 lines · Kevin Wolf

Quoted 12 lines

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Quoted 7 lines

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

I hope to get some more comments on how to proceed here for 0.13.

Sometimes an improvement has a side effect and it makes sense to hold back the improvement until the side effect can be resolved. The period of time in which users could rely on qcow2 data integrity is small to none, I feel reverting the commit makes sense.

Quoted 8 lines

Quoted 3 lines

Right, that's the vague feeling I have, too.

If we don't think of qcow2 as integer format, why don't we just default to cache=unsafe there then? That way you could keep all the syncs in place making it stable with cache=!unsafe, but the default for users would be fast albeit unsafe, which it already is.

Well, safety is not boolean. Considering to make it mostly safe instead of completely safe because of the performance doesn't mean that we should make it completely unsafe.

That said, what we should do is changing the cache mode to unsafe in certain places in qemu-img, e.g. in convert for the destination image. If it fails, you'll throw it away anyway.

Quoted 5 lines

Well, safety is not boolean. Considering to make it mostly safe instead of completely safe because of the performance doesn't mean that we should make it completely unsafe.

What is safety then? A vague feeling of "oh today is monday so my data is safe, but on tuesday I always lose my image data"? Either we promise to keep data safe or we don't. There is no in between.

Quoted 5 lines · Alexander Graf

Quoted 5 lines

Well, safety is not boolean. Considering to make it mostly safe instead of completely safe because of the performance doesn't mean that we should make it completely unsafe.

Quoted 4 lines

What is safety then? A vague feeling of "oh today is monday so my data is safe, but on tuesday I always lose my image data"? Either we promise to keep data safe or we don't. There is no in between.

Do you drive a car?

Though in general I agree we shouldn't compromise on data integrity.

Quoted 5 lines · Avi Kivity

Quoted 4 lines · Alexander Graf

Quoted 4 lines

Well, safety is not boolean. Considering to make it mostly safe instead of completely safe because of the performance doesn't mean that we should make it completely unsafe.

Quoted 5 lines

Quoted 4 lines

What is safety then? A vague feeling of "oh today is monday so my data is safe, but on tuesday I always lose my image data"? Either we promise to keep data safe or we don't. There is no in between.

Do you drive a car?

Would you buy a car where the breaks are known to not always work? ;)

Quoted 1 line

Though in general I agree we shouldn't compromise on data integrity.

That's my point. Either we go for it or we don't.

Quoted 6 lines

Quoted 5 lines · Avi Kivity

Quoted 4 lines · Alexander Graf

Quoted 4 lines

Well, safety is not boolean. Considering to make it mostly safe instead of completely safe because of the performance doesn't mean that we should make it completely unsafe.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

What is safety then? A vague feeling of "oh today is monday so my data is safe, but on tuesday I always lose my image data"? Either we promise to keep data safe or we don't. There is no in between.

Do you drive a car?

Would you buy a car where the breaks are known to not always work? ;)

That's not the case, even with cache=unsafe.

Quoted 2 lines

Quoted 1 line

Though in general I agree we shouldn't compromise on data integrity.

That's my point. Either we go for it or we don't.

I don't know how bad the performance regression is, and how large the integrity risk is. I'd default towards preserving integrity, but maybe this situation is different.

Quoted 4 lines

I don't know how bad the performance regression is, and how large the integrity risk is. I'd default towards preserving integrity, but maybe this situation is different.

I have reports of installations taking like 50 min instead of 14 min. My own qemu-io based test goes up from 1 s to 23 s. And I think the winner is Michael's image conversion which went up from 30 s to 49 min.

So it's not like we're talking about just some 10 or 20 percent.

Quoted 4 lines

Quoted 4 lines

I don't know how bad the performance regression is, and how large the integrity risk is. I'd default towards preserving integrity, but maybe this situation is different.

Quoted 5 lines

I have reports of installations taking like 50 min instead of 14 min. My own qemu-io based test goes up from 1 s to 23 s. And I think the winner is Michael's image conversion which went up from 30 s to 49 min.

So it's not like we're talking about just some 10 or 20 percent.

Image conversion should be done with cache=unsafe (with an fsync at the

end to make sure a guest isn't launched with volatile data in the cache[1]). The io test isn't a user workload.

The 14 min -> 50 min regression is pretty bad, but I'm not sure it's bad enough to merit risking user data.

[1] or better, when launching a guest with cache!=unsafe start with an fsync to make sure it's on disk.

Am 24.08.2010 13:02, schrieb Stefan Hajnoczi:

Quoted 3 lines

QEMU 0.12.5 has qcow2 sync metadata writes in commit 37060c28e522843fbf6f7e59af745dfcb05b132c.  Was the performance regression spotted on 0.12.5 or 0.13?

Both. You mean we should consider a 0.12.6 if we decide to revert? I think so far 0.12.5 was planned to be last 0.12.x release.

Quoted 7 lines · Kevin Wolf

Am 24.08.2010 13:02, schrieb Stefan Hajnoczi:

Quoted 3 lines

QEMU 0.12.5 has qcow2 sync metadata writes in commit 37060c28e522843fbf6f7e59af745dfcb05b132c. Was the performance regression spotted on 0.12.5 or 0.13?

Both. You mean we should consider a 0.12.6 if we decide to revert? I think so far 0.12.5 was planned to be last 0.12.x release.

Yes, especially if distros will revert the patches manually without an upstream release. I can see arguments for either way though.

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

I hope to get some more comments on how to proceed here for 0.13.

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

I hope to get some more comments on how to proceed here for 0.13.

Kevin

Quoted 14 lines

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

Quoted 2 lines

I hope to get some more comments on how to proceed here for 0.13.

How fundamental of an issue is this? Is this something we think we know how to fix and we just don't think there's time to fix it for 0.13?

And with this fix in place, how much confidence do we have in qcow2 with respect to data integrity on power loss?

We've shipped every version of QEMU since qcow2 was introduced with known data corruptions. It sucks big time. I think it's either that building an image format is a really hard problem akin to making a file system and we shouldn't be in that business or that qcow2 is bad as an image format which makes this all harder than it should be.

Am 24.08.2010 15:01, schrieb Anthony Liguori:

Quoted 16 lines · Kevin Wolf

Quoted 15 lines

This reverts commit 8b3b720620a1137a1b794fc3ed64734236f94e06.

This fix has caused severe slowdowns on recent kernels that actually do flush when they are told so. Reverting this patch hurts correctness and means that we could get corrupted images in case of a host crash. This means that qcow2 might not be an option for some people without this fix. On the other hand, I get reports that the slowdown is so massive that not reverting it would mean that people can't use it either because it just takes ages to complete stuff. It probably can be fixed, but not in time for 0.13.0.

Usually, if there's a possible tradeoff between correctness and performance, I tend to choose correctness, but I'm not so sure in this case. I'm not sure with reverting either, which is why I post this as an RFC only.

Quoted 6 lines

Quoted 3 lines

I hope to get some more comments on how to proceed here for 0.13.

How fundamental of an issue is this? Is this something we think we know how to fix and we just don't think there's time to fix it for 0.13?

I think we can improve things basically by trying to batch metadata writes and do them in parallel while already processing the next requests.

I'm not sure what the numbers are going to look like with something like this in place, I need to try it. It's definitely not something that I want to go into 0.13 at this point.

Quoted 3 lines

And with this fix in place, how much confidence do we have in qcow2 with respect to data integrity on power loss?

How do you measure confidence? :-)

There are power failure tests and I don't have any bugs open to that respect. I'm not sure how intensively it's tested, though.

Quoted 6 lines

We've shipped every version of QEMU since qcow2 was introduced with known data corruptions. It sucks big time. I think it's either that building an image format is a really hard problem akin to making a file system and we shouldn't be in that business or that qcow2 is bad as an image format which makes this all harder than it should be.

I tend to say that it's just hard to get right. Most of the problems that were fixed in qcow2 over the last year are probably present in our VMDK implementation as well, just to pick one example.

Kevin

I'm not sure this patch is needed in the first place.

If you have a sequence of operations like:

0) receive guest write request Z
1) submit write A
2) write A completes
3) submit write B
4) write B completes
5) report guest write Z complete

You're adding a:

4.5) sync write B

Which is ultimately unnecessary if what you care about is avoiding reordering of step (2) and (4). When a write() request completes, you're guaranteed that a subsequent read() request will return the written data. That's always true. If I could do a write(A) followed by a write(B) and then read()=A, no software would actually function correctly.

It's important to make sure that you don't get image corruption if (2) happens but not (4). But I think that's okay in qcow2 today.

Quoted 14 lines · Anthony Liguori

I'm not sure this patch is needed in the first place.

If you have a sequence of operations like:

0) receive guest write request Z
1) submit write A
2) write A completes
3) submit write B
4) write B completes
5) report guest write Z complete

You're adding a:

Quoted 5 lines

4.5) sync write B

Which is ultimately unnecessary if what you care about is avoiding reordering of step (2) and (4). When a write() request completes, you're guaranteed that a subsequent read() request will return the written data. That's always true. If I could do a write(A) followed by a write(B) and then read()=A, no software would actually function correctly.

It's important to make sure that you don't get image corruption if (2) happens but not (4). But I think that's okay in qcow2 today.

It's about metadata writes. If an operation changes metadata, we must sync it to disk before writing any data or other metadata which depends on it, regardless of any promises to the guest.

Quoted 1 line

It's about metadata writes. If an operation changes metadata, we must sync it to disk before writing any data or other metadata which depends on it, regardless of any promises to the guest.

Why? If the metadata isn't sync, we loose the write.

But that can happen anyway because we're not sync'ing the data

We need to sync the metadata in the event of a guest initiated flush, but we shouldn't need to for a normal write.

Quoted 8 lines · Anthony Liguori

Quoted 1 line

It's about metadata writes. If an operation changes metadata, we must sync it to disk before writing any data or other metadata which depends on it, regardless of any promises to the guest.

Why? If the metadata isn't sync, we loose the write.

But that can happen anyway because we're not sync'ing the data

We need to sync the metadata in the event of a guest initiated flush, but we shouldn't need to for a normal write.

1. Allocate a cluster (increase refcount table)

2. Link cluster to L2 table

3. Second operation makes it to disk; first still in pagecache

4. Crash

5. Dangling pointer from L2 to freed cluster

Quoted 6 lines

1. Allocate a cluster (increase refcount table)

2. Link cluster to L2 table

3. Second operation makes it to disk; first still in pagecache

Quoted 3 lines

4. Crash

5. Dangling pointer from L2 to freed cluster

Yes, having this discussion in IRC.

The problem is that we maintain a refcount table.

If we didn't do internal disk snapshots, we wouldn't have this problem. IOW, VMDK doesn't have this problem so the answer to my very first question is that qcow2 is too difficult a format to get right.

Quoted 8 lines · Anthony Liguori

Quoted 8 lines

1. Allocate a cluster (increase refcount table)

2. Link cluster to L2 table

3. Second operation makes it to disk; first still in pagecache

4. Crash

Quoted 6 lines

Quoted 1 line

5. Dangling pointer from L2 to freed cluster

Yes, having this discussion in IRC.

The problem is that we maintain a refcount table.

Are you sure that's the only issue?

Quoted 1 line

If we didn't do internal disk snapshots, we wouldn't have this problem. IOW, VMDK doesn't have this problem so the answer to my very first question is that qcow2 is too difficult a format to get right.

One doesn't follow from the other (though I'm no fan of internal snapshots, myself).

Quoted 1 line

One doesn't follow from the other (though I'm no fan of internal snapshots, myself).

It does. Let's consider the failure scenarios:

1) guest submits write request
2) allocate extent
3) write data to disk (a)
4) write (a) completes
5) update reference count table for new extent (b)
6) write (b) completes
7) write extent table (c)
8) write (c) completes
9) complete guest write request

If this all happened in order and we lost power, the worst case error is that we leak a block which isn't terrible.

But we're not guaranteed that this happens in order.

If (b) or (c) happen before (a), then the image is not corrupted but data gets lost. That's okay because it's part of the guest contract.

If (c) happens before (b), then we've created an extent that's attached to a table with a zero reference count. This is a corrupt image.

Let's consider if we eliminate the reference count table which means eliminating internal snapshots.

1) guest submits write request
2) allocate extent
3) write data to disk (a)
4) write (a) completes
5) write extent table (c)
6) write (c) completes
7) complete guest write request

If this all happens in order and we lose power, we just leak a block. It means we need a periodic fsck.

If (c) completes before (a), then it means that the image is not corrupted but data gets lost. This is okay based on the guest contract.

And that's it. There is no scenario where the disk is corrupted.

Quoted 2 lines

If (c) happens before (b), then we've created an extent that's attached to a table with a zero reference count. This is a corrupt image.

If the only issue is new block allocation, it can be easily solved.

Instead of allocating exactly the needed amount of blocks, allocate
a large extent and hold them in memory.

The next allocation can then be filled from memory, so the
allocation sync is amortized over many blocks. A power fail will leak the preallocated blocks, losing some megabytes of address space, but not real disk space.

Quoted 10 lines

Let's consider if we eliminate the reference count table which means eliminating internal snapshots.

1) guest submits write request
2) allocate extent
3) write data to disk (a)
4) write (a) completes
5) write extent table (c)
6) write (c) completes
7) complete guest write request

Quoted 5 lines

If this all happens in order and we lose power, we just leak a block. It means we need a periodic fsck.

If (c) completes before (a), then it means that the image is not corrupted but data gets lost. This is okay based on the guest contract.

And that's it. There is no scenario where the disk is corrupted.

_if_ that's the only failure mode.

Quoted 4 lines · Avi Kivity

Quoted 2 lines

If (c) happens before (b), then we've created an extent that's attached to a table with a zero reference count. This is a corrupt image.

Quoted 1 line

If the only issue is new block allocation, it can be easily solved.

Technically, I believe there are similar issues around creating snapshots but I don't think we care.

Quoted 2 lines

Instead of allocating exactly the needed amount of blocks, allocate
a large extent and hold them in memory.

So you're suggesting that we allocate a bunch of blocks, update the ref count table so that they are seen as allocated even though they aren't attached to an l1 table?

Quoted 3 lines

The next allocation can then be filled from memory, so the
allocation sync is amortized over many blocks. A power fail will leak the preallocated blocks, losing some megabytes of address space, but not real disk space.

It's a clever idea, but it would lose real disk space which is probably not a huge issue.

Quoted 4 lines

Quoted 2 lines

And that's it. There is no scenario where the disk is corrupted.

_if_ that's the only failure mode.

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

Quoted 5 lines

Quoted 3 lines

The next allocation can then be filled from memory, so the
allocation sync is amortized over many blocks. A power fail will leak the preallocated blocks, losing some megabytes of address space, but not real disk space.

It's a clever idea, but it would lose real disk space which is probably not a huge issue.

Not real disk space since no pwrite() would ever touch the disk. If the image were copied, _then_ we'd lose the disk space, if the copy command and filesystem don't optimize zeros away. \

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

And that's it. There is no scenario where the disk is corrupted.

_if_ that's the only failure mode.

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

I can't think of one, but that's not saying much.

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

Quoted 2 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

You can do this in one of two ways. You can do online compaction or you can maintain a free list. Online compaction has an advantage because it does not require any operations in the fast path whereas a free list would require ordered metadata updates (must remove something from the first list before updating the l2 table) which implies a sync.

At a high level, I don't think online compaction requires any specific support from an image format.

Quoted 8 lines · Anthony Liguori

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

You can do this in one of two ways. You can do online compaction or you can maintain a free list. Online compaction has an advantage because it does not require any operations in the fast path whereas a free list would require ordered metadata updates (must remove something from the first list before updating the l2 table) which implies a sync.

DISCARD/TRIM can queue blocks to the same preallocated block list we have to optimize allocation. New
allocations can come from this list, if it grows too large we sync part of it to disk to avoid loss of a lot of free space on power fail.

Quoted 2 lines

At a high level, I don't think online compaction requires any specific support from an image format.

You need to know that the block is free and can be reallocated.

Quoted 5 lines

Quoted 3 lines

At a high level, I don't think online compaction requires any specific support from an image format.

You need to know that the block is free and can be reallocated.

Semantically, TRIM/DISCARD means that "I don't care about the contents of the block anymore until I do another write." Behind the scenes, we can keep track of which blocks have been discarded in an in-memory list whereas the first write to the block causes it to be evicted from the discarded list.

A background task would attempt to detect idle I/O and copy a block from the end of the file to a location on the discarded list. When the copy has completed, you can then remove the L2 entry for the discarded block (effectively punching a hole in the image), sync, and then update the l2 entry for the block at the end of file location to point to the new block location. You can then ftruncate to reduce overall file size.

If you tried to maintain a free list, then you would need to sync on TRIM/DISCARD which is potentially a fast path. While a background task may be less efficient in the short term, it's just as efficient in the long term and it has the advantage of keeping any fast path fast.

Quoted 3 lines

If you tried to maintain a free list, then you would need to sync on TRIM/DISCARD which is potentially a fast path. While a background task may be less efficient in the short term, it's just as efficient in the long term and it has the advantage of keeping any fast path fast.

You only need to sync when the free list size grows beyond the amount of space you're prepared to lose on power fail. And you may be able to defer the background task indefinitely by satisfying new allocations from the free list.

Quoted 5 lines · Avi Kivity

Quoted 3 lines

If you tried to maintain a free list, then you would need to sync on TRIM/DISCARD which is potentially a fast path. While a background task may be less efficient in the short term, it's just as efficient in the long term and it has the advantage of keeping any fast path fast.

You only need to sync when the free list size grows beyond the amount of space you're prepared to lose on power fail. And you may be able to defer the background task indefinitely by satisfying new allocations from the free list.

Free does not mean free. If you immediately punch a hole in the l2 without doing a sync, then you're never sure whether the hole is there on disk or not. So if you then allocate that block and put it somewhere else in another l2 table, you need to sync the previous l2 change before you update the new l2.

Otherwise you can have two l2 entries pointing to the same block after a power failure. That's not a leak, that's a data corruption.

Quoted 7 lines · Anthony Liguori

Quoted 5 lines · Avi Kivity

Quoted 3 lines

If you tried to maintain a free list, then you would need to sync on TRIM/DISCARD which is potentially a fast path. While a background task may be less efficient in the short term, it's just as efficient in the long term and it has the advantage of keeping any fast path fast.

You only need to sync when the free list size grows beyond the amount of space you're prepared to lose on power fail. And you may be able to defer the background task indefinitely by satisfying new allocations from the free list.

Quoted 3 lines

Free does not mean free. If you immediately punch a hole in the l2 without doing a sync, then you're never sure whether the hole is there on disk or not. So if you then allocate that block and put it somewhere else in another l2 table, you need to sync the previous l2 change before you update the new l2.

Otherwise you can have two l2 entries pointing to the same block after a power failure. That's not a leak, that's a data corruption.

L2 certainly needs to be updated before the block is reused. But that's not different from a file format without a free list.

The batching I was referring to was only for free list management, same as the allocation issue which started this thread.

Quoted 2 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

BTW, something that had the features of qcow2 that people actually used andwas fully asynchronous, performed well, and had a high degree of confidence in data integrity would be a major step forward, not backwards.

BTW, something that had the features of qcow2 that people actually used andwas fully asynchronous, performed well, and had a high degree of confidence in data integrity would be a major step forward, not backwards.

Quoted 3 lines · Anthony Liguori

BTW, something that had the features of qcow2 that people actually used andwas fully asynchronous, performed well, and had a high degree of confidence in data integrity would be a major step forward, not backwards.

That means you'll actually need to agree on a feature set first.

Quoted 3 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

Quoted 5 lines · Avi Kivity

Quoted 3 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

Quoted 6 lines

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

Together with file level snapshots Thin Provisioning support basically makes qcow2 obsolete.

I already have a prototype implementation of scsi discard for qemu, which together with the XFS extent size hints allows us to very efficiently using growing and shrinking images on the bare filesystem. XFS at this point doesn't have file-level snapshots, but I'm planning to port the hole punhing and extent size hints to btrfs and maybe ocfs2 which will give you all that.

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Avi Kivity

Quoted 3 lines · Anthony Liguori

If we had another disk format that only supported growth and metadata for a backing file, can you think of another failure scenario?

btw, only supporting growth is a step backwards. Currently file-backed disks keep growing even the guest-used storage doesn't grow, since once we allocate something we never release it. But eventually guests will start using TRIM or DISCARD or however it's called, and then we can expose it and reclaim unused blocks.

Quoted 3 lines

Together with file level snapshots Thin Provisioning support basically makes qcow2 obsolete.

I think we'll always need a sparse image format. As long as people copy images to USB thumb drives containing vfat file systems or over dumb transports like HTTP, it will be necessary.