[Qemu-devel] [PATCH 0/4 for-3.0?] NBD fixes for unaligned images

[Qemu-devel] [PATCH 0/4 for-3.0?] NBD fixes for unaligned images

[Eric Blake]

Rich reported a bug when using qemu as client to nbdkit serving
a non-sector-aligned image; in turn, I found a second bug with
qemu as server of such an image.

Both bugs were present in 2.12, and thus are not new regressions
in 3.0. If there is a reason to spin -rc4, then these could be
included; but this series alone is not a driving reason to cause
-rc4.

Eric Blake (4):
  block: Add bdrv_get_request_alignment()
  nbd/server: Advertise actual minimum block size
  iotests: Add 228 to test NBD on unaligned images
  nbd/client: Deal with unaligned size from server

 include/sysemu/block-backend.h |  1 +
 block/block-backend.c          |  7 +++
 block/nbd.c                    | 11 ++++-
 nbd/server.c                   | 10 +++--
 tests/qemu-iotests/228         | 96 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/228.out     |  8 ++++
 tests/qemu-iotests/group       |  1 +
 7 files changed, 129 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/228
 create mode 100644 tests/qemu-iotests/228.out

-- 
2.14.4

[Qemu-devel] [PATCH 1/4] block: Add bdrv_get_request_alignment()

[Eric Blake]

The next patch needs access to a device's minimum permitted
alignment, since NBD wants to advertise this to clients. Add
an accessor function, borrowing from blk_get_max_transfer()
for accessing a backend's block limits.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/sysemu/block-backend.h | 1 +
 block/block-backend.c          | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 830d873f24f..20f8bbbce37 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -176,6 +176,7 @@ bool blk_is_available(BlockBackend *blk);
 void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
+uint32_t blk_get_request_alignment(BlockBackend *blk);
 uint32_t blk_get_max_transfer(BlockBackend *blk);
 int blk_get_max_iov(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
diff --git a/block/block-backend.c b/block/block-backend.c
index f2f75a977d7..fb8c827d117 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1798,6 +1798,13 @@ int blk_get_flags(BlockBackend *blk)
     }
 }

