[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 18/39] qcow2: Refactor get_cluster_table()
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v3 18/39] qcow2: Refactor get_cluster_table() |
Date: |
Thu, 1 Feb 2018 19:10:58 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
On 2018-02-01 11:40, Alberto Garcia wrote:
> 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.
Ah, right, yeah... Well, too bad. Then I probably can't ask for the
assert() that badly.
Reviewed-by: Max Reitz <address@hidden>
signature.asc
Description: OpenPGP digital signature