Message ID | 1431080484-9760-1-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
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) {
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
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) {
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(-)