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 <mre...@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)
>
> 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?
>
>
> 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.
>
>
> 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.
>
> 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.
I think you want just
bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE);
Best regards,
Maxim Levitsky
> > + 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;
>
>
