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

[Andrey Shinkevich]

On 08/04/2019 13:04, Kevin Wolf wrote:
> 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.
> 
> Kevin
> 

The return value for that custom distributed file system is
"Invalid argument", that's
"EINVAL: mode is FALLOC_FL_ZERO_RANGE, but the file referred to
by fd is not a regular file".

When I reproduced the bug, I saw that the alignment in the
bdrv_co_do_pwrite_zeroes() was set to '1' in that case:

MAX(bs->bl.pwrite_zeroes_alignment /=0/),
     bs->bl.request_alignment /=1/);

With my first patch I had not sent before, a new member of the structure
BlockLimits, say pwrite_zeroes_alignment_min, was set to 4K for a raw
file and the alignment was made for the block. Then zeroes were written
to the image for the unaligned head and tail by invoking the
bdrv_driver_pwritev(). That approach has cons also: we would write to
the disk always.

The file system maintainers say that the bug is a particular case and
they may not return the general error such as 'unsupported'.
-- 
Andrey