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

[Antoine Damhet]

On Mon, Jul 20, 2020 at 04:07:26PM +0200, Kevin Wolf wrote:
> Am 17.07.2020 um 15:56 hat antoine.damhet@blade-group.com geschrieben:
> > From: Antoine Damhet <antoine.damhet@blade-group.com>
> > 
> > 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 <antoine.damhet@blade-group.com>
> 
> 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
> 
> 

-- 
Antoine 'xdbob' Damhet

signature.asc
Description: PGP signature