On 23.08.2022 09:38, Vladimir Sementsov-Ogievskiy wrote:
> On 8/22/22 12:05, Alexander Ivanov wrote:
>> Make data_end pointing to the end of the last cluster if a leak was
>> fixed.
>> Otherwise set the file size to data_end.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> block/parallels.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index c245ca35cd..2be56871bc 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -513,7 +513,15 @@ static int coroutine_fn
>> parallels_co_check(BlockDriverState *bs,
>> res->leaks_fixed += count;
>> }
>> }
>> -
>> + /*
>> + * If res->image_end_offset less than the file size,
>> + * a leak was fixed and we should set the new offset to
>> s->data_end.
>> + * Otherwise set the file size to s->data_end.
>
> I'm not sure about English :) For me "set A to B" means A := B, but
> you use it visa versa..
I hesitated about this. I saw both variants and used the incorrect one
probably. =)
>
>> + */
>> + if (res->image_end_offset < size && fix & BDRV_FIX_LEAKS) {
>> + size = res->image_end_offset;
>> + }
>> + s->data_end = size >> BDRV_SECTOR_BITS;
>
> Hmm, actually, when size < data_end, you can shrink data_end only if
> (fix & BDRV_FIX_ERRORS), otherwise BAT is still not fixed.
We shrink an image if there is a leak and (fix & BDRV_FIX_LEAKS).
Otherwise we set data_end to the file size. In such a way we don't
change the file size in parallels_close() even if we have out-of-image
offsets in BAT.
>
>> out:
>> qemu_co_mutex_unlock(&s->lock);
>> return ret;
>
>
> Actually I think we only need to modify s->data_end after successful
> BAT fixing above. Then, bdrv_truncate is formally unrelated to
> s->data_end and shouldn't touch it.
>
> So, I think, more correct is something like
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 2be56871bc..b268788974 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -479,20 +479,24 @@ static int coroutine_fn
> parallels_co_check(BlockDriverState *bs,
> prev_off = off;
> }
>
> ret = 0;
> if (flush_bat) {
> ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size,
> s->header, 0);
> if (ret < 0) {
> res->check_errors++;
> goto out;
> }
> +
> + /* We have dropped some wrong clusters, update data_end */
> + assert(s->data_end * BDRV_SECTOR_SIZE >= high_off +
> s->cluster_size);
> + s->data_end = (high_off + s->cluster_size) / BDRV_SECTOR_SIZE;
> }
>
> res->image_end_offset = high_off + s->cluster_size;
> if (size > res->image_end_offset) {
>
If an image was opened in RW mode for checking without fixing (I don't
know if it's possible), data_end can be set outside the image and the
image will be expanded in parallels_close().
OK, I would propose to fix data_end in two places:
1) after out-of-image check set data_end to the highest offset with (off
+ cluster_size <= file_size) condition, independent on fix flags;
2) after leak check, only if (size > res->image_end_offset).
In such a way we can have only one inconsistence after any check:
out-of-image offsets isn't fixed and we have offsets after data_end. But
it is much better than if we set data_end to petabytes, for example.