+/* Returns the minimum request alignment, in bytes; guaranteed nonzero */
+uint32_t blk_get_request_alignment(BlockBackend *blk)
+{
+    BlockDriverState *bs = blk_bs(blk);
+    return bs ? bs->bl.request_alignment : BDRV_SECTOR_SIZE;
+}
+
 /* Returns the maximum transfer length, in bytes; guaranteed nonzero */
 uint32_t blk_get_max_transfer(BlockBackend *blk)
 {
-- 
2.14.4

Re: [Qemu-devel] [PATCH 1/4] block: Add bdrv_get_request_alignment()

[Vladimir Sementsov-Ogievskiy]

02.08.2018 17:48, Eric Blake wrote:
> The next patch needs access to a device's minimum permitted
> alignment, since NBD wants to advertise this to clients. Add
> an accessor function, borrowing from blk_get_max_transfer()
> for accessing a backend's block limits.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 2/4] nbd/server: Advertise actual minimum block size

[Eric Blake]

Both NBD_CMD_BLOCK_STATUS and structured NBD_CMD_READ will split
their reply according to bdrv_block_status() boundaries. If the
block device has a request_alignment smaller than 512, but we
advertise a block alignment of 512 to the client, then this can
result in the server reply violating client expectations by
reporting a smaller region of the export than what the client
is permitted to address.  Thus, it is imperative that we
advertise the actual minimum block limit, rather than blindly
rounding it up to 512 (bdrv_block_status() cannot return status
aligned any smaller than request_alignment).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index ea5fe0eb336..cd3c41f895b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -608,10 +608,12 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
     /* Send NBD_INFO_BLOCK_SIZE always, but tweak the minimum size
      * according to whether the client requested it, and according to
      * whether this is OPT_INFO or OPT_GO. */
-    /* minimum - 1 for back-compat, or 512 if client is new enough.
-     * TODO: consult blk_bs(blk)->bl.request_alignment? */
-    sizes[0] =
-            (client->opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1;
+    /* minimum - 1 for back-compat, or actual if client will obey it. */
+    if (client->opt == NBD_OPT_INFO || blocksize) {
+        sizes[0] = blk_get_request_alignment(exp->blk);
+    } else {
+        sizes[0] = 1;
+    }
     /* preferred - Hard-code to 4096 for now.
      * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
     sizes[1] = 4096;
-- 
2.14.4

Re: [Qemu-devel] [PATCH 2/4] nbd/server: Advertise actual minimum block size

[Vladimir Sementsov-Ogievskiy]

02.08.2018 17:48, Eric Blake wrote:
> Both NBD_CMD_BLOCK_STATUS and structured NBD_CMD_READ will split
> their reply according to bdrv_block_status() boundaries. If the
> block device has a request_alignment smaller than 512, but we
> advertise a block alignment of 512 to the client, then this can
> result in the server reply violating client expectations by
> reporting a smaller region of the export than what the client
> is permitted to address.  Thus, it is imperative that we
> advertise the actual minimum block limit, rather than blindly
> rounding it up to 512 (bdrv_block_status() cannot return status
> aligned any smaller than request_alignment).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   nbd/server.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index ea5fe0eb336..cd3c41f895b 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -608,10 +608,12 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
>       /* Send NBD_INFO_BLOCK_SIZE always, but tweak the minimum size
>        * according to whether the client requested it, and according to
>        * whether this is OPT_INFO or OPT_GO. */
> -    /* minimum - 1 for back-compat, or 512 if client is new enough.
> -     * TODO: consult blk_bs(blk)->bl.request_alignment? */
> -    sizes[0] =
> -            (client->opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1;
> +    /* minimum - 1 for back-compat, or actual if client will obey it. */
> +    if (client->opt == NBD_OPT_INFO || blocksize) {
> +        sizes[0] = blk_get_request_alignment(exp->blk);
> +    } else {
> +        sizes[0] = 1;
> +    }
>       /* preferred - Hard-code to 4096 for now.
>        * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
>       sizes[1] = 4096;

Here, what about dirty bitmap export through BLOCK_STATUS? Could it's 
granularity be less than request_alignment? Shouldn't we handle it somehow?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] nbd/server: Advertise actual minimum block size

[Eric Blake]

On 08/17/2018 08:49 AM, Vladimir Sementsov-Ogievskiy wrote:

>> BDRV_SECTOR_SIZE : 1;
>> +    /* minimum - 1 for back-compat, or actual if client will obey it. */
>> +    if (client->opt == NBD_OPT_INFO || blocksize) {
>> +        sizes[0] = blk_get_request_alignment(exp->blk);
>> +    } else {
>> +        sizes[0] = 1;
>> +    }
>>       /* preferred - Hard-code to 4096 for now.
>>        * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
>>       sizes[1] = 4096;
> 
> Here, what about dirty bitmap export through BLOCK_STATUS? Could it's 
> granularity be less than request_alignment? Shouldn't we handle it somehow?

Can you create a dirty bitmap with a granularity smaller than 
request_alignment?  I know you can configure dirty bitmap granularity 
independently from cluster size (in both directions: either smaller or 
larger than cluster size), but that it has a least a minimum lower 
bounds of 512.  You're probably right that we also want it to have a 
minimum lower bound of the request_alignment (if you're using a device 
with 4k minimum I/O, request_alignment would be 4k, and having a dirty 
bitmap any smaller than that granularity is wasted space).

On the other hand, I also think we're safe for this patch: even if you 
waste the space by creating a bitmap with too-small granularity, the 
actions that write bits into the bitmap will still be aligned to 
request_alignment, which means you always set a multiple of bits per 
action; when reading back the dirty bitmap to report over 
NBD_CMD_BLOCK_STATUS, you'll never encounter a change in bit status 
except on alignment boundaries (based on how the bits were written), and 
thus what NBD reports will still be aligned to the advertised minimum 
size, rather than the smaller bitmap granularity.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/4] nbd/server: Advertise actual minimum block size

[Vladimir Sementsov-Ogievskiy]

17.08.2018 17:53, Eric Blake wrote:
> On 08/17/2018 08:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>> BDRV_SECTOR_SIZE : 1;
>>> +    /* minimum - 1 for back-compat, or actual if client will obey 
>>> it. */
>>> +    if (client->opt == NBD_OPT_INFO || blocksize) {
>>> +        sizes[0] = blk_get_request_alignment(exp->blk);
>>> +    } else {
>>> +        sizes[0] = 1;
>>> +    }
>>>       /* preferred - Hard-code to 4096 for now.
>>>        * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
>>>       sizes[1] = 4096;
>>
>> Here, what about dirty bitmap export through BLOCK_STATUS? Could it's 
>> granularity be less than request_alignment? Shouldn't we handle it 
>> somehow?
>
> Can you create a dirty bitmap with a granularity smaller than 
> request_alignment?  I know you can configure dirty bitmap granularity 
> independently from cluster size (in both directions: either smaller or 
> larger than cluster size), but that it has a least a minimum lower 
> bounds of 512.  You're probably right that we also want it to have a 
> minimum lower bound of the request_alignment (if you're using a device 
> with 4k minimum I/O, request_alignment would be 4k, and having a dirty 
> bitmap any smaller than that granularity is wasted space).
>
> On the other hand, I also think we're safe for this patch: even if you 
> waste the space by creating a bitmap with too-small granularity, the 
> actions that write bits into the bitmap will still be aligned to 
> request_alignment,

Not quite right: nbd_export_bitmap searches through the backing chain 
for the bitmap, and it may correspond to the bds with smaller 
request_alignment.

> which means you always set a multiple of bits per action; when reading 
> back the dirty bitmap to report over NBD_CMD_BLOCK_STATUS, you'll 
> never encounter a change in bit status except on alignment boundaries 
> (based on how the bits were written), and thus what NBD reports will 
> still be aligned to the advertised minimum size, rather than the 
> smaller bitmap granularity.
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] nbd/server: Advertise actual minimum block size

[Eric Blake]

On 08/17/2018 10:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Can you create a dirty bitmap with a granularity smaller than 
>> request_alignment?  I know you can configure dirty bitmap granularity 
>> independently from cluster size (in both directions: either smaller or 
>> larger than cluster size), but that it has a least a minimum lower 
>> bounds of 512.  You're probably right that we also want it to have a 
>> minimum lower bound of the request_alignment (if you're using a device 
>> with 4k minimum I/O, request_alignment would be 4k, and having a dirty 
>> bitmap any smaller than that granularity is wasted space).
>>
>> On the other hand, I also think we're safe for this patch: even if you 
>> waste the space by creating a bitmap with too-small granularity, the 
>> actions that write bits into the bitmap will still be aligned to 
>> request_alignment,
> 
> Not quite right: nbd_export_bitmap searches through the backing chain 
> for the bitmap, and it may correspond to the bds with smaller 
> request_alignment.

Hmm. Interesting setup:

base (512-byte alignment, bitmap granularity) <- active (4k alignment)

so you're stating that exporting 'active' but with the 'bitmap' 
inherited from the backing chain from base means that the bitmap may 
have transitions in between the alignment advertised by active.  Then it 
sounds like nbd/server.c:bitmap_to_extents() should be aware of that 
fact, and intentionally round up (anywhere that it finds a mid-alignment 
transition, treat the entire aligned region as dirty. It never hurts to 
mark more of the image dirty than necessary, but does hurt to expose a 
mid-alignment transition to an NBD client not expecting one).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH 3/4] iotests: Add 228 to test NBD on unaligned images

[Eric Blake]

Add a test for the NBD server fix in the previous patch.  In
short, when serving a raw POSIX file that is not aligned to
sector boundaries, qemu must not split a structured read or
block status result any smaller than the block size that it
advertised to the client; since qemu as client rejects servers
that split up a block status.

Not tested yet, but worth adding to this test: an NBD server
that can advertise a non-sector-aligned size (such as nbdkit)
causes qemu as the NBD client to misbehave when it rounds the
size up and accesses beyond the advertised size. Qemu as NBD
server never advertises a non-sector-aligned size (since
bdrv_getlength() currently rounds up to sector boundaries);
until qemu can act as such a server, testing this flaw will
have to rely on external binaries.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/228     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/228.out |  8 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 105 insertions(+)
 create mode 100755 tests/qemu-iotests/228
 create mode 100644 tests/qemu-iotests/228.out

diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228
new file mode 100755
index 00000000000..390fe5f6512
--- /dev/null
+++ b/tests/qemu-iotests/228
@@ -0,0 +1,96 @@
+#!/bin/bash
+#
+# Test qemu-nbd vs. unaligned images
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1 # failure is the default!
+
+nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
+rm -f "${TEST_DIR}/qemu-nbd.pid"
+
+_cleanup_nbd()
+{
+    local NBD_PID
+    if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
+        read NBD_PID < "${TEST_DIR}/qemu-nbd.pid"
+        rm -f "${TEST_DIR}/qemu-nbd.pid"
+        if [ -n "$NBD_PID" ]; then
+            kill "$NBD_PID"
+        fi
+    fi
+    rm -f "$nbd_unix_socket"
+}
+
+_wait_for_nbd()
+{
+    for ((i = 0; i < 300; i++))
+    do
+        if [ -r "$nbd_unix_socket" ]; then
+            return
+        fi
+        sleep 0.1
+    done
+    echo "Failed in check of unix socket created by qemu-nbd"
+    exit 1
+}
+
+_cleanup()
+{
+    _cleanup_test_img
+    _cleanup_nbd
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto nbd
+_supported_os Linux
+_require_command QEMU_NBD
+
+echo
+echo "=== Exporting unaligned raw image ==="
+echo
+
+# can't use _make_test_img, because qemu-img rounds image size up,
+# and because we want to use Unix socket rather than TCP port. Likewise,
+# we have to redirect TEST_IMG to our server.
+printf %01000d 0 > "$TEST_IMG_FILE"
+_cleanup_nbd
+$QEMU_NBD -f $IMGFMT -v -t -k "$nbd_unix_socket" -e 42 -x '' "$TEST_IMG_FILE" &
+_wait_for_nbd
+TEST_IMG="nbd:unix:$nbd_unix_socket"
+
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+$QEMU_IO -c map "$TEST_IMG"
+
+# Not tested yet: we also want to ensure that qemu as NBD client does
+# not access beyond the end of a server's advertised unaligned size.
+# However, since qemu as server always rounds up to a sector alignment,
+# we would have to use nbdkit to provoke the current client failures.
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/228.out b/tests/qemu-iotests/228.out
new file mode 100644
index 00000000000..057e3732f8c
--- /dev/null
+++ b/tests/qemu-iotests/228.out
@@ -0,0 +1,8 @@
+QA output created by 228
+
+=== Exporting unaligned raw image ===
+
+[{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true},
+{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true}]
+1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b973dc842d9..5bfe2e246d5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -225,3 +225,4 @@
 225 rw auto quick
 226 auto quick
 227 auto quick
+228 rw auto quick
-- 
2.14.4

[Qemu-devel] [PATCH 4/4] nbd/client: Deal with unaligned size from server

[Eric Blake]

When a server advertises an unaligned size but no block sizes,
the code was rounding up to a sector-aligned size (a known
limitation of bdrv_getlength()), then assuming a request_alignment
of 512 (the recommendation of the NBD spec for maximum portability).
However, this means that qemu will actually attempt to access the
padding bytes of the trailing partial sector.

An easy demonstration, using nbdkit as the server:
$ nbdkit -fv random size=1023
$ qemu-io -r -f raw -c 'r -v 0 1023' nbd://localhost:10809
read failed: Invalid argument

because the client rounded the request up to 1024 bytes, which
nbdkit then rejected as beyond the advertised size of 1023.

Note that qemu as the server refuses to send an unaligned size, as
it has already rounded the unaligned image up to sector size, and
then happily resizes the image on access (at least when serving a
POSIX file over NBD).

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c
index e87699fb73b..a3e6889c57f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -473,7 +473,16 @@ 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, then pick the
+     * largest power of 2 that evenly divides the advertised size, but
+     * does not exceed a sector.
+     */
+    if (!min) {
+        min = 1 << ctz32(BDRV_SECTOR_SIZE | s->info.size);
+    }
+
+    bs->bl.request_alignment = min;
     bs->bl.max_pdiscard = max;
     bs->bl.max_pwrite_zeroes = max;
     bs->bl.max_transfer = max;
-- 
2.14.4

Re: [Qemu-devel] [PATCH 4/4] nbd/client: Deal with unaligned size from server

[Vladimir Sementsov-Ogievskiy]

02.08.2018 17:48, Eric Blake wrote:
> When a server advertises an unaligned size but no block sizes,
> the code was rounding up to a sector-aligned size (a known
> limitation of bdrv_getlength()), then assuming a request_alignment
> of 512 (the recommendation of the NBD spec for maximum portability).
> However, this means that qemu will actually attempt to access the
> padding bytes of the trailing partial sector.
>
> An easy demonstration, using nbdkit as the server:
> $ nbdkit -fv random size=1023
> $ qemu-io -r -f raw -c 'r -v 0 1023' nbd://localhost:10809
> read failed: Invalid argument
>
> because the client rounded the request up to 1024 bytes, which
> nbdkit then rejected as beyond the advertised size of 1023.
>
> Note that qemu as the server refuses to send an unaligned size, as
> it has already rounded the unaligned image up to sector size, and
> then happily resizes the image on access (at least when serving a
> POSIX file over NBD).
>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/4] nbd/client: Deal with unaligned size from server

[Eric Blake]

On 08/17/2018 08:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> 02.08.2018 17:48, Eric Blake wrote:
>> When a server advertises an unaligned size but no block sizes,
>> the code was rounding up to a sector-aligned size (a known
>> limitation of bdrv_getlength()), then assuming a request_alignment
>> of 512 (the recommendation of the NBD spec for maximum portability).
>> However, this means that qemu will actually attempt to access the
>> padding bytes of the trailing partial sector.
>>
>> An easy demonstration, using nbdkit as the server:
>> $ nbdkit -fv random size=1023
>> $ qemu-io -r -f raw -c 'r -v 0 1023' nbd://localhost:10809
>> read failed: Invalid argument
>>
>> because the client rounded the request up to 1024 bytes, which
>> nbdkit then rejected as beyond the advertised size of 1023.
>>
>> Note that qemu as the server refuses to send an unaligned size, as
>> it has already rounded the unaligned image up to sector size, and
>> then happily resizes the image on access (at least when serving a
>> POSIX file over NBD).
>>
>> Reported-by: Richard W.M. Jones <rjones@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

This patch is not a full solution. It fixes things so that a client 
accessing the first half of the final sector no longer rounds things up 
and chokes the server, but does not prevent a client from attempting to 
access the second half of the final sector (where that access still 
reaches the server).  I probably need yet another patch, similar to 
Rich's 'nbdkit --filter truncate', where reads of the trailing hole 
created by qemu's rounding are padded to NUL without asking the server, 
and where writes are ignored if all zero or cause ENOSPACE if nonzero.

Or, a much bigger patch series to make qemu quit rounding size up :) 
(I'd like to get there someday, but it's faster to kick out the quick 
patch for just NBD than to audit the entire block stack)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [Qemu-block] [PATCH 0/4 for-3.0?] NBD fixes for unaligned images

[John Snow]

Hi, has this series been superseded or do we need a respin for 3.1?

(It's not quite been a month, but it caught my eye.)

--js

On 08/02/2018 10:48 AM, Eric Blake wrote:
> Rich reported a bug when using qemu as client to nbdkit serving
> a non-sector-aligned image; in turn, I found a second bug with
> qemu as server of such an image.
> 
> Both bugs were present in 2.12, and thus are not new regressions
> in 3.0. If there is a reason to spin -rc4, then these could be
> included; but this series alone is not a driving reason to cause
> -rc4.
> 
> Eric Blake (4):
>   block: Add bdrv_get_request_alignment()
>   nbd/server: Advertise actual minimum block size
>   iotests: Add 228 to test NBD on unaligned images
>   nbd/client: Deal with unaligned size from server
> 
>  include/sysemu/block-backend.h |  1 +
>  block/block-backend.c          |  7 +++
>  block/nbd.c                    | 11 ++++-
>  nbd/server.c                   | 10 +++--
>  tests/qemu-iotests/228         | 96 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/228.out     |  8 ++++
>  tests/qemu-iotests/group       |  1 +
>  7 files changed, 129 insertions(+), 5 deletions(-)
>  create mode 100755 tests/qemu-iotests/228
>  create mode 100644 tests/qemu-iotests/228.out
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/4 for-3.0?] NBD fixes for unaligned images

[Eric Blake]

On 9/10/18 2:25 PM, John Snow wrote:
> Hi, has this series been superseded or do we need a respin for 3.1?

Needs a respin.

There are two issues still to be solved:

1. Asking the top-most block layer for its alignment isn't necessarily 
right for qemu as server.  If we have:

backing (512) <- active (4k)

and tell the client that they can't access anything smaller than 4k 
granularity, but then read through to the backing layer which does just 
that, then we're wrong.  Either the block layer has to be beefed up to 
make bdrv_block_status() round backing file's smaller granularity into 
the size we advertised (perhaps by adding a flag to bdrv_block_status() 
to declare whether we prefer the most accurate information vs. the 
rounded information) - or else qemu as NBD server should ALWAYS 
advertise a blocksize of 1-512 (1 because we can, even if by 
read-modify-write; or 512 because except for EOF issues on a 
non-sector-aligned file, we don't encounter mid-sector transitions 
anywhere else, and EOF is easier to special case than everywhere).

2. Patch 5 fixes qemu as client to not round valid requests past EOF, 
but does not fix the case of a request that intentionally exceeds EOF 
but fits within the rounded file size from reaching the server. Either 
the NBD client code should itself perform EOF validation (reads from the 
EOF hole see zero, writes of anything other than zero fail with ENOSPC), 
or the NBD client code should round the server's file size down instead 
of up.  I haven't decided which approach is better.

But right now, these fixes are taking a back seat to me trying to get a 
libvirt incremental backup demo working.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org