[Qemu-devel] [PATCH 0/3] respect bdrv_getlength() error code

[Qemu-devel] [PATCH 0/3] respect bdrv_getlength() error code

[Denis V. Lunev]

These cases were reported by Markus Armbruster <armbru@redhat.com>
Patches add error checking of the bdrv_getlength() call or remove
the call of that function.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Markus Armbruster <armbru@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH 1/3] block: respect error code from bdrv_getlength in handle_aiocb_write_zeroes

[Denis V. Lunev]

Original idea beyond the code in question was the following: we have failed
to write zeroes with fallocate(FALLOC_FL_ZERO_RANGE) as the simplest
approach and via fallocate(FALLOC_FL_PUNCH_HOLE)/fallocate(0). We have the
only chance now: if the request comes beyond end of the file. Thus we
should calculate file length and respect the error code from that op.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Markus Armbruster <armbru@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/file-posix.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index cfbb236f6f..f4de022ae0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1339,6 +1339,9 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
 #if defined(CONFIG_FALLOCATE) || defined(CONFIG_XFS)
     BDRVRawState *s = aiocb->bs->opaque;
 #endif
+#ifdef CONFIG_FALLOCATE
+    int64_t len;
+#endif
 
     if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
         return handle_aiocb_write_zeroes_block(aiocb);
@@ -1381,7 +1384,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
 #endif
 
 #ifdef CONFIG_FALLOCATE
-    if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
+    /* Last resort: we are trying to extend the file with zeroed data. This
+     * can be done via fallocate(fd, 0) */
+    len = bdrv_getlength(aiocb->bs);
+    if (s->has_fallocate && len >= 0 && aiocb->aio_offset >= len) {
         int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
         if (ret == 0 || ret != -ENOTSUP) {
             return ret;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/3] block: respect error code from bdrv_getlength in handle_aiocb_write_zeroes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

On 08/04/2017 10:10 AM, Denis V. Lunev wrote:
> Original idea beyond the code in question was the following: we have failed
> to write zeroes with fallocate(FALLOC_FL_ZERO_RANGE) as the simplest
> approach and via fallocate(FALLOC_FL_PUNCH_HOLE)/fallocate(0). We have the
> only chance now: if the request comes beyond end of the file. Thus we
> should calculate file length and respect the error code from that op.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/file-posix.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 2/3] parallels: respect error code of bdrv_getlength() in allocate_clusters()

[Denis V. Lunev]

If we can not get the file length, the state of BDS is broken completely.
Return error to the caller.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Markus Armbruster <armbru@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 5bbdfabb7a..6794e53c0b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -192,7 +192,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
                                  int nb_sectors, int *pnum)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t pos, space, idx, to_allocate, i;
+    int64_t pos, space, idx, to_allocate, i, len;
 
     pos = block_status(s, sector_num, nb_sectors, pnum);
     if (pos > 0) {
@@ -214,7 +214,11 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
     assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
 
     space = to_allocate * s->tracks;
-    if (s->data_end + space > bdrv_getlength(bs->file->bs) >> BDRV_SECTOR_BITS) {
+    len = bdrv_getlength(bs->file->bs);
+    if (len < 0) {
+        return len;
+    }
+    if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
         int ret;
         space += s->prealloc_size;
         if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/3] parallels: respect error code of bdrv_getlength() in allocate_clusters()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On 08/04/2017 10:10 AM, Denis V. Lunev wrote:
> If we can not get the file length, the state of BDS is broken completely.
> Return error to the caller.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 3/3] parallels: drop check that bdrv_truncate() is working

[Denis V. Lunev]

This would be actually strange and error prone. If truncate() nowadays
will fail, there is something fatally wrong. Let's check for that during
the actual work.

The only fallback case is when the file is not zero initialized. In this
case we should switch to preallocation via fallocate().

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Markus Armbruster <armbru@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6794e53c0b..e1e06d23cc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail_options;
     }
 
