Re: [Qemu-devel] [PATCH v3 18/39] qcow2: Refactor get_cluster_table()

[Alberto Garcia]

On Wed 31 Jan 2018 09:11:48 PM CET, Max Reitz wrote:
> On 2018-01-26 15:59, Alberto Garcia wrote:
>> After the previous patch we're now always using l2_load() in
>> get_cluster_table() regardless of whether a new L2 table has to be
>> allocated or not.
>> 
>> This patch refactors that part of the code to use one single l2_load()
>> call.
>> 
>> Signed-off-by: Alberto Garcia <address@hidden>
>> ---
>>  block/qcow2-cluster.c | 21 +++++++--------------
>>  1 file changed, 7 insertions(+), 14 deletions(-)
>> 
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 2a53c1dc5f..0c0cab85e8 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -695,15 +695,7 @@ static int get_cluster_table(BlockDriverState *bs, 
>> uint64_t offset,
>>          return -EIO;
>>      }
>>  
>> -    /* seek the l2 table of the given l2 offset */
>> -
>> -    if (s->l1_table[l1_index] & QCOW_OFLAG_COPIED) {
>> -        /* load the l2 table in memory */
>> -        ret = l2_load(bs, offset, l2_offset, &l2_table);
>> -        if (ret < 0) {
>> -            return ret;
>> -        }
>> -    } else {
>> +    if (!(s->l1_table[l1_index] & QCOW_OFLAG_COPIED)) {
>>          /* First allocate a new L2 table (and do COW if needed) */
>>          ret = l2_allocate(bs, l1_index);
>>          if (ret < 0) {
>> @@ -719,11 +711,12 @@ static int get_cluster_table(BlockDriverState *bs, 
>> uint64_t offset,
>>          /* Get the offset of the newly-allocated l2 table */
>>          l2_offset = s->l1_table[l1_index] & L1E_OFFSET_MASK;
>>          assert(offset_into_cluster(s, l2_offset) == 0);
>> -        /* Load the l2 table in memory */
>> -        ret = l2_load(bs, offset, l2_offset, &l2_table);
>> -        if (ret < 0) {
>> -            return ret;
>> -        }
>> +    }
>> +
>> +    /* load the l2 table in memory */
>> +    ret = l2_load(bs, offset, l2_offset, &l2_table);
>> +    if (ret < 0) {
>> +        return ret;
>>      }
>
> I'd pull the l2_offset assignment (and alignment check) down below the
> QCOW_OFLAG_COPIED check, saving us another two lines (!!1!) which we
> could then spend on an
>
> assert(s->l1_table[l1_index] & QCOW_OFLAG_COPIED);

I'm not sure if I'm following you. We need to set l2_offset before and
after allocating a new L2 table.

Before, because we need the offset of the old table (if there was one)
in order to decrease its refcount.

And after, because we need the offset of the new table in order to load
it.

Berto