Re: [PATCH v4 11/30] qcow2: Add l2_entry_size()

[Vladimir Sementsov-Ogievskiy]

14.04.2020 15:20, Alberto Garcia wrote:
> On Tue 14 Apr 2020 11:44:57 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
> > >        /* allocate a new l2 entry */
> > >    
> > > -    l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
> > > +    l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s));
> >
> > hmm. s->l2_size * l2_entry_size, isn't it just s->cluster_size always?
> > Maybe, just refactor these things?
>
> I think the patch is simpler to follow if I only do the strictly
> necessary changes and don't mix them with other things.
>
> > >            nb_new_l2_tables = DIV_ROUND_UP(nb_new_data_clusters,
> > > -                                        s->cluster_size / sizeof(uint64_t));
> > > +                                        s->cluster_size / l2_entry_size(s));
> >
> > Isn't it just s->l2_size ?
>
> Yes, same as before.
>
> > >            /* The cluster range may not be aligned to L2 boundaries, so add one 
> > > L2
> > >             * table for a potential head/tail */
> > >            nb_new_l2_tables++;
> >
> > Conversions looks correct, but how to check that we have converted
> > everything?
>
> I went through all cases, I think I didn't miss any!
>
> > I found this not converted chunk:
> >
> >       /* total size of L2 tables */
> >       nl2e = aligned_total_size / cluster_size;
> >       nl2e = ROUND_UP(nl2e, cluster_size / sizeof(uint64_t));
> >       meta_size += nl2e * sizeof(uint64_t);
>
> This is used by qcow2_measure() and is fixed on a later patch because,
> unlike all other cases, it does not use a BlockDriverState to determine
> the size of an L2 entry.
>
> > Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all
> > sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE,
> > REFTABLE_ENTRY_SIZE etc?
>
> That wouldn't be a bad thing I guess but, again, for a separate patch or
> series.
>
> > And all occurrences of pure '8' (not many of them exist)
>
> I think most/all nowadays only refer to the number of bits per byte.
>
> Maybe there's a couple that still need to be fixed, but we have been
> removing a lot of numeric literals from the qcow2 code (see for example
> b6c246942b, 3afea40243 or a35f87f50d).


git grep '\<8\>' block/qcow2*

shows at least

qcow2-cluster.c:            s->l1_table_offset + 8 * l1_start_index, bufsize, 
false);
qcow2-cluster.c:                           s->l1_table_offset + 8 * 
l1_start_index,


--
Best regards,
Vladimir