[Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Max Reitz]

Some image formats (e.g. qcow2) require the underlying file to grow on
write accesses, but this is in fact not supported by all protocols (e.g.
nbd does not). If such a format requiring file growth is used
non-read-only over a protocol which does not support this, a warning
should be issued.

This warning is issued for example whenever one tries to export a qcow2
image over nbd-server and use the export from qemu.


This series depends on v2 of my series
"block: driver should override flags in bdrv_open()".


v2: Reworked because BDS.growable has been removed in the meantime, so
    this version does not need to reuse it and thus break its intended
    use.


Max Reitz (3):
  iotests: Make nested read in 072 and 089 read-only
  block: Introduce BDS.growing
  block: Introduce BlockDriver.requires_growing_file

 block.c                   | 20 ++++++++++++++++++++
 block/blkdebug.c          |  2 ++
 block/blkverify.c         |  2 ++
 block/iscsi.c             |  2 ++
 block/nbd.c               |  2 ++
 block/qcow.c              |  1 +
 block/qcow2.c             |  7 +++++++
 block/qed.c               |  1 +
 block/quorum.c            |  5 +++++
 block/raw_bsd.c           |  1 +
 block/vdi.c               |  2 ++
 block/vhdx.c              |  2 ++
 block/vmdk.c              |  1 +
 block/vpc.c               |  2 ++
 include/block/block_int.h |  7 +++++++
 tests/qemu-iotests/072    |  2 +-
 tests/qemu-iotests/089    |  2 +-
 17 files changed, 59 insertions(+), 2 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/3] iotests: Make nested read in 072 and 089 read-only

[Max Reitz]

iotests 072 and 089 create a nested qcow2-in-qcow2 image. This should be
opened read-only, for one because it is indeed read only, and also
because writing to it would probably turn out bad (the outer qcow2 image
cannot grow on demand, so no clusters can be allocated for the inner
one).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/072 | 2 +-
 tests/qemu-iotests/089 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/072 b/tests/qemu-iotests/072
index e4a723d..7bcf9f8 100755
--- a/tests/qemu-iotests/072
+++ b/tests/qemu-iotests/072
@@ -55,7 +55,7 @@ $QEMU_IO -c 'write -P 42 0 512' -c 'write -P 23 512 512' \
 
 $QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
 
-$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=$IMGFMT,file.file.filename=$TEST_IMG" \
+$QEMU_IO -c "open -r -o driver=$IMGFMT,file.driver=$IMGFMT,file.file.filename=$TEST_IMG" \
          -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
          -c 'read -P 66 1024 512' | _filter_qemu_io
 
diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
index 3e0038d..af22e1f 100755
--- a/tests/qemu-iotests/089
+++ b/tests/qemu-iotests/089
@@ -69,7 +69,7 @@ $QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
 
 $QEMU_IO_PROG --cache $CACHEMODE \
          -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
-         -c 'read -P 66 1024 512' "json:{
+         -c 'read -P 66 1024 512' -r "json:{
     \"driver\": \"$IMGFMT\",
     \"file\": {
         \"driver\": \"$IMGFMT\",
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 1/3] iotests: Make nested read in 072 and 089 read-only

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On 03/19/2015 01:03 PM, Max Reitz wrote:
> iotests 072 and 089 create a nested qcow2-in-qcow2 image. This should be
> opened read-only, for one because it is indeed read only, and also
> because writing to it would probably turn out bad (the outer qcow2 image
> cannot grow on demand, so no clusters can be allocated for the inner
> one).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/072 | 2 +-
>  tests/qemu-iotests/089 | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v2 2/3] block: Introduce BDS.growing

[Max Reitz]

This flag is set if the BDS's size can be increased by writing beyond
its end.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 4 ++++
 block/blkdebug.c          | 2 ++
 block/blkverify.c         | 2 ++
 block/iscsi.c             | 2 ++
 block/nbd.c               | 2 ++
 block/qcow2.c             | 5 +++++
 block/quorum.c            | 5 +++++
 block/raw_bsd.c           | 1 +
 include/block/block_int.h | 3 +++
 9 files changed, 26 insertions(+)

diff --git a/block.c b/block.c
index 4c620b1..6da30f8 100644
--- a/block.c
+++ b/block.c
@@ -1575,6 +1575,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
      * (the inverse results in an error message from bdrv_open_common()) */
     assert(!(flags & BDRV_O_PROTOCOL) || !file);
 
