[RESEND] file-posix: Handle `EINVAL` fallocate return value

Commit Message

Antoine Damhet July 17, 2020, 1:56 p.m. UTC

From: Antoine Damhet <[e-mail]>

The `detect-zeroes=unmap` option may issue unaligned
`FALLOC_FL_PUNCH_HOLE` requests, raw block devices can (and will) return
`EINVAL`, qemu should then write the zeroes to the blockdev instead of
issuing an `IO_ERROR`.

Signed-off-by: Antoine Damhet <[e-mail]>
---

I am resending this patch because:
1. I sent the first version from the wrong email address
2. I forgot to send it to the devel mailing list

Please forgive me for the inconvenience.

 block/file-posix.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Kevin Wolf July 20, 2020, 2:07 p.m. UTC | #1

Am 17.07.2020 um 15:56 hat [e-mail] geschrieben:
> From: Antoine Damhet <[e-mail]>
> 
> The `detect-zeroes=unmap` option may issue unaligned
> `FALLOC_FL_PUNCH_HOLE` requests, raw block devices can (and will) return
> `EINVAL`, qemu should then write the zeroes to the blockdev instead of
> issuing an `IO_ERROR`.
> 
> Signed-off-by: Antoine Damhet <[e-mail]>

Do you have a simple reproducer for this? I tried it with something like
this (also with a LV instead of loop, but it didn't really make a
difference):

$ ./qemu-io -c 'write -P 0 42 1234' --image-opts driver=host_device,filename=/dev/loop0,cache.direct=on,detect-zeroes=on
wrote 1234/1234 bytes at offset 42
1.205 KiB, 1 ops; 00.00 sec (2.021 MiB/sec and 1717.5697 ops/sec)

So I don't seem to run into an error.

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 8067e238cb..b2fabcc1b8 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1620,7 +1620,11 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
>  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
>      int ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
>                             aiocb->aio_offset, aiocb->aio_nbytes);
> -    if (ret != -ENOTSUP) {
> +    switch (ret) {
> +    case -ENOTSUP:
> +    case -EINVAL:
> +        break;
> +    default:
>          return ret;
>      }
>  #endif

This means that we fall back to BLKZEROOUT in case of -EINVAL. Does this
return a better error code in the relevant cases, or did you just happen
to test a case where it was skipped or returned -ENOTSUP?

Kevin

Antoine Damhet July 20, 2020, 3:37 p.m. UTC | #2

On Mon, Jul 20, 2020 at 04:07:26PM +0200, Kevin Wolf wrote:
> Am 17.07.2020 um 15:56 hat [e-mail] geschrieben:
> > From: Antoine Damhet <[e-mail]>
> > 
> > The `detect-zeroes=unmap` option may issue unaligned
> > `FALLOC_FL_PUNCH_HOLE` requests, raw block devices can (and will) return
> > `EINVAL`, qemu should then write the zeroes to the blockdev instead of
> > issuing an `IO_ERROR`.
> > 
> > Signed-off-by: Antoine Damhet <[e-mail]>
> 
> Do you have a simple reproducer for this? I tried it with something like
> this (also with a LV instead of loop, but it didn't really make a
> difference):
> 
> $ ./qemu-io -c 'write -P 0 42 1234' --image-opts driver=host_device,filename=/dev/loop0,cache.direct=on,detect-zeroes=on
> wrote 1234/1234 bytes at offset 42
> 1.205 KiB, 1 ops; 00.00 sec (2.021 MiB/sec and 1717.5697 ops/sec)
> 
> So I don't seem to run into an error.

```
$ qemu-io -c 'write -P 0 42 1234' --image-opts driver=host_device,filename=/dev/loop0,detect-zeroes=unmap
write failed: Invalid argument
```

This seems do do the trick :) (We triggered the bug with Windows 10
guests and with an iSCSI drive so it was hardly a simple reproducer).

