Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Add blk_truncate_for_formatting()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7407 bytes --]

On 16.07.19 17:45, Maxim Levitsky wrote:
> On Tue, 2019-07-16 at 16:08 +0300, Maxim Levitsky wrote:
>> On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  include/sysemu/block-backend.h | 12 ++++++++
>>>  block/block-backend.c          | 54 ++++++++++++++++++++++++++++++++++
>>>  2 files changed, 66 insertions(+)
>>>
>>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>>> index 733c4957eb..cd9ec8bf52 100644
>>> --- a/include/sysemu/block-backend.h
>>> +++ b/include/sysemu/block-backend.h
>>> @@ -236,6 +236,18 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>>>                            int bytes);
>>>  int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>>>                   Error **errp);
>>> +
>>> +/**
>>> + * Wrapper of blk_truncate() for format drivers that need to truncate
>>> + * their protocol node before formatting it.
>>> + * Invoke blk_truncate() to truncate the file to @offset; if that
>>> + * fails with -ENOTSUP (and the file is already big enough), try to
>>> + * overwrite the first sector with zeroes.  If that succeeds, return
>>> + * success.
>>> + */
>>> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset,
>>> +                                Error **errp);
>>> +
>>>  int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
>>>  int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
>>>                       int64_t pos, int size);
>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>> index a8d160fd5d..c0e64b1ee1 100644
>>> --- a/block/block-backend.c
>>> +++ b/block/block-backend.c
>>> @@ -2041,6 +2041,60 @@ int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>>>      return bdrv_truncate(blk->root, offset, prealloc, errp);
>>>  }
>>>  
>>> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset, Error **errp)
>>> +{
>>> +    Error *local_err = NULL;
>>> +    int64_t current_size;
>>> +    int bytes_to_clear;
>>> +    int ret;
>>> +
>>> +    ret = blk_truncate(blk, offset, PREALLOC_MODE_OFF, &local_err);
>>> +    if (ret < 0 && ret != -ENOTSUP) {
>>> +        error_propagate(errp, local_err);
>>> +        return ret;
>>> +    } else if (ret >= 0) {
>>> +        return ret;
>>> +    }
>>
>> What if the truncate does succeed? For example the current implementation of raw_co_truncate,
>> does return zero when you truncate to less that block device size 
>> (and this is kind of wrong since you can't really change the block device size)

Ah, yes, stupid me.

>> Even more, I see is that in the later patch, you call this with offset == 0 which
>> I think will always succeed on a raw block device, thus skipping the zeroing code.
>>
>> How about just doing the zeroing in the bdrv_create_file_fallback?

Hm.  I can try.  The block drivers that use
blk_truncate_for_formatting() write a full header to the image file, so
they don’t need the sector be zero.

Alternatively, I could just treat ret == 0 the same as -ENOTSUP.  Then
the code would just go on to invoke blk_getlength() and see for itself
what to do.

>> Another idea:
>>
>> blk_truncate_for_formatting would first truncate the file to 0, then
>> check if the size of the file became zero in addition to the successful return value.
>>
>> If the file size became zero, truncate the file to the requested size - this should make sure that file is empty.
>> Otherwise, zero the first sector.
>>
>> It might also be nice to add a check that if the size didn't became zero, that it remained the same
>> to avoid strange situations of semi broken truncate.

Hm, I would expect the block device to handle that.  A state between
“successful resize” and “did not change at all, as intended for this
device” should always be an error.

But the device should zero the first cluster like we do here.

>> Also I would rename the function to something like blk_raw_format_file,
>> basically a function which tries its best to erase an existing file contents
>>
>>
>> Yet another idea would to drop the lying in the raw_co_truncate (on block devices), and fail always,
>> unless asked to truncate to the exact file size, and let the callers deal with that.
>> Callers where it is not critical for the truncate to work can just ignore this failure.
>> That is probably hard to implement 
>>
>> Or we can add a truncate 'mode' to .bdrv_co_truncate, which would let the caller indicate its intention,
>> that is if the caller must truncate to that size or it can accept truncate ending up in bigger file that it asked for.

Hm.  That sounds interesting.  Currently, qemu-img resize tries to
inquire whether the truncate did anything useful by checking the length
post-truncate.  It prints a warning if the size didn’t change.

Adding a flag would simplify that and probably this, so that sounds
useful indeed.

>> As we once discussed on IRC, the fact that truncate on a block device 'succeeds',
>> despite not really beeing able to change the block device size, causes other issues,
>> like not beeing able to use preallocation=full when creating a qcow2 image on a block device.
>>
>> Best regards,
>> 	Maxim Levitsky
>>
>>> +
>>> +    current_size = blk_getlength(blk);
>>> +    if (current_size < 0) {
>>> +        error_free(local_err);
>>> +        error_setg_errno(errp, -current_size,
>>> +                         "Failed to inquire new image file's current length");
>>> +        return current_size;
>>> +    }
>>> +
>>> +    if (current_size < offset) {
>>> +        /* Need to grow the image, but we failed to do that */
>>> +        error_propagate(errp, local_err);
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    error_free(local_err);
>>> +    /*
>>> +     * We can deal with images that are too big.  We just need to
>>> +     * clear the first sector.
>>> +     */
>>> +
>>> +    bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE) - offset;
> Also this I think is wrong when offset !=0, since assuming real world device, the
> MIN will be just BDRV_SECTOR_SIZE, so the result of this statement is negative number.

Oh, damn, yes.  Thanks!

> I think you want just
> bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE);

I don’t think I want that because I want to start clearing from @offset,
so I do need to subtract it.

But of course I don’t need to do anything if @offset >=
BDRV_SECTOR_SIZE, so there should just be an additional if () block.

Max

>>> +    if (bytes_to_clear) {
>>> +        if (!(blk->root->perm & BLK_PERM_WRITE)) {
>>> +            error_setg(errp, "Cannot clear first sector of new image: "
>>> +                       "Write permission missing");
>>> +            return -EPERM;
>>> +        }
>>> +
>>> +        ret = blk_pwrite_zeroes(blk, offset, bytes_to_clear, 0);
>>> +        if (ret < 0) {
>>> +            error_setg_errno(errp, -ret, "Failed to clear the first sector of "
>>> +                             "the new image");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static void blk_pdiscard_entry(void *opaque)
>>>  {
>>>      BlkRwCo *rwco = opaque;
>>
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]