+    /* Default, can be overridden by drv->bdrv_file_open() (conversely,
+     * drv->bdrv_open() can override bs->growing being false by default) */
+    bs->growing = flags & BDRV_O_PROTOCOL;
+
     /* Open the image */
     ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
     if (ret < 0) {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 63611e0..83807f7 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -437,6 +437,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
+    bs->growing = bs->file->growing;
+
     /* Set request alignment */
     align = qemu_opt_get_size(opts, "align", bs->request_alignment);
     if (align > 0 && align < INT_MAX && !(align & (align - 1))) {
diff --git a/block/blkverify.c b/block/blkverify.c
index 438dff8..6add81c 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -141,6 +141,8 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    bs->growing = bs->file->growing && s->test_file->growing;
+
     ret = 0;
 fail:
     qemu_opts_del(opts);
diff --git a/block/iscsi.c b/block/iscsi.c
index 3e34b1f..dd94cda 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1285,6 +1285,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     const char *filename;
     int i, ret = 0;
 
+    bs->growing = false;
+
     if ((BDRV_SECTOR_SIZE % 512) != 0) {
         error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. "
                    "BDRV_SECTOR_SIZE(%lld) is not a multiple "
diff --git a/block/nbd.c b/block/nbd.c
index 2176186..c433555 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -262,6 +262,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     int result, sock;
     Error *local_err = NULL;
 
+    bs->growing = false;
+
     /* Pop the config into our state object. Exit if invalid. */
     nbd_config(s, options, &export, &local_err);
     if (local_err) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 32bdf75..b4e2aa3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -552,6 +552,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     int overlap_check_template = 0;
     uint64_t l2_cache_size, refcount_cache_size;
 
+    /* Note: bs->growing is not set, even though qcow2 supports writes beyond
+     * the virtual disk size; these writes do not increase the disk size,
+     * however, which would be necessary for that flag to be set (also, that
+     * behavior is used internally by qcow2 only anyway). */
+
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read qcow2 header");
diff --git a/block/quorum.c b/block/quorum.c
index 437b122..7a6bcb7 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -974,6 +974,11 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
         opened[i] = true;
     }
 
+    bs->growing = true;
+    for (i = 0; i < s->num_children; i++) {
+        bs->growing &= s->bs[i]->growing;
+    }
+
     g_free(opened);
     goto exit;
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index e3d2d04..72fd951 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -208,6 +208,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     bs->sg = bs->file->sg;
+    bs->growing = bs->file->growing;
 
     if (bs->probed && !bdrv_is_read_only(bs)) {
         fprintf(stderr,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dccb092..01dee2a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -345,6 +345,9 @@ struct BlockDriverState {
                          note this is a reference count */
     bool probed;
 
+    /* Set if writes beyond the BDS's size make it grow */
+    bool growing;
+
     BlockDriver *drv; /* NULL means no media */
     void *opaque;
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 2/3] block: Introduce BDS.growing

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

On 03/19/2015 01:03 PM, Max Reitz wrote:
> This flag is set if the BDS's size can be increased by writing beyond
> its end.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   | 4 ++++
>  block/blkdebug.c          | 2 ++
>  block/blkverify.c         | 2 ++
>  block/iscsi.c             | 2 ++
>  block/nbd.c               | 2 ++
>  block/qcow2.c             | 5 +++++
>  block/quorum.c            | 5 +++++
>  block/raw_bsd.c           | 1 +
>  include/block/block_int.h | 3 +++
>  9 files changed, 26 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v2 3/3] block: Introduce BlockDriver.requires_growing_file

[Max Reitz]

This flag is set if write accesses to BDSs managed by the respective
driver may lead to writes beyond the end of the underlying file BDS,
expecting it to increase its size accordingly.

This behavior, however, is only supported by protocol BDSs having the
BDS.growing flag set. If they do not, emit a warning.

An example of such a misconfiguration is exporting a qcow2 file using
the reference NBD server implementation, which cannot interpret the
qcow2 format and will thus expose it directly over the network. When
accessing that export with qemu, qemu's qcow2 driver may try to allocate
clusters over the NBD connection, writing to addresses beyond the size
of the NBD export, which will then fail (without the user knowing why,
without this warning).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 16 ++++++++++++++++
 block/qcow.c              |  1 +
 block/qcow2.c             |  2 ++
 block/qed.c               |  1 +
 block/vdi.c               |  2 ++
 block/vhdx.c              |  2 ++
 block/vmdk.c              |  1 +
 block/vpc.c               |  2 ++
 include/block/block_int.h |  4 ++++
 9 files changed, 31 insertions(+)

diff --git a/block.c b/block.c
index 6da30f8..2db27b5 100644
--- a/block.c
+++ b/block.c
@@ -1585,6 +1585,22 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         goto fail;
     }
 
+    /* requires_growing_file may only be true for format block drivers */
+    assert(!bs->drv->requires_growing_file || bs->file);
+    if (bs->drv->requires_growing_file && !bs->file->growing &&
+        (flags & BDRV_O_RDWR))
+    {
+        error_report("WARNING: Writes to the image '%s' in format '%s'\n"
+                     "         may require the file to grow which is not "
+                     "supported for protocol '%s'.\n"
+                     "         Please open it read-only or try to export "
+                     "it in a different format\n"
+                     "         over the protocol (e.g. use qemu-nbd "
+                     "instead of nbd-server).",
+                     filename ?: "", drv->format_name,
+                     bs->file->drv->format_name);
+    }
+
     if (file && (bs->file != file)) {
         bdrv_unref(file);
         file = NULL;
diff --git a/block/qcow.c b/block/qcow.c
index 0558969..5719285 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -962,6 +962,7 @@ static BlockDriver bdrv_qcow = {
     .bdrv_create            = qcow_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
     .supports_backing       = true,
+    .requires_growing_file  = true,
 
     .bdrv_co_readv          = qcow_co_readv,
     .bdrv_co_writev         = qcow_co_writev,
diff --git a/block/qcow2.c b/block/qcow2.c
index b4e2aa3..ad1028b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2944,6 +2944,8 @@ BlockDriver bdrv_qcow2 = {
     .supports_backing           = true,
     .bdrv_change_backing_file   = qcow2_change_backing_file,
 
+    .requires_growing_file      = true,
+
     .bdrv_refresh_limits        = qcow2_refresh_limits,
     .bdrv_invalidate_cache      = qcow2_invalidate_cache,
 
diff --git a/block/qed.c b/block/qed.c
index 892b13c..1cd1233 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1662,6 +1662,7 @@ static BlockDriver bdrv_qed = {
     .instance_size            = sizeof(BDRVQEDState),
     .create_opts              = &qed_create_opts,
     .supports_backing         = true,
+    .requires_growing_file    = true,
 
     .bdrv_probe               = bdrv_qed_probe,
     .bdrv_rebind              = bdrv_qed_rebind,
diff --git a/block/vdi.c b/block/vdi.c
index 53bd02f..13ffd91 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -903,6 +903,8 @@ static BlockDriver bdrv_vdi = {
 
     .create_opts = &vdi_create_opts,
     .bdrv_check = vdi_check,
+
+    .requires_growing_file = true,
 };
 
 static void bdrv_vdi_init(void)
diff --git a/block/vhdx.c b/block/vhdx.c
index bb3ed45..bc1aaca 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1970,6 +1970,8 @@ static BlockDriver bdrv_vhdx = {
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
 
     .create_opts            = &vhdx_create_opts,
+
+    .requires_growing_file  = true,
 };
 
 static void bdrv_vhdx_init(void)
diff --git a/block/vmdk.c b/block/vmdk.c
index 8410a15..31ae764 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2293,6 +2293,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_attach_aio_context      = vmdk_attach_aio_context,
 
     .supports_backing             = true,
+    .requires_growing_file        = true,
     .create_opts                  = &vmdk_create_opts,
 };
 
diff --git a/block/vpc.c b/block/vpc.c
index 43e768e..4c26273 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -924,6 +924,8 @@ static BlockDriver bdrv_vpc = {
 
     .create_opts            = &vpc_create_opts,
     .bdrv_has_zero_init     = vpc_has_zero_init,
+
+    .requires_growing_file  = true,
 };
 
 static void bdrv_vpc_init(void)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 01dee2a..be44c80 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -109,6 +109,10 @@ struct BlockDriver {
     /* Set if a driver can support backing files */
     bool supports_backing;
 
+    /* Set if the underlying BDS.file needs to support writes beyond its size to
+     * be grown implicitly in order for this driver's BDSs to be writable */
+    bool requires_growing_file;
+
     /* For handling image reopen for split or non-split files */
     int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
                                BlockReopenQueue *queue, Error **errp);
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 3/3] block: Introduce BlockDriver.requires_growing_file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

On 03/19/2015 01:03 PM, Max Reitz wrote:
> This flag is set if write accesses to BDSs managed by the respective
> driver may lead to writes beyond the end of the underlying file BDS,
> expecting it to increase its size accordingly.
> 
> This behavior, however, is only supported by protocol BDSs having the
> BDS.growing flag set. If they do not, emit a warning.
> 
> An example of such a misconfiguration is exporting a qcow2 file using
> the reference NBD server implementation, which cannot interpret the
> qcow2 format and will thus expose it directly over the network. When
> accessing that export with qemu, qemu's qcow2 driver may try to allocate
> clusters over the NBD connection, writing to addresses beyond the size
> of the NBD export, which will then fail (without the user knowing why,
> without this warning).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   | 16 ++++++++++++++++
>  block/qcow.c              |  1 +
>  block/qcow2.c             |  2 ++
>  block/qed.c               |  1 +
>  block/vdi.c               |  2 ++
>  block/vhdx.c              |  2 ++
>  block/vmdk.c              |  1 +
>  block/vpc.c               |  2 ++
>  include/block/block_int.h |  4 ++++
>  9 files changed, 31 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Thu, Mar 19, 2015 at 03:03:18PM -0400, Max Reitz wrote:
> Some image formats (e.g. qcow2) require the underlying file to grow on
> write accesses, but this is in fact not supported by all protocols (e.g.
> nbd does not). If such a format requiring file growth is used
> non-read-only over a protocol which does not support this, a warning
> should be issued.
> 
> This warning is issued for example whenever one tries to export a qcow2
> image over nbd-server and use the export from qemu.

The warning implies that the user should switch to read-only or a
different protocol, but this configuration is perfectly normal.  For
example, oVirt uses qcow2 on LVM volumes.

Introducing a warning for a normal QEMU invocation is a bit weird.

What is the point of this series?  Were users confused that they hit
ENOSPC?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Max Reitz]

On 05.05.2015 11:46, Stefan Hajnoczi wrote:
> On Thu, Mar 19, 2015 at 03:03:18PM -0400, Max Reitz wrote:
>> Some image formats (e.g. qcow2) require the underlying file to grow on
>> write accesses, but this is in fact not supported by all protocols (e.g.
>> nbd does not). If such a format requiring file growth is used
>> non-read-only over a protocol which does not support this, a warning
>> should be issued.
>>
>> This warning is issued for example whenever one tries to export a qcow2
>> image over nbd-server and use the export from qemu.
> The warning implies that the user should switch to read-only or a
> different protocol, but this configuration is perfectly normal.  For
> example, oVirt uses qcow2 on LVM volumes.
>
> Introducing a warning for a normal QEMU invocation is a bit weird.
>
> What is the point of this series?  Were users confused that they hit
> ENOSPC?

Users were confused when exporting a qcow2 image using nbd-server 
instead of qemu-img, and then accessing that NBD export with qemu 
(subsequently getting I/O errors on guest writes, if the image is not 
yet fully allocated): http://bugzilla.redhat.com/show_bug.cgi?id=1090713

Max

Re: [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 06/05/2015 15:04, Max Reitz wrote:
>>
>> Introducing a warning for a normal QEMU invocation is a bit weird.
>>
>> What is the point of this series?  Were users confused that they hit
>> ENOSPC?
> 
> Users were confused when exporting a qcow2 image using nbd-server
> instead of qemu-img, and then accessing that NBD export with qemu
> (subsequently getting I/O errors on guest writes, if the image is not
> yet fully allocated): http://bugzilla.redhat.com/show_bug.cgi?id=1090713

I think NBD exports of non-raw images falls within the case of "user
should know what they're doing".  In particular, you don't even need
metadata preallocation, you just need a "truncate -s10G file.qcow2"
before invoking nbd-server.

So I think it's not worth fixing this, even though I see how it can be a
minor UI/UX issue.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Max Reitz]

On 06.05.2015 17:30, Paolo Bonzini wrote:
>
> On 06/05/2015 15:04, Max Reitz wrote:
>>> Introducing a warning for a normal QEMU invocation is a bit weird.
>>>
>>> What is the point of this series?  Were users confused that they hit
>>> ENOSPC?
>> Users were confused when exporting a qcow2 image using nbd-server
>> instead of qemu-img, and then accessing that NBD export with qemu
>> (subsequently getting I/O errors on guest writes, if the image is not
>> yet fully allocated): http://bugzilla.redhat.com/show_bug.cgi?id=1090713
> I think NBD exports of non-raw images falls within the case of "user
> should know what they're doing".  In particular, you don't even need
> metadata preallocation, you just need a "truncate -s10G file.qcow2"
> before invoking nbd-server.

Well, actually, you need to export it with qemu-nbd instead of 
nbd-server; which this series is trying to tell the users.

> So I think it's not worth fixing this, even though I see how it can be a
> minor UI/UX issue.

I very much think it would be worth fixing, if there wasn't the problem 
with legitimate use cases throwing unnecessary warnings.

I remember having a discussion with Kevin about this series (v1) 
regarding qcow2 on LVM; I think my point was that the warning is 
basically still correct, or only needs rewording (oops, I guess I did 
forget that in v2). If you are using qcow2 on LVM, you need to know 
exactly what you are doing, so a warning about this is indeed 
appropriate (in my opinion, that is).

So I think if we can word the warning in a way to make it clear that 
there are legitimate use cases, but you need to know what you are doing, 
I think it's worth having this warning. Users who know what they're 
doing won't be surprised or at least will know what it means, while 
users who don't know what it means most probably don't know what they're 
doing and thus the warning is appropriate for them.

And if you're using management software (which hopefully does know what 
it's doing), the warning shouldn't be prominently visible anyway (in 
case of using libvirt, stderr is written to a log file, right?).

Max

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 06/05/2015 18:12, Max Reitz wrote:
> I very much think it would be worth fixing, if there wasn't the problem
> with legitimate use cases throwing unnecessary warnings.

Right.

> I remember having a discussion with Kevin about this series (v1)
> regarding qcow2 on LVM; I think my point was that the warning is
> basically still correct, or only needs rewording (oops, I guess I did
> forget that in v2). If you are using qcow2 on LVM, you need to know
> exactly what you are doing, so a warning about this is indeed
> appropriate (in my opinion, that is).

There's another thing to check.  In the BZ you linked you got an EINVAL
or EIO.  Why didn't you get an ENOSPC?  Can you check if virtio-scsi
gives ENOSPC?

If so, you could perhaps only warn for werror=report.  But even then,
there are legitimate cases where you want the guest to see the ENOSPC.
In fact, that's the reason why virtio-scsi converts ENOSPC to a SCSI
"SPACE ALLOCATION FAILED" sense code. :)

> So I think if we can word the warning in a way to make it clear that
> there are legitimate use cases, but you need to know what you are doing,
> I think it's worth having this warning. Users who know what they're
> doing won't be surprised or at least will know what it means, while
> users who don't know what it means most probably don't know what they're
> doing and thus the warning is appropriate for them.

I don't know...  But then, I'm not a maintainer of this code. :)

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Max Reitz]

