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

[Maxim Levitsky]

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 <address@hidden>
> > ---
> >  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;
> 
>