Am 21.03.2018 um 15:10 hat Alberto Garcia geschrieben: > On Wed 21 Mar 2018 02:52:38 PM CET, Kevin Wolf wrote: > > >> qemu-img create -f qcow2 -o cluster_size=512 hd.qcow2 1M > >> qemu-io -c 'write 0 124k' hd.qcow2 > >> qemu-io -c 'write 124k 512' hd.qcow2 > > > > So if I understand correctly, this is what we get: > > > > 0x20000: free > > 0x20200: refcount block > > 0x20400: data cluster > > 0x20600: potential next data cluster > > Yes. > > >> What this patch does is reset s->free_cluster_index to its previous > >> value when alloc_refcount_block() returns -EAGAIN. This way the caller > >> will try to allocate again the original clusters if they are still > >> free. > > > > This is an improvement, because now we should avoid the unused cluster: > > > > 0x20000: data cluster > > 0x20200: refcount block > > 0x20400: potential next data cluster > > That's correct. > > > But now the data clusters are fragmented. Should we try to change the > > logic so that already the refcount block allocation can make use of > > the free space? That is: > > > > 0x20000: refcount block > > 0x20200: data cluster > > 0x20400: contiguous potential next data cluster > > Well, the clusters before 0x20000 are also data clusters so there's > going to be fragmentation anyway. Also, if you're writing more than 1 > data cluster in a single request when the refcount block allocation > happens all those new data clusters are still going to be contiguous > (before the new refcount block).
That's true. I just remembered that when I looked at an image recently, I noticed that the refcount block wasn't in the first cluster and I disliked it, though mostly because it felt untidy rather than being a problem. There was one effect that might count as an advantage, though: The first few data clusters covered by the refcount block were corrupted before the overlap check for the refcount block stopped it. Not necessarily worth changing the logic, I just thought I'd bring it up and see whether anyone else dislikes it. > Another possibility that I considered was to make alloc_clusters_noref() > return the offset but let free_cluster_index untouched until the > refcounts are correct, but this requires more code and I fear that > keeping free_cluster_index pointing to the clusters we're trying to > allocate can lead to unexpected surprises. Possibly. I think if I wanted to change the order, I'd consider returning a hard error from update_refcount() when no refcount block is available, and then the caller would have to call alloc_refcount_block() manually before retrying update_refcount(). > The solution I chose is -I think- the simplest and easiest to audit. > > On a related note, I'm using a script that I wrote myself to debug qcow2 > images. It prints the contents of the header and lists all host > clusters, indicating the type of each one. I find it useful to debug > problems like this one. If there's nothing similar already existing I > can post it and we could put it in the scripts/ directory. Having a qcow2 analysis script in the repo sounds like a good idea. John has something, too. Maybe we can check whether the two things complement each other and then check in a script that combines both (or if one provides a superset of the other, just check in that one). Kevin