On 06.05.2015 18:20, Paolo Bonzini wrote:
>
> On 06/05/2015 18:12, Max Reitz wrote:
>> I very much think it would be worth fixing, if there wasn't the problem
>> with legitimate use cases throwing unnecessary warnings.
> Right.
>
>> I remember having a discussion with Kevin about this series (v1)
>> regarding qcow2 on LVM; I think my point was that the warning is
>> basically still correct, or only needs rewording (oops, I guess I did
>> forget that in v2). If you are using qcow2 on LVM, you need to know
>> exactly what you are doing, so a warning about this is indeed
>> appropriate (in my opinion, that is).
> There's another thing to check.  In the BZ you linked you got an EINVAL
> or EIO.  Why didn't you get an ENOSPC?

Because qcow2 tries to write beyond the end of the file; the NBD client 
implementation passes that on to the server, and the server simply 
reports an error (which the NBD client turns into EIO).

We could make the NBD client detect this condition and report ENOSPC 
immediately. But I don't think this would improve matters, for people 
would then complain "Linux reports 'no space left on device' in qemu, 
while df reports that there is enough space available". It's the same 
thing, people don't know what they're doing and nobody warned them that 
what they are doing might be wrong.

> Can you check if virtio-scsi
> gives ENOSPC?

In which configuration? Using virtio-scsi on top of qcow2 on top of some 
SCSI passthrough block driver?

