Re: [PATCH 4/8] block: Add @exact parameter to bdrv_co_truncate()

[Max Reitz]

On 18.09.19 22:50, Maxim Levitsky wrote:
> On Wed, 2019-09-18 at 11:51 +0200, Max Reitz wrote:
>> We have two drivers (iscsi and file-posix) that (in some cases) return
>> success from their .bdrv_co_truncate() implementation if the block
>> device is larger than the requested offset, but cannot be shrunk.  Some
>> callers do not want that behavior, so this patch adds a new parameter
>> that they can use to turn off that behavior.
>>
>> This patch just adds the parameter and lets the block/io.c and
>> block/block-backend.c functions pass it around.  All other callers
>> always pass false and none of the implementations evaluate it, so that
>> this patch does not change existing behavior.  Future patches take care
>> of that.
>>
>> Suggested-by: Maxim Levitsky <address@hidden>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  include/block/block.h          |  6 +++---
>>  include/block/block_int.h      | 17 ++++++++++++++++-
>>  include/sysemu/block-backend.h |  4 ++--
>>  block/block-backend.c          |  6 +++---
>>  block/commit.c                 |  5 +++--
>>  block/crypto.c                 |  8 ++++----
>>  block/file-posix.c             |  3 ++-
>>  block/file-win32.c             |  3 ++-
>>  block/gluster.c                |  1 +
>>  block/io.c                     | 20 +++++++++++++-------
>>  block/iscsi.c                  |  3 ++-
>>  block/mirror.c                 |  4 ++--
>>  block/nfs.c                    |  2 +-
>>  block/parallels.c              |  6 +++---
>>  block/qcow.c                   |  4 ++--
>>  block/qcow2-refcount.c         |  2 +-
>>  block/qcow2.c                  | 22 ++++++++++++----------
>>  block/qed.c                    |  3 ++-
>>  block/raw-format.c             |  5 +++--
>>  block/rbd.c                    |  1 +
>>  block/sheepdog.c               |  5 +++--
>>  block/ssh.c                    |  3 ++-
>>  block/vdi.c                    |  2 +-
>>  block/vhdx-log.c               |  4 ++--
>>  block/vhdx.c                   |  7 ++++---
>>  block/vmdk.c                   |  8 ++++----
>>  block/vpc.c                    |  2 +-
>>  blockdev.c                     |  2 +-
>>  qemu-img.c                     |  2 +-
>>  qemu-io-cmds.c                 |  2 +-
>>  tests/test-block-iothread.c    |  8 ++++----
>>  31 files changed, 102 insertions(+), 68 deletions(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 37c9de7446..2f905ae4b7 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -346,10 +346,10 @@ BlockDriverState 
>> *bdrv_find_backing_image(BlockDriverState *bs,
>>      const char *backing_file);
>>  void bdrv_refresh_filename(BlockDriverState *bs);
>>  
>> -int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
>> +int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool 
>> exact,
>>                                    PreallocMode prealloc, Error **errp);
>> -int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
>> -                  Error **errp);
>> +int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
>> +                  PreallocMode prealloc, Error **errp);
>>  
>>  int64_t bdrv_nb_sectors(BlockDriverState *bs);
>>  int64_t bdrv_getlength(BlockDriverState *bs);
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 0422acdf1c..197cc6e7c3 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -334,8 +334,23 @@ struct BlockDriver {
>>       * bdrv_parse_filename.
>>       */
>>      const char *protocol_name;
>> +
>> +    /*
>> +     * Truncate @bs to @offset bytes using the given @prealloc mode
>> +     * when growing.  Modes other than PREALLOC_MODE_OFF should be
>> +     * rejected when shrinking @bs.
>> +     *
>> +     * If @exact is true, @bs must be resized to exactly @offset.
>> +     * Otherwise, it is sufficient for @bs (if it is a host block
>> +     * device and thus there is no way to resize it) to be at least
>> +     * @offset bytes in length.
>> +     *
>> +     * If @exact is true and this function fails but would succeed
>> +     * with @exact = false, it should return -ENOTSUP.
>> +     */
> Thanks for adding a documentation here!
> One minor nitpick:
> I would write
> 
> Otherwise, it is sufficient for @bs (for example if it is a host block
> device and thus there is no way to resize it) to be at least @offset bytes in 
> length.

Hm, so just add the “for example”?  I’d rather not add it.  This would
then read as if it were OK for files that aren’t block devices to also
return success when they cannot be shrunk just because we don’t support
it.  But it isn’t.  If the protocol theoretically allows it and it makes
sense, drivers shouldn’t return success with exact=false simply because
we haven’t implemented it.

For example, you can shrink files over ssh, I’m sure.  But our driver
doesn’t allow it.  It should thus return ENOTSUP even with exact=false.

The “Return success for shrinking even when the file cannot be shrunk”
really is only for block devices, because then the user will have no
expectation that the shrinking will actually work.  (For ssh, they will
expect it to work, so we must not pretend it succeeded when it didn’t.)

Max

signature.asc
Description: OpenPGP digital signature