Re: [Qemu-block] [PATCH] block/io.c: fix for the allocation failure

[Kevin Wolf]

Am 08.04.2019 um 12:04 hat Kevin Wolf geschrieben:
> Am 08.04.2019 um 11:44 hat Andrey Shinkevich geschrieben:
> > 
> > 
> > On 06/04/2019 01:50, John Snow wrote:
> > > 
> > > 
> > > On 4/5/19 10:24 AM, Andrey Shinkevich wrote:
> > >> On a file system used by the customer, fallocate() returns an error
> > >> if the block is not properly aligned. So, bdrv_co_pwrite_zeroes()
> > >> fails. We can handle that case the same way as it is done for the
> > >> unsupported cases, namely, call to bdrv_driver_pwritev() that writes
> > >> zeroes to an image for the unaligned chunk of the block.
> > >>
> > >> Suggested-by: Denis V. Lunev <address@hidden>
> > >> Signed-off-by: Andrey Shinkevich <address@hidden>
> > >> ---
> > >>   block/io.c | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/block/io.c b/block/io.c
> > >> index dfc153b..0412a51 100644
> > >> --- a/block/io.c
> > >> +++ b/block/io.c
> > >> @@ -1516,7 +1516,7 @@ static int coroutine_fn 
> > >> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> > >>               assert(!bs->supported_zero_flags);
> > >>           }
> > >>   
> > >> -        if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) {
> > >> +        if (ret < 0 && !(flags & BDRV_REQ_NO_FALLBACK)) {
> > >>               /* Fall back to bounce buffer if write zeroes is 
> > >> unsupported */
> > >>               BdrvRequestFlags write_flags = flags & 
> > >> ~BDRV_REQ_ZERO_WRITE;
> > >>   
> > >>
> > > 
> > > I suppose that if fallocate fails for any reason and we're allowing
> > > fallback, we're either going to succeed ... or fail again very soon
> > > thereafter.
> > > 
> > > Are there any cases where it is vital to not ignore the first fallocate
> > > failure? I'm a little wary of ignoring the return code from
> > > bdrv_co_pwrite_zeroes, but I am assuming that if there is a "real"
> > > failure here that the following bounce writes will also fail "safely."
> > > 
> > > I'm not completely confident, but I have no tangible objections:
> > > Reviewed-by: John Snow <address@hidden>
> > > 
> > 
> > Thank you for your review, John!
> > 
> > Let me clarify the circumstances and quote the bug report:
> > "Customer had Win-2012 VM with 50GB system disk which was later resized 
> > to 256GB without resizing the partition inside VM.
> > Now, while trying to resize to 50G, the following error will appear
> > 'Failed to reduce the number of L2 tables: Invalid argument'
> > It was found that it is possible to shrink the disk to 128G and any size 
> > above that number, but size below 128G will bring the mentioned error."
> > 
> > The fallocate() returns no error on that file system if the offset and
> > the (offset + bytes) parameters of the bdrv_co_do_pwrite_zeroes() both
> > are aligned to 4K.
> 
> What is the return value you get from this file system?
> 
> Maybe turning that into ENOTSUP in file-posix would be less invasive.
> Just falling back for any error gives me the vague feeling that it could
> cause problems sooner or later.

Also, which file system is this? This seems to be a file system bug.
fallocate() isn't documented to possibly have alignment restrictions for
FALLOC_FL_ZERO_RANGE (if this is the operation you're talking about).
FALLOC_FL_PUNCH_HOLE even explicitly mentions the behaviour for partial
blocks, so there is no doubt that operations for partial blocks are
considered valid. Operations that may impose restrictions are explicitly
documented as such.

So in any case, this would only be a workaround for a buggy file system.
The real bug needs to be fixed there.

Kevin