> If so, you could perhaps only warn for werror=report.  But even then,
> there are legitimate cases where you want the guest to see the ENOSPC.
> In fact, that's the reason why virtio-scsi converts ENOSPC to a SCSI
> "SPACE ALLOCATION FAILED" sense code. :)

Sounds like we ought to make NBD return ENOSPC no matter the fate of 
this series.

The problem with only warning for a certain non-default configuration is 
that people who don't know what they are doing are more likely to use 
the default configuration, so I'd like the warning to appear then.

>> So I think if we can word the warning in a way to make it clear that
>> there are legitimate use cases, but you need to know what you are doing,
>> I think it's worth having this warning. Users who know what they're
>> doing won't be surprised or at least will know what it means, while
>> users who don't know what it means most probably don't know what they're
>> doing and thus the warning is appropriate for them.
> I don't know...  But then, I'm not a maintainer of this code. :)

Well, this is not about this code in particular, but more about qemu's 
interface design in general, so I'm grateful about any opinion on it. :-)

Max

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 06/05/2015 18:37, Max Reitz wrote:
> Because qcow2 tries to write beyond the end of the file; the NBD client
> implementation passes that on to the server, and the server simply
> reports an error (which the NBD client turns into EIO).

Where?

    qemu_coroutine_yield();
    *reply = s->reply;
    if (reply->handle != request->handle) {
        reply->error = EIO;
    } else {
        if (qiov && reply->error == 0) {
            ret = qemu_co_recvv(s->sock, qiov->iov, qiov->niov,
                                offset, request->len);
            if (ret != request->len) {
                reply->error = EIO;
            }
        }

        /* Tell the read handler to read another header.  */
        s->reply.handle = 0;
    }

It should get into the "else" and then see reply->error != 0.  So the
guest should see ENOSPC.

>> Can you check if virtio-scsi
>> gives ENOSPC?
> 
> In which configuration? Using virtio-scsi on top of qcow2 on top of some
> SCSI passthrough block driver?  Sounds like we ought to make NBD return
> ENOSPC no matter the fate of this series.

virtio-scsi on top of qcow2 on top of NBD.  No passthrough: "-device
scsi-disk" should work.  It seems to me that NBD _should_ be getting
ENOSPC, unless it's nbd-server that throws away the error.

> The problem with only warning for a certain non-default configuration is
> that people who don't know what they are doing are more likely to use
> the default configuration, so I'd like the warning to appear then.

That's entirely true.  Perhaps the werror default should be changed from
enospc to report after all!

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Max Reitz]

