Re: [Qemu-devel] [PATCH v2 4/5] block: Perform copy-on-read in loop

[Kevin Wolf <kwolf@redhat.com>]

Am 04.10.2017 um 03:43 hat Eric Blake geschrieben:
> Improve our braindead copy-on-read implementation.  Pre-patch,
> we have multiple issues:
> - we create a bounce buffer and perform a write for the entire
> request, even if the active image already has 99% of the
> clusters occupied, and really only needs to copy-on-read the
> remaining 1% of the clusters
> - our bounce buffer was as large as the read request, and can
> needlessly exhaust our memory by using double the memory of
> the request size (the original request plus our bounce buffer),
> rather than a capped maximum overhead beyond the original
> - if a driver has a max_transfer limit, we are bypassing the
> normal code in bdrv_aligned_preadv() that fragments to that
> limit, and instead attempt to read the entire buffer from the
> driver in one go, which some drivers may assert on
> - a client can request a large request of nearly 2G such that
> rounding the request out to cluster boundaries results in a
> byte count larger than 2G.  While this cannot exceed 32 bits,
> it DOES have some follow-on problems:
> -- the call to bdrv_driver_pread() can assert for exceeding
> BDRV_REQUEST_MAX_BYTES, if the driver is old and lacks
> .bdrv_co_preadv
> -- if the buffer is all zeroes, the subsequent call to
> bdrv_co_do_pwrite_zeroes is a no-op due to a negative size,
> which means we did not actually copy on read
> 
> Fix all of these issues by breaking up the action into a loop,
> where each iteration is capped to sane limits.  Also, querying
> the allocation status allows us to optimize: when data is
> already present in the active layer, we don't need to bounce.
> 
> Note that the code has a telling comment that copy-on-read
> should probably be a filter driver rather than a bolt-in hack
> in io.c; but that remains a task for another day.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: avoid uninit ret on 0-length op [patchew, Kevin]

Reviewed-by: Kevin Wolf <kwolf@redhat.com>