-    if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
-            bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs),
-                          PREALLOC_MODE_OFF, NULL) != 0) {
+    if (!bdrv_has_zero_init(bs->file->bs)) {
         s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
     }
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH 3/3] parallels: drop check that bdrv_truncate() is working

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

On 08/04/2017 10:10 AM, Denis V. Lunev wrote:
> This would be actually strange and error prone. If truncate() nowadays
> will fail, there is something fatally wrong. Let's check for that during
> the actual work.
> 
> The only fallback case is when the file is not zero initialized. In this
> case we should switch to preallocation via fallocate().

I got confused by the commit message.  Here's my attempt an an
alternative, to see if I'm understanding the point of this patch:

The code was trying to truncate a file to its current length, as an
optimization to help decide whether an alternative prealloc mode was
useful.  But it forgot to check whether bdrv_getlength() succeeded, and
dealing with that failure just complicates what was supposed to be a
no-op probe for optimizing later operation.  We will still properly fail
later when an actual truncation attempt is made and the device can't
support it.  So when deciding how to set prealloc_mode while opening the
device, all we really need is to check just the one condition that
matters - knowing whether the device is zero initialized.

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

The code change makes sense at first glance, but I'm a bit reluctant to
give R-b, since the commit message threw me off and I'm not familiar
with the parallels code in the first place.

> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 6794e53c0b..e1e06d23cc 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail_options;
>      }
>  
> -    if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
> -            bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs),
> -                          PREALLOC_MODE_OFF, NULL) != 0) {
> +    if (!bdrv_has_zero_init(bs->file->bs)) {
>          s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
>      }
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 3/3] parallels: drop check that bdrv_truncate() is working

[Denis V. Lunev]

On 08/04/2017 10:32 PM, Eric Blake wrote:
> On 08/04/2017 10:10 AM, Denis V. Lunev wrote:
>> This would be actually strange and error prone. If truncate() nowadays
>> will fail, there is something fatally wrong. Let's check for that during
>> the actual work.
>>
>> The only fallback case is when the file is not zero initialized. In this
>> case we should switch to preallocation via fallocate().
> I got confused by the commit message.  Here's my attempt an an
> alternative, to see if I'm understanding the point of this patch:
>
> The code was trying to truncate a file to its current length, as an
> optimization to help decide whether an alternative prealloc mode was
> useful.  But it forgot to check whether bdrv_getlength() succeeded, and
> dealing with that failure just complicates what was supposed to be a
> no-op probe for optimizing later operation.  We will still properly fail
> later when an actual truncation attempt is made and the device can't
> support it.  So when deciding how to set prealloc_mode while opening the
> device, all we really need is to check just the one condition that
> matters - knowing whether the device is zero initialized.
>

This is not an optimization. This is check that truncate is working for
underlying BDS. 2 years ago I though that I have seen some BDSes
without truncate support.

So, with this patch I have dropped this check. If the user has specified
to change preallocation mode, he is responsible to be correct. If truncate
is broken, this will be revealed at the first attempt to expand the file.

>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  block/parallels.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
> The code change makes sense at first glance, but I'm a bit reluctant to
> give R-b, since the commit message threw me off and I'm not familiar
> with the parallels code in the first place.
>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 6794e53c0b..e1e06d23cc 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>          goto fail_options;
>>      }
>>  
>> -    if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
>> -            bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs),
>> -                          PREALLOC_MODE_OFF, NULL) != 0) {
>> +    if (!bdrv_has_zero_init(bs->file->bs)) {
>>          s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
>>      }
>>  
>>

Re: [Qemu-devel] [PATCH 0/3] respect bdrv_getlength() error code

[Kevin Wolf]

Am 04.08.2017 um 17:10 hat Denis V. Lunev geschrieben:
> These cases were reported by Markus Armbruster <armbru@redhat.com>
> Patches add error checking of the bdrv_getlength() call or remove
> the call of that function.

Thanks, applied to the block branch.

Kevin