On 06.05.2015 18:47, Paolo Bonzini wrote:
>
> On 06/05/2015 18:37, Max Reitz wrote:
>> Because qcow2 tries to write beyond the end of the file; the NBD client
>> implementation passes that on to the server, and the server simply
>> reports an error (which the NBD client turns into EIO).
> Where?
>
>      qemu_coroutine_yield();
>      *reply = s->reply;
>      if (reply->handle != request->handle) {
>          reply->error = EIO;
>      } else {
>          if (qiov && reply->error == 0) {
>              ret = qemu_co_recvv(s->sock, qiov->iov, qiov->niov,
>                                  offset, request->len);
>              if (ret != request->len) {
>                  reply->error = EIO;
>              }
>          }
>
>          /* Tell the read handler to read another header.  */
>          s->reply.handle = 0;
>      }
>
> It should get into the "else" and then see reply->error != 0.  So the
> guest should see ENOSPC.

The guest sees whatever has been written into reply->error, and that 
field hasn't been written by this function in that case. It has been 
written by nbd_receive_reply() in nbd.c, and that value comes directly 
from the server. In case of qemu-nbd being the server, a write beyond 
the EOF should be caught by blk_check_byte_request() in 
block/block-backend.c, which returns -EIO. So that's where the EIO comes 
from.

I don't know whether this EIO is subsequently converted to ENOSPC 
because of werror=enospc, but considering that 
https://bugzilla.redhat.com/show_bug.cgi?id=1090713 did not override 
werror, it doesn't look like it.

>>> Can you check if virtio-scsi
>>> gives ENOSPC?
>> In which configuration? Using virtio-scsi on top of qcow2 on top of some
>> SCSI passthrough block driver?  Sounds like we ought to make NBD return
>> ENOSPC no matter the fate of this series.
> virtio-scsi on top of qcow2 on top of NBD.  No passthrough: "-device
> scsi-disk" should work.  It seems to me that NBD _should_ be getting
> ENOSPC, unless it's nbd-server that throws away the error.

Well, nbd-server will return whatever it feels like, I don't know about 
that. qemu-nbd on the other hand I do know about, and as written above, 
it will return EIO because of the check performed from blk_write(). So 
that error is sent to the client, where the nbd BDS will return it, too, 
and then qcow2 will propagate it up to virtio-scsi. It looks to me like 
scsi_handle_rw_error() will then call scsi_check_condition(r, 
SENSE_CODE(IO_ERROR)).

(Note that I think werror is only used for block jobs, but not for 
normal guest operations, so EIO coming from a BDS is not converted to 
ENOSPC for guest requests)

I can test these assumptions, if you'd like me to, but that will have to 
wait until Friday. :-)

Max

>> The problem with only warning for a certain non-default configuration is
>> that people who don't know what they are doing are more likely to use
>> the default configuration, so I'd like the warning to appear then.
> That's entirely true.  Perhaps the werror default should be changed from
> enospc to report after all!
>
> Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 06/05/2015 19:23, Max Reitz wrote:
> The guest sees whatever has been written into reply->error, and that
> field hasn't been written by this function in that case. It has been
> written by nbd_receive_reply() in nbd.c, and that value comes directly
> from the server. In case of qemu-nbd being the server, a write beyond
> the EOF should be caught by blk_check_byte_request() in
> block/block-backend.c, which returns -EIO. So that's where the EIO comes
> from.

Fair enough.  This makes sense, but then we have to create ENOSPC elsewhere.

> I don't know whether this EIO is subsequently converted to ENOSPC
> because of werror=enospc, but considering that
> https://bugzilla.redhat.com/show_bug.cgi?id=1090713 did not override
> werror, it doesn't look like it.

No, it shouldn't indeed.

Could alloc_clusters_noref do bdrv_truncate and return ENOSPC if it
fails?  That's how for example qcow and vhdx work.  vdi has the same
problem.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Kevin Wolf]

Am 07.05.2015 um 14:20 hat Paolo Bonzini geschrieben:
> 
> 
> On 06/05/2015 19:23, Max Reitz wrote:
> > The guest sees whatever has been written into reply->error, and that
> > field hasn't been written by this function in that case. It has been
> > written by nbd_receive_reply() in nbd.c, and that value comes directly
> > from the server. In case of qemu-nbd being the server, a write beyond
> > the EOF should be caught by blk_check_byte_request() in
> > block/block-backend.c, which returns -EIO. So that's where the EIO comes
> > from.
> 
> Fair enough.  This makes sense, but then we have to create ENOSPC elsewhere.
> 
> > I don't know whether this EIO is subsequently converted to ENOSPC
> > because of werror=enospc, but considering that
> > https://bugzilla.redhat.com/show_bug.cgi?id=1090713 did not override
> > werror, it doesn't look like it.
> 
> No, it shouldn't indeed.
> 
> Could alloc_clusters_noref do bdrv_truncate and return ENOSPC if it
> fails?  That's how for example qcow and vhdx work.  vdi has the same
> problem.

If you want NBD to return -ENOSPC for writes after EOF, change the
nbd block driver to do just that. There's no reason to add additional
overhead to qcow2 (even over raw-posix) for that.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 07/05/2015 14:29, Kevin Wolf wrote:
> > No, it shouldn't indeed.
> > 
> > Could alloc_clusters_noref do bdrv_truncate and return ENOSPC if it
> > fails?  That's how for example qcow and vhdx work.  vdi has the same
> > problem.
> 
> If you want NBD to return -ENOSPC for writes after EOF, change the
> nbd block driver to do just that. There's no reason to add additional
> overhead to qcow2 (even over raw-posix) for that.

Does ENOSPC over LVM (dm-linear) work at all, and who generates the
ENOSPC there?

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Kevin Wolf]

Am 07.05.2015 um 14:47 hat Paolo Bonzini geschrieben:
> 
> 
> On 07/05/2015 14:29, Kevin Wolf wrote:
> > > No, it shouldn't indeed.
> > > 
> > > Could alloc_clusters_noref do bdrv_truncate and return ENOSPC if it
> > > fails?  That's how for example qcow and vhdx work.  vdi has the same
> > > problem.
> > 
> > If you want NBD to return -ENOSPC for writes after EOF, change the
> > nbd block driver to do just that. There's no reason to add additional
> > overhead to qcow2 (even over raw-posix) for that.
> 
> Does ENOSPC over LVM (dm-linear) work at all, and who generates the
> ENOSPC there?

