Re: [PATCH v4 18/30] qcow2: Add subcluster support to qcow2_get_host_offset()

[Vladimir Sementsov-Ogievskiy]

22.04.2020 14:54, Alberto Garcia wrote:
> On Wed 22 Apr 2020 10:07:30 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
> > > +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
> > > +                                        unsigned sc_index, uint64_t *l2_slice,
> > > +                                        int l2_index)
> > >    {
> > >        BDRVQcow2State *s = bs->opaque;
> >
> > preexist, but, worth asserting that nb_clusters are all in this
> > l2_slice?
>
> Ok.
>
> > > +        for (j = (i == 0) ? sc_index : 0; j < s->subclusters_per_cluster; j++) 
> > > {
> > > +            if (qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, j) != type) 
> > > {
> > > +                goto out;
> >
> > why not just return count from here? And then you don't need goto at
> > all. Hmm, may be out: code will be extended in further patches..
>
> It's not extended in further patches. I generally prefer having a single
> exit point but you're right that it probably doesn't make sense here.
>
> > >            /* Compressed clusters can only be processed one by one */
> > > -        c = 1;
> > > +        sc = s->subclusters_per_cluster - sc_index;
> >
> > should not we assert here that sc_index == 0? Otherwise the caller
> > definitely doing something wrong.
>
> No, no, the guest offset doesn't need to be cluster aligned so sc_index
> can perfectly be != 0.

Hmm. yes. The only caller actually doesn't call count_contiguous_subclusters 
for compressed cluster case, but it may be refactored to do so, and then it does

  bytes_available = ((int64_t)sc + sc_index) << s->subcluster_bits;

so, even if intermediate sc is not very meaningful for compressed clusters (as 
we can't access sub-chunk of compressed cluster in any way), the resulting 
bytes_available is meaningful and it rely on sc being exactly what it is..

Ok

> > > +    case QCOW2_SUBCLUSTER_ZERO_ALLOC:
> > > +    case QCOW2_SUBCLUSTER_NORMAL:
> > > +    case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
> > > +        sc = count_contiguous_subclusters(bs, nb_clusters, sc_index,
> > > +                                          l2_slice, l2_index);
> > >            *host_offset = l2_entry & L2E_OFFSET_MASK;
> > >            if (offset_into_cluster(s, *host_offset)) {
> >
> > Hmm, you may move "sc = count_contiguous_subclusters" to be after the
> > switch-block, as it is universal now. And keep only offset calculation
> > and error checking in the switch-block.
>
> That's actually a good idea, thanks !! (plus we actually get to use the
> QCOW2_SUBCLUSTER_COMPRESSED check in count_contiguous_subclusters(),
> which is currently dead code).
>
> Berto


--
Best regards,
Vladimir