Re: [Qemu-devel] [PATCH v3 18/39] qcow2: Refactor get_cluster_table()
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
>>> ---
>>> 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 18/39] qcow2: Refactor get_cluster_table()
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
>> ---
>> 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
Re: [Qemu-devel] [PATCH v3 18/39] qcow2: Refactor get_cluster_table()
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
> ---
> 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);
Max
>
> /* find the cluster offset for the given disk offset */
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 18/39] qcow2: Refactor get_cluster_table()
On 01/26/2018 08:59 AM, 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 > --- > block/qcow2-cluster.c | 21 +++-- > 1 file changed, 7 insertions(+), 14 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
