Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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
Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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-block] [Qemu-devel] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
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 pgptGtY8wVrcK.pgp Description: PGP signature