> 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 8067e238cb..b2fabcc1b8 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1620,7 +1620,11 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
> >  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> >      int ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> >                             aiocb->aio_offset, aiocb->aio_nbytes);
> > -    if (ret != -ENOTSUP) {
> > +    switch (ret) {
> > +    case -ENOTSUP:
> > +    case -EINVAL:
> > +        break;
> > +    default:
> >          return ret;
> >      }
> >  #endif
> 
> This means that we fall back to BLKZEROOUT in case of -EINVAL. Does this
> return a better error code in the relevant cases, or did you just happen
> to test a case where it was skipped or returned -ENOTSUP?

I guess I misinterpreted the comment before calling
`handle_aiocb_write_zeroes`.

The codepath is:
* handle_aiocb_write_zeroes_unmap -> handle_aiocb_write_zeroes -> handle_aiocb_write_zeroes_block

In witch the code will return `-ENOSTUP` (`!s->has_write_zeroes`) and
never fall back to `BLKZEROOUT`.

So it's working as I expected but now I am unsure that my fix is the
right thing to do, what do you think ?

> 
> Kevin
> 
>

Kevin Wolf July 21, 2020, 2:21 p.m. UTC | #3

Am 20.07.2020 um 17:37 hat Antoine Damhet geschrieben:
> On Mon, Jul 20, 2020 at 04:07:26PM +0200, Kevin Wolf wrote:
> > Am 17.07.2020 um 15:56 hat [e-mail] geschrieben:
> > > From: Antoine Damhet <[e-mail]>
> > > 
> > > The `detect-zeroes=unmap` option may issue unaligned
> > > `FALLOC_FL_PUNCH_HOLE` requests, raw block devices can (and will) return
> > > `EINVAL`, qemu should then write the zeroes to the blockdev instead of
> > > issuing an `IO_ERROR`.
> > > 
> > > Signed-off-by: Antoine Damhet <[e-mail]>
> > 
> > Do you have a simple reproducer for this? I tried it with something like
> > this (also with a LV instead of loop, but it didn't really make a
> > difference):
> > 
> > $ ./qemu-io -c 'write -P 0 42 1234' --image-opts driver=host_device,filename=/dev/loop0,cache.direct=on,detect-zeroes=on
> > wrote 1234/1234 bytes at offset 42
> > 1.205 KiB, 1 ops; 00.00 sec (2.021 MiB/sec and 1717.5697 ops/sec)
> > 
> > So I don't seem to run into an error.
> 
> ```
> $ qemu-io -c 'write -P 0 42 1234' --image-opts driver=host_device,filename=/dev/loop0,detect-zeroes=unmap
> write failed: Invalid argument
> ```
> 
> This seems do do the trick :) (We triggered the bug with Windows 10
> guests and with an iSCSI drive so it was hardly a simple reproducer).

Oops, I made a stupid mistake with the detect-zeroes syntax there. :-)

So you actually need non-O_DIRECT. Okay, I can reproduce with your line.

> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 8067e238cb..b2fabcc1b8 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -1620,7 +1620,11 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
> > >  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> > >      int ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> > >                             aiocb->aio_offset, aiocb->aio_nbytes);
> > > -    if (ret != -ENOTSUP) {
> > > +    switch (ret) {
> > > +    case -ENOTSUP:
> > > +    case -EINVAL:
> > > +        break;
> > > +    default:
> > >          return ret;
> > >      }
> > >  #endif
> > 
> > This means that we fall back to BLKZEROOUT in case of -EINVAL. Does this
> > return a better error code in the relevant cases, or did you just happen
> > to test a case where it was skipped or returned -ENOTSUP?
> 
> I guess I misinterpreted the comment before calling
> `handle_aiocb_write_zeroes`.
> 
> The codepath is:
> * handle_aiocb_write_zeroes_unmap -> handle_aiocb_write_zeroes -> handle_aiocb_write_zeroes_block
> 
> In witch the code will return `-ENOSTUP` (`!s->has_write_zeroes`) and
> never fall back to `BLKZEROOUT`.
> 
> So it's working as I expected but now I am unsure that my fix is the
> right thing to do, what do you think ?

It's obviously fixing something for you, though it might not be as
complete as we'd like. Maybe we would just need to do the same thing in
more places, though ideally we would need reproducers for each of them.

I just noticed that handle_aiocb_write_zeroes() already turns EINVAL
into ENOTSUP, so your change is not unprecedented.

I'll just apply your patch for now, and we can always fix more cases on
top of it.

Kevin

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index 8067e238cb..b2fabcc1b8 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1620,7 +1620,11 @@  static int handle_aiocb_write_zeroes_unmap(void *opaque)
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
     int ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
                            aiocb->aio_offset, aiocb->aio_nbytes);
-    if (ret != -ENOTSUP) {
+    switch (ret) {
+    case -ENOTSUP:
+    case -EINVAL:
+        break;
+    default:
         return ret;
     }
 #endif