The LVM use case is what oVirt uses, so I'm pretty sure that it works.
I'm now sure who generates the ENOSPC, but it's not qemu anyway. If I
had to guess, I'd say that the kernel block layer might just forbid
writing after EOF for any block device.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 07/05/2015 15:20, Kevin Wolf wrote:
> > Does ENOSPC over LVM (dm-linear) work at all, and who generates the
> > ENOSPC there?
>
> The LVM use case is what oVirt uses, so I'm pretty sure that it works.
> I'm now sure who generates the ENOSPC, but it's not qemu anyway. If I
> had to guess, I'd say that the kernel block layer might just forbid
> writing after EOF for any block device.

Indeed, though it's VFS (blkdev_write_iter in fs/block_dev.c) and not
the block layer.  It looks like we need this:

diff --git a/block/block-backend.c b/block/block-backend.c
index 93e46f3..e54c433 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -461,7 +461,7 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
     }
 
     if (offset > len || len - offset < size) {
-        return -EIO;
+        return -ENOSPC;
     }
 
     return 0;

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Kevin Wolf]

Am 07.05.2015 um 15:55 hat Paolo Bonzini geschrieben:
> 
> 
> On 07/05/2015 15:20, Kevin Wolf wrote:
> > > Does ENOSPC over LVM (dm-linear) work at all, and who generates the
> > > ENOSPC there?
> >
> > The LVM use case is what oVirt uses, so I'm pretty sure that it works.
> > I'm now sure who generates the ENOSPC, but it's not qemu anyway. If I
> > had to guess, I'd say that the kernel block layer might just forbid
> > writing after EOF for any block device.
> 
> Indeed, though it's VFS (blkdev_write_iter in fs/block_dev.c) and not
> the block layer.  It looks like we need this:
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..e54c433 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -461,7 +461,7 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
>      }
>  
>      if (offset > len || len - offset < size) {
> -        return -EIO;
> +        return -ENOSPC;
>      }

This is not right for two reasons: The first is that this is
BlockBackend code and it wouldn't even take effect for the qcow2 case
where we're writing past EOF only on the protocol layer. The second is
that -ENOSPC is only for writes and not for reads.

For the protocol level, bdrv_aligned_preadv() has code to handle reads
past EOF if bs->zero_beyond_eof is set. This is always the case, except
for qcow2, which has the snapshot VM state after EOF, so the driver is
called for that.

For writes, the driver is always called. The expectiation is that beyond
EOF it resizes the image file if it can, and returns -ENOSPC if it can't.
We could change this to have a check directly in bdrv_aligned_pwrite()
and then drivers would have to advertise whether they can extend a file
beyond EOF or not so we know whether to apply the check or not
(essentially the growable flag that Max wants to add), but I'm not sure
what we would win with that.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 07/05/2015 16:07, Kevin Wolf wrote:
> This is not right for two reasons: The first is that this is
> BlockBackend code

I think it would take effect for the qemu-nbd case though.

> and it wouldn't even take effect for the qcow2 case
> where we're writing past EOF only on the protocol layer. The second is
> that -ENOSPC is only for writes and not for reads.

This is right.

Reads in the kernel return 0, but in QEMU we do not want that.  The code
currently returns -EIO, but perhaps -EINVAL is a better match.  It also
happens to be what Linux returns for discards.

Paolo

> For the protocol level, bdrv_aligned_preadv() has code to handle reads
> past EOF if bs->zero_beyond_eof is set. This is always the case, except
> for qcow2, which has the snapshot VM state after EOF, so the driver is
> called for that.
> 
> For writes, the driver is always called. The expectiation is that beyond
> EOF it resizes the image file if it can, and returns -ENOSPC if it can't.
> We could change this to have a check directly in bdrv_aligned_pwrite()
> and then drivers would have to advertise whether they can extend a file
> beyond EOF or not so we know whether to apply the check or not
> (essentially the growable flag that Max wants to add), but I'm not sure
> what we would win with that.

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Kevin Wolf]

Am 07.05.2015 um 16:16 hat Paolo Bonzini geschrieben:
> 
> 
> On 07/05/2015 16:07, Kevin Wolf wrote:
> > This is not right for two reasons: The first is that this is
> > BlockBackend code
> 
> I think it would take effect for the qemu-nbd case though.

Oh, you want to change the server code rather than the client?

Wait... Are you saying that NBD sends a (platform specific) errno value
over the network? :-/

In theory, what error code the NBD server needs to send should be
specified by the NBD protocol. Am I right to assume that it doesn't do
that? In any case, I'm not sure whether qemu's internal error code
should change just for NBD. Producing the right error code for the
protocol is the job of nbd_co_receive_request().

> > and it wouldn't even take effect for the qcow2 case
> > where we're writing past EOF only on the protocol layer. The second is
> > that -ENOSPC is only for writes and not for reads.
> 
> This is right.
> 
> Reads in the kernel return 0, but in QEMU we do not want that.  The code
> currently returns -EIO, but perhaps -EINVAL is a better match.  It also
> happens to be what Linux returns for discards.

Perhaps it is, yes. It shouldn't make a difference for guests anyway.
(Unlike -ENOSPC for writes, which would trigger werror=enospc! That's
most likely not what we want.)

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 07/05/2015 16:34, Kevin Wolf wrote:
> Am 07.05.2015 um 16:16 hat Paolo Bonzini geschrieben:
>>
>>
>> On 07/05/2015 16:07, Kevin Wolf wrote:
>>> This is not right for two reasons: The first is that this is
>>> BlockBackend code
>>
>> I think it would take effect for the qemu-nbd case though.
> 
> Oh, you want to change the server code rather than the client?

Yes.

> Wait... Are you saying that NBD sends a (platform specific) errno value
> over the network? :-/

