Re: [Qemu-block] [PATCH v3 2/6] nbd/client: Lower min_block for block-status, unaligned size

[Vladimir Sementsov-Ogievskiy]

29.03.2019 7:27, Eric Blake wrote:
> We have a latent bug in our NBD client code, tickled by the brand new
> nbdkit 1.11.10 block status support:
> 
> $ nbdkit --filter=log --filter=truncate -U - \
>             data data="1" size=511 truncate=64K logfile=/dev/stdout \
>             --run 'qemu-img convert $nbd /var/tmp/out'
> ...
> qemu-img: block/io.c:2122: bdrv_co_block_status: Assertion `*pnum && 
> QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset' failed.
> 
> The culprit? Our implementation of .bdrv_co_block_status can return
> unaligned block status for any server that operates with a lower
> actual alignment than what we tell the block layer in
> request_alignment, in violation of the block layer's constraints. To
> date, we've been unable to trip the bug, because qemu as NBD server
> always advertises block sizing (at which point it is a server bug if
> the server sends unaligned status - although qemu 3.1 is such a server
> and I've sent separate patches for 4.0 both to get the server to obey
> the spec, and to let the client to tolerate server oddities at EOF).
> 
> But nbdkit does not (yet) advertise block sizing, and therefore is not
> in violation of the spec for returning block status at whatever
> boundaries it wants, and those unaligned results can occur anywhere
> rather than just at EOF. While we are still wise to avoid sending
> sub-sector read/write requests to a server of unknown origin, we MUST
> consider that a server telling us block status without an advertised
> block size is correct.  So, we either have to munge unaligned answers
> from the server into aligned ones that we hand back to the block
> layer, or we have to tell the block layer about a smaller alignment.
> 
> Similarly, if the server advertises an image size that is not
> sector-aligned, we might as well assume that the server intends to let
> us access those tail bytes, and therefore supports a minimum block
> size of 1, regardless of whether the server supports block status
> (although we still need more patches to fix the problem that with an
> unaligned image, we can send read or block status requests that exceed
> EOF to the server). Again, qemu as server cannot trip this problem
> (because it rounds images to sector alignment), but nbdkit advertised
> unaligned size even before it gained block status support.
> 
> Solve both alignment problems at once by using better heuristics on
> what alignment to report to the block layer when the server did not
> give us something to work with. Note that very few NBD servers
> implement block status (to date, only qemu and nbdkit are known to do
> so); and as the NBD spec mentioned block sizing constraints prior to
> documenting block status, it can be assumed that any future
> implementations of block status are aware that they must advertise
> block size if they want a minimum size other than 1.
> 
> We've had a long history of struggles with picking the right alignment
> to use in the block layer, as evidenced by the commit message of
> fd8d372d (v2.12) that introduced the current choice of forced 512-byte
> alignment.
> 
> There is no iotest coverage for this fix, because qemu can't provoke
> it, and I didn't want to make test 241 dependent on nbdkit.
> 
> Fixes: fd8d372d
> Reported-by: Richard W.M. Jones <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   block/nbd.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 2e72df528ac..208be596027 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -437,7 +437,24 @@ static void nbd_refresh_limits(BlockDriverState *bs, 
> Error **errp)
>       uint32_t min = s->info.min_block;
>       uint32_t max = MIN_NON_ZERO(NBD_MAX_BUFFER_SIZE, s->info.max_block);
> 
> -    bs->bl.request_alignment = min ? min : BDRV_SECTOR_SIZE;
> +    /*
> +     * If the server did not advertise an alignment:
> +     * - a size that is not sector-aligned implies that an alignment
> +     *   of 1 can be used to access those tail bytes
> +     * - advertisement of block status requires an alignment of 1, so
> +     *   that we don't violate block layer constraints that block
> +     *   status is always aligned (as we can't control whether the
> +     *   server will report sub-sector extents, such as a hole at EOF
> +     *   on an unaligned POSIX file)
> +     * - otherwise, assume the server is so old that we are safer avoiding
> +     *   sub-sector requests
> +     */
> +    if (!min) {
> +        min = (!QEMU_IS_ALIGNED(s->info.size, BDRV_SECTOR_SIZE) ||
> +               s->info.base_allocation) ? 1 : BDRV_SECTOR_SIZE;
> +    }
> +
> +    bs->bl.request_alignment = min;
>       bs->bl.max_pdiscard = max;
>       bs->bl.max_pwrite_zeroes = max;
>       bs->bl.max_transfer = max;
> 


-- 
Best regards,
Vladimir