On 26.08.19 00:03, Nir Soffer wrote: > When creating an image with preallocation "off" or "falloc", the first > block of the image is typically not allocated. When using Gluster > storage backed by XFS filesystem, reading this block using direct I/O > succeeds regardless of request length, fooling alignment detection. > > In this case we fallback to a safe value (4096) instead of the optimal > value (512), which may lead to unneeded data copying when aligning > requests. Allocating the first block avoids the fallback. > > Since we allocate the first block even with preallocation=off, we no > longer create images with zero disk size: > > $ ./qemu-img create -f raw test.raw 1g > Formatting 'test.raw', fmt=raw size=1073741824 > > $ ls -lhs test.raw > 4.0K -rw-r--r--. 1 nsoffer nsoffer 1.0G Aug 16 23:48 test.raw > > And converting the image requires additional cluster: > > $ ./qemu-img measure -f raw -O qcow2 test.raw > required size: 458752 > fully allocated size: 1074135040 > > I did quick performance test for copying disks with qemu-img convert to > new raw target image to Gluster storage with sector size of 512 bytes: > > for i in $(seq 10); do > rm -f dst.raw > sleep 10 > time ./qemu-img convert -f raw -O raw -t none -T none src.raw dst.raw > done > > Here is a table comparing the total time spent: > > Type Before(s) After(s) Diff(%) > --------------------------------------- > real 530.028 469.123 -11.4 > user 17.204 10.768 -37.4 > sys 17.881 7.011 -60.7 > > We can see very clear improvement in CPU usage. > > Signed-off-by: Nir Soffer <[email protected]> > --- > block/file-posix.c | 43 +++++++++++++++++++ > tests/qemu-iotests/{150.out => 150.out.qcow2} | 0 > tests/qemu-iotests/150.out.raw | 12 ++++++ > tests/qemu-iotests/175 | 19 +++++--- > tests/qemu-iotests/175.out | 8 ++-- > tests/qemu-iotests/178.out.qcow2 | 4 +- > tests/qemu-iotests/221.out | 12 ++++-- > tests/qemu-iotests/253.out | 12 ++++-- > 8 files changed, 90 insertions(+), 20 deletions(-) > rename tests/qemu-iotests/{150.out => 150.out.qcow2} (100%) > create mode 100644 tests/qemu-iotests/150.out.raw > > diff --git a/block/file-posix.c b/block/file-posix.c > index fbeb0068db..51688ae3fc 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1749,6 +1749,39 @@ static int handle_aiocb_discard(void *opaque) > return ret; > } > > +/* > + * Help alignment probing by allocating the first block. > + * > + * When reading with direct I/O from unallocated area on Gluster backed by > XFS, > + * reading succeeds regardless of request length. In this case we fallback to > + * safe alignment which is not optimal. Allocating the first block avoids > this > + * fallback. > + * > + * fd may be opened with O_DIRECT, but we don't know the buffer alignment or > + * request alignment, so we use safe values. > + * > + * Returns: 0 on success, -errno on failure. Since this is an optimization, > + * caller may ignore failures. > + */ > +static int allocate_first_block(int fd, size_t max_size) > +{ > + size_t write_size = MIN(MAX_BLOCKSIZE, max_size);
Hm, well, there was a reason why I proposed rounding this down to the
next power of two. If max_size is not a power of two but below
MAX_BLOCKSIZE, write_size will not be a power of two, and thus the write
below may fail even if write_size exceeds the physical block size.
You can see that in the test case you add by using e.g. 768 as the
destination size (provided your test filesystem has a block size of 512).
Now I would like to say that it’s stupid to resize an O_DIRECT file to a
size that is not a multiple of the block size; but I’ve had a bug
assigned to me before because that didn’t work.
But maybe it’s actually better if it doesn’t work. I don’t know.
> + size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
> + void *buf;
> + ssize_t n;
> +
> + buf = qemu_memalign(max_align, write_size);
> + memset(buf, 0, write_size);
> +
> + do {
> + n = pwrite(fd, buf, write_size, 0);
> + } while (n == -1 && errno == EINTR);
> +
> + qemu_vfree(buf);
> +
> + return (n == -1) ? -errno : 0;
> +}
> +
> static int handle_aiocb_truncate(void *opaque)
> {
> RawPosixAIOData *aiocb = opaque;
> @@ -1788,6 +1821,13 @@ static int handle_aiocb_truncate(void *opaque)
> /* posix_fallocate() doesn't set errno. */
> error_setg_errno(errp, -result,
> "Could not preallocate new data");
> + } else if (current_length == 0) {
> + /*
> + * Needed only if posix_fallocate() used fallocate(), but we
> + * don't have a way to detect that.
This sounds a bit weird because fallocate() is what we call
posix_fallocate() for. I’d’ve liked something that states more
explicitly that unaligned reads from fallocated areas may succeed even
with O_DIRECT, hence the need for allocate_first_block().
> Optimize future alignment
> + * probing; ignore failures.
> + */
> + allocate_first_block(fd, offset);
> }
> } else {
> result = 0;
[...]
> diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
> index 51e62c8276..d54cb43c39 100755
> --- a/tests/qemu-iotests/175
> +++ b/tests/qemu-iotests/175
> @@ -37,14 +37,16 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> # the file size. This function hides the resulting difference in the
> # stat -c '%b' output.
> # Parameter 1: Number of blocks an empty file occupies
> -# Parameter 2: Image size in bytes
> +# Parameter 2: Minimal number of blocks in an image
> +# Parameter 3: Image size in bytes
> _filter_blocks()
> {
> extra_blocks=$1
> - img_size=$2
> + min_blocks=$2
> + img_size=$3
>
> - sed -e "s/blocks=$extra_blocks\\(\$\\|[^0-9]\\)/nothing allocated/" \
> - -e "s/blocks=$((extra_blocks + img_size /
> 512))\\(\$\\|[^0-9]\\)/everything allocated/"
> + sed -e "s/blocks=$((min_blocks))\\(\$\\|[^0-9]\\)/min allocation/" \
Superfluous parentheses ($(())), but not wrong.
So I think I can give a
Reviewed-by: Max Reitz <[email protected]>
signature.asc
Description: OpenPGP digital signature
