On 26.10.19 23:25, Alberto Garcia wrote: > Hi, > > here's the new version of the patches to add subcluster allocation > support to qcow2. > > Please refer to the cover letter of the first version for a full > description of the patches: > > https://lists.gnu.org/archive/html/qemu-block/2019-10/msg00983.html > > This version includes a few tests, but I'm planning to add more for > the next revision.
I think what would help most with testing is if it were possible to simply run the iotests with -o extended_l2=on. In general, the RFC looks OK to me. The one thing I dislike is that I feel that it is a bit, well, uncourageous. Now, after looking at the series, I don’t know whether you really changed everything that needs to be changed so it can deal with subclusters. To me it feels like this is because you tried to keep everything as it is and only do minimal changes. That is usually a good thing, but here I don’t know, because this way we can’t simply grep for places that need fixing (because they use /\<cluster/ instead of /subcluster/). To me it feels like with subclusters, the whole design should be different. Note that the following is just a very naïve idea, but anyway: I feel like what we need to separate isn’t L2 entries vs. clusters vs. subclusters (so a separation based on, well, syntax?) but a separation based on offset vs. allocation status (so a separation based on semantics). So I imagine there would be one function that sets a whole cluster’s (i.e., a group of subclusters) allocation offset; and another function that sets individual subclusters’ allocation status. Reversely, there should be a function to query a cluster’s/subcluster’s allocation offset, and another to query a subcluster’s type. To me it looks like the places where QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER is handled separately from QCOW2_CLUSTER_UNALLOCATED are places where we’re really not interested in the subcluster’s type at all, but just whether there’s an allocation or not. This is the case in qcow2_get_cluster_offset(), calculate_l2_meta(), and qcow2_co_block_status(). (These places should then use the function to query the allocation offset and evaluate the result instead of querying the subcluster type.) Does that sound in any way acceptable to you? You have more insight into this now and so maybe you know already that it can’t work. (Or maybe it’s just too invasive.) Right now, all I can do is grep for QCOW2_CLUSTER_ and set_l2_entry(). And all of those places look OK to me. But I just can’t shake off the uneasy feeling of not being able to really know whether this series really got to all the places that need adjustment. Max
signature.asc
Description: OpenPGP digital signature