[Qemu-devel] [PATCH] qemu-nbd: return ENOSPC for out-of-range writes

[Qemu-devel] [PATCH] qemu-nbd: return ENOSPC for out-of-range writes

[Paolo Bonzini]

This ensures that werror=enospc works fine for NBD-backed devices.
Recovery can be done through live snapshots even if the NBD server
does not support online resizing.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/nbd.c b/nbd.c
index 57d71b2..a04ba80 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1297,7 +1297,8 @@ static void nbd_trip(void *opaque)
                     request.from, request.len,
                     (uint64_t)exp->size, (uint64_t)exp->dev_offset);
         LOG("requested operation past EOF--bad client?");
-        goto invalid_request;
+        reply.error = (command == NBD_CMD_WRITE) ? ENOSPC : EINVAL;
+        goto error_reply;
     }
 
     switch (command) {
@@ -1390,7 +1391,6 @@ static void nbd_trip(void *opaque)
         break;
     default:
         LOG("invalid request type (%u) received", request.type);
-    invalid_request:
         reply.error = EINVAL;
     error_reply:
         if (nbd_co_send_reply(req, &reply, 0) < 0) {
-- 
2.3.5

Re: [Qemu-devel] [PATCH] qemu-nbd: return ENOSPC for out-of-range writes

[Max Reitz]

On 08.05.2015 12:21, Paolo Bonzini wrote:
> This ensures that werror=enospc works fine for NBD-backed devices.
> Recovery can be done through live snapshots even if the NBD server
> does not support online resizing.
>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   nbd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Good thing: NBD doesn't have a very extensive manual, so in theory we 
can do whatever we want to.
Bad thing: Because of this, our implementation generally seems to look 
to the reference implementation (nbd-server).

Well, and that reference implementation handles this case here: 
https://github.com/yoe/nbd/blob/master/nbd-server.c#l1621 - so it does 
always send EINVAL.

Two things follow: First, our implementation will behave differently 
from nbd-server. Second, even if we don't care, werror=enospc will only 
work with qemu-nbd, and not when using nbd-server.

One could argue that you should only use qemu-nbd with qemu, and not 
nbd-server, but then I'm wondering why we implemented a pre-existing 
protocol in the first place and didn't implement our own...

I think this should be done in the client. If a request beyond the EOF 
(for writes) is to be generated, ENOSPC should be returned before making 
the request to the server at all.

Alternatively, we could actually use my series which introduces the 
debatable (and debated) warning to implement this case more generally: 
It does introduce a flag which tells the block layer whether a BDS 
supports write accesses beyond EOF or not, so using that the block layer 
itself would be able to intercept such writes and return ENOSPC 
automatically.

Max

> diff --git a/nbd.c b/nbd.c
> index 57d71b2..a04ba80 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -1297,7 +1297,8 @@ static void nbd_trip(void *opaque)
>                       request.from, request.len,
>                       (uint64_t)exp->size, (uint64_t)exp->dev_offset);
>           LOG("requested operation past EOF--bad client?");
> -        goto invalid_request;
> +        reply.error = (command == NBD_CMD_WRITE) ? ENOSPC : EINVAL;
> +        goto error_reply;
>       }
>   
>       switch (command) {
> @@ -1390,7 +1391,6 @@ static void nbd_trip(void *opaque)
>           break;
>       default:
>           LOG("invalid request type (%u) received", request.type);
> -    invalid_request:
>           reply.error = EINVAL;
>       error_reply:
>           if (nbd_co_send_reply(req, &reply, 0) < 0) {

Re: [Qemu-devel] [PATCH] qemu-nbd: return ENOSPC for out-of-range writes

[Paolo Bonzini]

On 08/05/2015 14:38, Max Reitz wrote:
> Two things follow: First, our implementation will behave differently
> from nbd-server. Second, even if we don't care, werror=enospc will only
> work with qemu-nbd, and not when using nbd-server.

I've sent a patch to fix this for nbd-server too.

However, for this reason, Kevin suggested fixing the client too.

Paolo