Yes. :/  That said, at least the error codes that Linux places in
/usr/include/asm/errno-base.h seem to be pretty much standard---at least
Windows and most Unices share them---with the exception of EAGAIN.

I'll send a patch to NBD to standardize the set of error codes that it
sends.

> In theory, what error code the NBD server needs to send should be
> specified by the NBD protocol. Am I right to assume that it doesn't do
> that?

Nope.

> In any case, I'm not sure whether qemu's internal error code
> should change just for NBD. Producing the right error code for the
> protocol is the job of nbd_co_receive_request().

Ok, so it shouldn't reach blk_check_request at all.  But then, we should
aim at making blk_check_request's checks assertions.

>>> and it wouldn't even take effect for the qcow2 case
>>> where we're writing past EOF only on the protocol layer. The second is
>>> that -ENOSPC is only for writes and not for reads.
>>
>> This is right.
>>
>> Reads in the kernel return 0, but in QEMU we do not want that.  The code
>> currently returns -EIO, but perhaps -EINVAL is a better match.  It also
>> happens to be what Linux returns for discards.
> 
> Perhaps it is, yes. It shouldn't make a difference for guests anyway.
> (Unlike -ENOSPC for writes, which would trigger werror=enospc! That's
> most likely not what we want.)

Yes, we want the check duplicated in all BlockBackend users.  Most of
them already do it, see the work that Markus did last year I think.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Kevin Wolf]

Am 07.05.2015 um 16:50 hat Paolo Bonzini geschrieben:
> On 07/05/2015 16:34, Kevin Wolf wrote:
> > Am 07.05.2015 um 16:16 hat Paolo Bonzini geschrieben:
> >>
> >>
> >> On 07/05/2015 16:07, Kevin Wolf wrote:
> >>> This is not right for two reasons: The first is that this is
> >>> BlockBackend code
> >>
> >> I think it would take effect for the qemu-nbd case though.
> > 
> > Oh, you want to change the server code rather than the client?
> 
> Yes.

Actually, considering all the information in this thread, I'm inclined
that we should change both sides. qemu-nbd because ENOSPC might be what
clients expect by analogy with Linux block devices, even if the
behaviour for accesses beyond the device size isn't specified in the NBD
protocol and the server might just do anything. As long as the behaviour
is undefined, it's nice to do what most people may expect.

And as the real fix change the nbd client, because even if new qemu-nbd
versions will be nice, we shouldn't rely on undefined behaviour. We know
that old qemu-nbd servers won't produce ENOSPC and I'm not sure what
other NBD servers do.

> > Wait... Are you saying that NBD sends a (platform specific) errno value
> > over the network? :-/
> 
> Yes. :/  That said, at least the error codes that Linux places in
> /usr/include/asm/errno-base.h seem to be pretty much standard---at least
> Windows and most Unices share them---with the exception of EAGAIN.
> 
> I'll send a patch to NBD to standardize the set of error codes that it
> sends.

Thanks, that will be helpful in the future.

Is this the right place to look up the spec?
http://sourceforge.net/p/nbd/code/ci/master/tree/doc/proto.txt

If so, the commands seem to be hopelessly underspecified, especially
with respect to error conditions. And where it says something about
errors, it doesn't make sense: The server is forbidden to reply to a
NBD_CMD_FLUSH if it failed... (qemu-nbd ignores this, obviously)

> > In theory, what error code the NBD server needs to send should be
> > specified by the NBD protocol. Am I right to assume that it doesn't do
> > that?
> 
> Nope.
> 
> > In any case, I'm not sure whether qemu's internal error code
> > should change just for NBD. Producing the right error code for the
> > protocol is the job of nbd_co_receive_request().
> 
> Ok, so it shouldn't reach blk_check_request at all.  But then, we should
> aim at making blk_check_request's checks assertions.

Sounds fair as a goal, but I don't think all devices have such checks
yet. We've fixed the most common devices (IDE, scsi-disk and virtio-blk)
just a while ago.

> >>> and it wouldn't even take effect for the qcow2 case
> >>> where we're writing past EOF only on the protocol layer. The second is
> >>> that -ENOSPC is only for writes and not for reads.
> >>
> >> This is right.
> >>
> >> Reads in the kernel return 0, but in QEMU we do not want that.  The code
> >> currently returns -EIO, but perhaps -EINVAL is a better match.  It also
> >> happens to be what Linux returns for discards.
> > 
> > Perhaps it is, yes. It shouldn't make a difference for guests anyway.
> > (Unlike -ENOSPC for writes, which would trigger werror=enospc! That's
> > most likely not what we want.)
> 
> Yes, we want the check duplicated in all BlockBackend users.  Most of
> them already do it, see the work that Markus did last year I think.

I wouldn't call it duplicated because the action to take is different
for each device, but yes, the check belongs there.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 08/05/2015 12:08, Kevin Wolf wrote:
> Actually, considering all the information in this thread, I'm inclined
> that we should change both sides. qemu-nbd because ENOSPC might be what
> clients expect by analogy with Linux block devices, even if the
> behaviour for accesses beyond the device size isn't specified in the NBD
> protocol and the server might just do anything. As long as the behaviour
> is undefined, it's nice to do what most people may expect.
> 
> And as the real fix change the nbd client, because even if new qemu-nbd
> versions will be nice, we shouldn't rely on undefined behaviour. We know
> that old qemu-nbd servers won't produce ENOSPC and I'm not sure what
> other NBD servers do.

Sounds like a plan.

> Thanks, that will be helpful in the future.
> 
> Is this the right place to look up the spec?
> http://sourceforge.net/p/nbd/code/ci/master/tree/doc/proto.txt

Yes.

> If so, the commands seem to be hopelessly underspecified, especially
> with respect to error conditions. And where it says something about
> errors, it doesn't make sense: The server is forbidden to reply to a
> NBD_CMD_FLUSH if it failed... (qemu-nbd ignores this, obviously)

So does nbd-server. O:-)  Looks like you're reading the spec too
literally (which is never a bad thing).

