Re: [PATCH 4/5] block/io: fix bdrv_co_do_pwrite_zeroes head calculation

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

14.03.2020 0:47, Eric Blake wrote:
> On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It's wrong to update head using num in this place, as num may be
>> reduced during the iteration, and we'll have wrong head value on next
>> iteration.
>>
>> Instead update head at iteration end.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/io.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Offhand, I don't see how this fixes any bug....
> /me reads on
> 
>>
>> diff --git a/block/io.c b/block/io.c
>> index 75fd5600c2..c64566b4cf 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1785,7 +1785,6 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>>                * convenience, limit this request to max_transfer even if
>>                * we don't need to fall back to writes.  */
>>               num = MIN(MIN(bytes, max_transfer), alignment - head);
>> -            head = (head + num) % alignment;
>>               assert(num < max_write_zeroes);
> 
> Here, we've asserted that if head was non-zero, num was already smaller than max_write_zeroes.  The rest of the loop does indeed have code that appears like it can reduce num, but that code is guarded:
> 
>          /* limit request size */
>          if (num > max_write_zeroes) {
>              num = max_write_zeroes;
>          }
> ...
>          if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) {
>              /* Fall back to bounce buffer if write zeroes is unsupported */
>              BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;
> 
>              if ((flags & BDRV_REQ_FUA) &&
>                  !(bs->supported_write_flags & BDRV_REQ_FUA)) {
>                  /* No need for bdrv_driver_pwrite() to do a fallback
>                   * flush on each chunk; use just one at the end */
>                  write_flags &= ~BDRV_REQ_FUA;
>                  need_flush = true;
>              }
>              num = MIN(num, max_transfer);

Now I think that this is impossible. If we updated head above, than num is already less or equal to max_transfer, so is not updated by this line. Or do I now miss something?

So, the patch may be good as refactoring but not really needed for 5.0.

> 
> Oh. Now I see.  If max_write_zeroes is > max_transfer, but we fall back to a bounce buffer, it is indeed possible that a misaligned request that forces fallbacks to writes may indeed require more than one write to get to the point where it is then using a buffer aligned to max_write_zeroes.
> 
> Do we have an iotest provoking this, or is it theoretical?  With an iotest, this one is material for 5.0 even if the rest of the series misses soft freeze.
> 
>>           } else if (tail && num > alignment) {
>>               /* Shorten the request to the last aligned sector.  */
>> @@ -1844,6 +1843,9 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>>           offset += num;
>>           bytes -= num;
>> +        if (head) {
>> +            head = offset % alignment;
>> +        }
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir