On Tue, Oct 03, 2017 at 08:43:46PM -0500, Eric Blake wrote: > 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-sta...@nongnu.org > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v2: avoid uninit ret on 0-length op [patchew, Kevin] > --- > block/io.c | 120 > +++++++++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 82 insertions(+), 38 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>