>> Ok, so it shouldn't reach blk_check_request at all.  But then, we should
>> aim at making blk_check_request's checks assertions.
> 
> Sounds fair as a goal, but I don't think all devices have such checks
> yet. We've fixed the most common devices (IDE, scsi-disk and virtio-blk)
> just a while ago.

Indeed ("aim at").

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Kevin Wolf]

Am 08.05.2015 um 12:16 hat Paolo Bonzini geschrieben:
> On 08/05/2015 12:08, Kevin Wolf wrote:
> > If so, the commands seem to be hopelessly underspecified, especially
> > with respect to error conditions. And where it says something about
> > errors, it doesn't make sense: The server is forbidden to reply to a
> > NBD_CMD_FLUSH if it failed... (qemu-nbd ignores this, obviously)
> 
> So does nbd-server. O:-)  Looks like you're reading the spec too
> literally (which is never a bad thing).

I don't think there is something like reading a spec too literally.
Specs are meant to be read literally. If a specification is open to
interpretation, you don't need it. So I'd rather say I've found a bug
in the spec. ;-)

As you already seem to be working on the NBD mailing list, do you want
to fix this, or should I subscribe and send a patch myself?

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Paolo Bonzini]

On 08/05/2015 12:34, Kevin Wolf wrote:
> Am 08.05.2015 um 12:16 hat Paolo Bonzini geschrieben:
>> On 08/05/2015 12:08, Kevin Wolf wrote:
>>> If so, the commands seem to be hopelessly underspecified, especially
>>> with respect to error conditions. And where it says something about
>>> errors, it doesn't make sense: The server is forbidden to reply to a
>>> NBD_CMD_FLUSH if it failed... (qemu-nbd ignores this, obviously)
>>
>> So does nbd-server. O:-)  Looks like you're reading the spec too
>> literally (which is never a bad thing).
> 
> I don't think there is something like reading a spec too literally.
> Specs are meant to be read literally. If a specification is open to
> interpretation, you don't need it. So I'd rather say I've found a bug
> in the spec. ;-)

You have.  The bug is a single missing word ("successful") reply, but it
is still a bug.

There is another bug, in that it talks about "outstanding" writes rather
than completed" writes.

> As you already seem to be working on the NBD mailing list, do you want
> to fix this, or should I subscribe and send a patch myself?

You've been CCed on the fix.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols

[Max Reitz]

On 08.05.2015 12:08, Kevin Wolf wrote:
> Am 07.05.2015 um 16:50 hat Paolo Bonzini geschrieben:
>> On 07/05/2015 16:34, Kevin Wolf wrote:
>>> Am 07.05.2015 um 16:16 hat Paolo Bonzini geschrieben:
>>>>
>>>> On 07/05/2015 16:07, Kevin Wolf wrote:
>>>>> This is not right for two reasons: The first is that this is
>>>>> BlockBackend code
>>>> I think it would take effect for the qemu-nbd case though.
>>> Oh, you want to change the server code rather than the client?
>> Yes.
> Actually, considering all the information in this thread, I'm inclined
> that we should change both sides. qemu-nbd because ENOSPC might be what
> clients expect by analogy with Linux block devices, even if the
> behaviour for accesses beyond the device size isn't specified in the NBD
> protocol and the server might just do anything. As long as the behaviour
> is undefined, it's nice to do what most people may expect.

It is practically defined by what the reference implementation does, and 
that is return EINVAL (as I said in the other thread), with the 
reasoning being that it's an invalid request. I concur, the client 
should simply not send a request beyond the export length, doing so is 
wrong. So it is the client that should catch this case and return ENOSPC.

> And as the real fix change the nbd client, because even if new qemu-nbd
> versions will be nice, we shouldn't rely on undefined behaviour. We know
> that old qemu-nbd servers won't produce ENOSPC and I'm not sure what
> other NBD servers do.

Return EINVAL.

Max

>>> Wait... Are you saying that NBD sends a (platform specific) errno value
>>> over the network? :-/
>> Yes. :/  That said, at least the error codes that Linux places in
>> /usr/include/asm/errno-base.h seem to be pretty much standard---at least
>> Windows and most Unices share them---with the exception of EAGAIN.
>>
>> I'll send a patch to NBD to standardize the set of error codes that it
>> sends.
> Thanks, that will be helpful in the future.
>
> Is this the right place to look up the spec?
> http://sourceforge.net/p/nbd/code/ci/master/tree/doc/proto.txt
>
> If so, the commands seem to be hopelessly underspecified, especially
> with respect to error conditions. And where it says something about
> errors, it doesn't make sense: The server is forbidden to reply to a
> NBD_CMD_FLUSH if it failed... (qemu-nbd ignores this, obviously)
>
>>> In theory, what error code the NBD server needs to send should be
>>> specified by the NBD protocol. Am I right to assume that it doesn't do
>>> that?
>> Nope.
>>
>>> In any case, I'm not sure whether qemu's internal error code
>>> should change just for NBD. Producing the right error code for the
>>> protocol is the job of nbd_co_receive_request().
>> Ok, so it shouldn't reach blk_check_request at all.  But then, we should
>> aim at making blk_check_request's checks assertions.
> Sounds fair as a goal, but I don't think all devices have such checks
> yet. We've fixed the most common devices (IDE, scsi-disk and virtio-blk)
> just a while ago.
>
>>>>> and it wouldn't even take effect for the qcow2 case
>>>>> where we're writing past EOF only on the protocol layer. The second is
>>>>> that -ENOSPC is only for writes and not for reads.
>>>> This is right.
>>>>
>>>> Reads in the kernel return 0, but in QEMU we do not want that.  The code
>>>> currently returns -EIO, but perhaps -EINVAL is a better match.  It also
>>>> happens to be what Linux returns for discards.
>>> Perhaps it is, yes. It shouldn't make a difference for guests anyway.
>>> (Unlike -ENOSPC for writes, which would trigger werror=enospc! That's
>>> most likely not what we want.)
>> Yes, we want the check duplicated in all BlockBackend users.  Most of
>> them already do it, see the work that Markus did last year I think.
> I wouldn't call it duplicated because the action to take is different
> for each device, but yes, the check belongs there.
>
> Kevin