Re: [Qemu-devel] [RFC 3/5] block: Fall back to fallback truncate function
On 12.07.19 12:04, Kevin Wolf wrote:
> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>> file-posix does not need to basically duplicate our fallback truncate
>> implementation; and sheepdog can fall back to it for "shrinking" files.
>>
>> Signed-off-by: Max Reitz
>> ---
>> block/file-posix.c | 21 +
>> block/sheepdog.c | 2 +-
>> 2 files changed, 2 insertions(+), 21 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index ab05b51a66..bcddfc7fbe 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2031,23 +2031,7 @@ static int coroutine_fn
>> raw_co_truncate(BlockDriverState *bs, int64_t offset,
>> return raw_regular_truncate(bs, s->fd, offset, prealloc, errp);
>> }
>
> The only thing that is left here is a fstat() check to see that we
> really have a regular file here. But since this function is for the
> 'file' driver, we can just assume this and the function can go away
> altogether.
>
> In fact, 'file' with block/character devices has been deprecated since
> 3.0, so we can just remove support for it now and make it more than just
> an assumption.
>
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 6f402e5d4d..4af4961cb7 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -2301,7 +2301,7 @@ static int coroutine_fn
>> sd_co_truncate(BlockDriverState *bs, int64_t offset,
>> max_vdi_size = (UINT64_C(1) << s->inode.block_size_shift) *
>> MAX_DATA_OBJS;
>> if (offset < old_size) {
>> error_setg(errp, "shrinking is not supported");
>> -return -EINVAL;
>> +return -ENOTSUP;
>> } else if (offset > max_vdi_size) {
>> error_setg(errp, "too big image size");
>> return -EINVAL;
>
> The image will be unchanged and the guest will keep seeing the old image
> size, so is there any use case where having the fallback here makes
> sense? The only expected case where an image is shrunk is that the user
> explicitly sent block_resize - and in that case returning success, but
> doing nothing achieves nothing except confusing the user.
>
> file-posix has the same confusing case, but at least it also has cases
> where the fake truncate actually achieves something (such a allowing to
> create images on block devices).
Hm, yes, that’s right. It is as confusing for block devices, but that
at least gives something in return. For sheepdog (and others, like
ssh), there is nothing in return.
Explaining for every block driver why it needs to be a bit confusing and
fall back to the fixed-size device implementation (because it doesn’t
implement creation) seems a bit off, too.
Hm. Maybe the protocol creation fallback should just ignore failures
when truncating an image and in such a case zero the first sector of the
image? Maybe it should just ignore ENOTSUP errors?
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC 3/5] block: Fall back to fallback truncate function
Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
> file-posix does not need to basically duplicate our fallback truncate
> implementation; and sheepdog can fall back to it for "shrinking" files.
>
> Signed-off-by: Max Reitz
> ---
> block/file-posix.c | 21 +
> block/sheepdog.c | 2 +-
> 2 files changed, 2 insertions(+), 21 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index ab05b51a66..bcddfc7fbe 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2031,23 +2031,7 @@ static int coroutine_fn
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
> return raw_regular_truncate(bs, s->fd, offset, prealloc, errp);
> }
The only thing that is left here is a fstat() check to see that we
really have a regular file here. But since this function is for the
'file' driver, we can just assume this and the function can go away
altogether.
In fact, 'file' with block/character devices has been deprecated since
3.0, so we can just remove support for it now and make it more than just
an assumption.
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 6f402e5d4d..4af4961cb7 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2301,7 +2301,7 @@ static int coroutine_fn sd_co_truncate(BlockDriverState
> *bs, int64_t offset,
> max_vdi_size = (UINT64_C(1) << s->inode.block_size_shift) *
> MAX_DATA_OBJS;
> if (offset < old_size) {
> error_setg(errp, "shrinking is not supported");
> -return -EINVAL;
> +return -ENOTSUP;
> } else if (offset > max_vdi_size) {
> error_setg(errp, "too big image size");
> return -EINVAL;
The image will be unchanged and the guest will keep seeing the old image
size, so is there any use case where having the fallback here makes
sense? The only expected case where an image is shrunk is that the user
explicitly sent block_resize - and in that case returning success, but
doing nothing achieves nothing except confusing the user.
file-posix has the same confusing case, but at least it also has cases
where the fake truncate actually achieves something (such a allowing to
create images on block devices).
Kevin
[Qemu-devel] [RFC 3/5] block: Fall back to fallback truncate function
file-posix does not need to basically duplicate our fallback truncate
implementation; and sheepdog can fall back to it for "shrinking" files.
Signed-off-by: Max Reitz
---
block/file-posix.c | 21 +
block/sheepdog.c | 2 +-
2 files changed, 2 insertions(+), 21 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index ab05b51a66..bcddfc7fbe 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2031,23 +2031,7 @@ static int coroutine_fn raw_co_truncate(BlockDriverState
*bs, int64_t offset,
return raw_regular_truncate(bs, s->fd, offset, prealloc, errp);
}
-if (prealloc != PREALLOC_MODE_OFF) {
-error_setg(errp, "Preallocation mode '%s' unsupported for this "
- "non-regular file", PreallocMode_str(prealloc));
-return -ENOTSUP;
-}
-
-if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
-if (offset > raw_getlength(bs)) {
-error_setg(errp, "Cannot grow device files");
-return -EINVAL;
-}
-} else {
-error_setg(errp, "Resizing this file is not supported");
-return -ENOTSUP;
-}
-
-return 0;
+return -ENOTSUP;
}
#ifdef __OpenBSD__
@@ -3413,7 +3397,6 @@ static BlockDriver bdrv_host_device = {
.bdrv_io_unplug = raw_aio_unplug,
.bdrv_attach_aio_context = raw_aio_attach_aio_context,
-.bdrv_co_truncate = raw_co_truncate,
.bdrv_getlength= raw_getlength,
.bdrv_get_info = raw_get_info,
.bdrv_get_allocated_file_size
@@ -3537,7 +3520,6 @@ static BlockDriver bdrv_host_cdrom = {
.bdrv_io_unplug = raw_aio_unplug,
.bdrv_attach_aio_context = raw_aio_attach_aio_context,
-.bdrv_co_truncate= raw_co_truncate,
.bdrv_getlength = raw_getlength,
.has_variable_length = true,
.bdrv_get_allocated_file_size
@@ -3669,7 +3651,6 @@ static BlockDriver bdrv_host_cdrom = {
.bdrv_io_unplug = raw_aio_unplug,
.bdrv_attach_aio_context = raw_aio_attach_aio_context,
-.bdrv_co_truncate= raw_co_truncate,
.bdrv_getlength = raw_getlength,
.has_variable_length = true,
.bdrv_get_allocated_file_size
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 6f402e5d4d..4af4961cb7 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2301,7 +2301,7 @@ static int coroutine_fn sd_co_truncate(BlockDriverState
*bs, int64_t offset,
max_vdi_size = (UINT64_C(1) << s->inode.block_size_shift) * MAX_DATA_OBJS;
if (offset < old_size) {
error_setg(errp, "shrinking is not supported");
-return -EINVAL;
+return -ENOTSUP;
} else if (offset > max_vdi_size) {
error_setg(errp, "too big image size");
return -EINVAL;
--
2.21.0
