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

[Kevin Wolf]

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 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).

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

signature.asc
Description: PGP signature