Branch: refs/heads/master Home: https://github.com/qemu/qemu Commit: 066eb006b54308be60fc2a435a04cde8f4187502 https://github.com/qemu/qemu/commit/066eb006b54308be60fc2a435a04cde8f4187502 Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-01 (Thu, 01 Feb 2018)
Changed paths: M hw/9pfs/9p.c M hw/9pfs/9p.h M hw/9pfs/virtio-9p-device.c M hw/9pfs/xen-9p-backend.c Log Message: ----------- 9pfs: drop v9fs_register_transport() No good reasons to do this outside of v9fs_device_realize_common(). Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Commit: fc78d5ee7622342699d9d9626c8df712f1486e07 https://github.com/qemu/qemu/commit/fc78d5ee7622342699d9d9626c8df712f1486e07 Author: Keno Fischer <k...@juliacomputing.com> Date: 2018-02-01 (Thu, 01 Feb 2018) Changed paths: M hw/9pfs/9p.c M hw/9pfs/trace-events Log Message: ----------- 9pfs: Correctly handle cancelled requests # Background I was investigating spurious non-deterministic EINTR returns from various 9p file system operations in a Linux guest served from the qemu 9p server. ## EINTR, ERESTARTSYS and the linux kernel When a signal arrives that the Linux kernel needs to deliver to user-space while a given thread is blocked (in the 9p case waiting for a reply to its request in 9p_client_rpc -> wait_event_interruptible), it asks whatever driver is currently running to abort its current operation (in the 9p case causing the submission of a TFLUSH message) and return to user space. In these situations, the error message reported is generally ERESTARTSYS. If the userspace processes specified SA_RESTART, this means that the system call will get restarted upon completion of the signal handler delivery (assuming the signal handler doesn't modify the process state in complicated ways not relevant here). If SA_RESTART is not specified, ERESTARTSYS gets translated to EINTR and user space is expected to handle the restart itself. ## The 9p TFLUSH command The 9p TFLUSH commands requests that the server abort an ongoing operation. The man page [1] specifies: ``` If it recognizes oldtag as the tag of a pending transaction, it should abort any pending response and discard that tag. [...] When the client sends a Tflush, it must wait to receive the corresponding Rflush before reusing oldtag for subsequent messages. If a response to the flushed request is received before the Rflush, the client must honor the response as if it had not been flushed, since the completed request may signify a state change in the server ``` In particular, this means that the server must not send a reply with the orignal tag in response to the cancellation request, because the client is obligated to interpret such a reply as a coincidental reply to the original request. # The bug When qemu receives a TFlush request, it sets the `cancelled` flag on the relevant pdu. This flag is periodically checked, e.g. in `v9fs_co_name_to_path`, and if set, the operation is aborted and the error is set to EINTR. However, the server then violates the spec, by returning to the client an Rerror response, rather than discarding the message entirely. As a result, the client is required to assume that said Rerror response is a result of the original request, not a result of the cancellation and thus passes the EINTR error back to user space. This is not the worst thing it could do, however as discussed above, the correct error code would have been ERESTARTSYS, such that user space programs with SA_RESTART set get correctly restarted upon completion of the signal handler. Instead, such programs get spurious EINTR results that they were not expecting to handle. It should be noted that there are plenty of user space programs that do not set SA_RESTART and do not correctly handle EINTR either. However, that is then a userspace bug. It should also be noted that this bug has been mitigated by a recent commit to the Linux kernel [2], which essentially prevents the kernel from sending Tflush requests unless the process is about to die (in which case the process likely doesn't care about the response). Nevertheless, for older kernels and to comply with the spec, I believe this change is beneficial. # Implementation The fix is fairly simple, just skipping notification of a reply if the pdu was previously cancelled. We do however, also notify the transport layer that we're doing this, so it can clean up any resources it may be holding. I also added a new trace event to distinguish operations that caused an error reply from those that were cancelled. One complication is that we only omit sending the message on EINTR errors in order to avoid confusing the rest of the code (which may assume that a client knows about a fid if it sucessfully passed it off to pud_complete without checking for cancellation status). This does mean that if the server acts upon the cancellation flag, it always needs to set err to EINTR. I believe this is true of the current code. [1] https://9fans.github.io/plan9port/man/man9/flush.html [2] https://github.com/torvalds/linux/commit/9523feac272ccad2ad8186ba4fcc891 Signed-off-by: Keno Fischer <k...@juliacomputing.com> Reviewed-by: Greg Kurz <gr...@kaod.org> [groug, send a zero-sized reply instead of detaching the buffer] Signed-off-by: Greg Kurz <gr...@kaod.org> Acked-by: Michael S. Tsirkin <m...@redhat.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Commit: 693b21d2c7e54474017a4b45d36faa767279d4d4 https://github.com/qemu/qemu/commit/693b21d2c7e54474017a4b45d36faa767279d4d4 Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-01 (Thu, 01 Feb 2018) Changed paths: M tests/virtio-9p-test.c Log Message: ----------- tests: virtio-9p: move request tag to the test functions It doesn't really makes sense to hide the request tag from the test functions. It prevents to test the 9p server behavior when passed a wrong tag (ie, still in use or different from P9_NOTAG for a version request). Also the spec says that a tag is reusable as soon as the corresponding request was replied or flushed: no need to always increment tags like we do now. And finaly, an upcoming test of the flush command will need to manipulate tags explicitely. This simply changes all request functions to have a tag argument. Except for the version request which needs P9_NOTAG, all other tests can pass 0 since they wait for the reply before sending another request. Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Commit: 60b1fa9de1513713807b49be8a9f25077d5ab2ed https://github.com/qemu/qemu/commit/60b1fa9de1513713807b49be8a9f25077d5ab2ed Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-01 (Thu, 01 Feb 2018) Changed paths: M tests/virtio-9p-test.c Log Message: ----------- tests: virtio-9p: wait for completion in the test code In order to test request cancellation, we will need to send multiple requests and wait for the associated replies. Since we poll the ISR to know if a request completed, we may have several replies to parse when we detect ISR was set to 1. This patch moves the waiting out of the reply parsing path, up into the functional tests. Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Commit: 2893ddd5988a38196e3ca72853985814de831672 https://github.com/qemu/qemu/commit/2893ddd5988a38196e3ca72853985814de831672 Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-01 (Thu, 01 Feb 2018) Changed paths: M hw/9pfs/9p-synth.c M hw/9pfs/9p-synth.h M tests/virtio-9p-test.c Log Message: ----------- tests: virtio-9p: use the synth backend The purpose of virtio-9p-test is to test the virtio-9p device, especially the 9p server state machine. We don't really care what fsdev backend we're using. Moreover, if we want to be able to test the flush request or a device reset with in-flights I/O, it is close to impossible to achieve with a physical backend because we cannot ask it reliably to put an I/O on hold at a specific point in time. Fortunately, we can do that with the synthetic backend, which allows to register callbacks on read/write accesses to a specific file. This will be used by a later patch to test the 9P flush request. The walk request test is converted to using the synth backend. Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Commit: 82469aaefea4f8e7a4469c3ec1f680bbf0341c98 https://github.com/qemu/qemu/commit/82469aaefea4f8e7a4469c3ec1f680bbf0341c98 Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-01 (Thu, 01 Feb 2018) Changed paths: M hw/9pfs/9p-synth.c M hw/9pfs/9p-synth.h M tests/virtio-9p-test.c Log Message: ----------- tests: virtio-9p: add LOPEN operation test Trivial test of a successful open. Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Commit: 354b86f85f516fecb60185f9c2b8e5933177b300 https://github.com/qemu/qemu/commit/354b86f85f516fecb60185f9c2b8e5933177b300 Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-01 (Thu, 01 Feb 2018) Changed paths: M hw/9pfs/9p-synth.c M hw/9pfs/9p-synth.h M tests/virtio-9p-test.c Log Message: ----------- tests: virtio-9p: add WRITE operation test Trivial test of a successful write. Signed-off-by: Greg Kurz <gr...@kaod.org> (groug, handle potential overflow when computing request size, add missing g_free(buf), backend handles one written byte at a time to validate the server doesn't do short-reads) Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Commit: be3a6781605803b2c48a48135002869ed2c73cf1 https://github.com/qemu/qemu/commit/be3a6781605803b2c48a48135002869ed2c73cf1 Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-02 (Fri, 02 Feb 2018) Changed paths: M tests/libqos/virtio.c M tests/libqos/virtio.h M tests/virtio-9p-test.c M tests/virtio-blk-test.c M tests/virtio-net-test.c M tests/virtio-scsi-test.c Log Message: ----------- libqos/virtio: return length written into used descriptor When a 9p request is flushed (ie, cancelled) by the guest, the device is expected to simply mark the request as used, without sending a 9p reply (ie, without writing anything into the used buffer). To be able to test this, we need access to the length written by the device into the used descriptor. This patch adds a uint32_t * argument to qvirtqueue_get_buf() and qvirtio_wait_used_elem() for this purpose. All existing users are updated accordingly. Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Commit: 357e2f7f4e4dc68f01d5b81f5cd669874314e14a https://github.com/qemu/qemu/commit/357e2f7f4e4dc68f01d5b81f5cd669874314e14a Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-02 (Fri, 02 Feb 2018) Changed paths: M hw/9pfs/9p-synth.c M hw/9pfs/9p-synth.h M hw/9pfs/9p.c M tests/virtio-9p-test.c Log Message: ----------- tests: virtio-9p: add FLUSH operation test The idea is to send a victim request that will possibly block in the server and to send a flush request to cancel the victim request. This patch adds two test to verifiy that: - the server does not reply to a victim request that was actually cancelled - the server replies to the flush request after replying to the victim request if it could not cancel it 9p request cancellation reference: http://man.cat-v.org/plan_9/5/flush Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> (groug, change the test to only write a single byte to avoid any alignment or endianess consideration) Commit: 9ea776ee7d4061c043d0fbf89aa85f86ec0cf8a2 https://github.com/qemu/qemu/commit/9ea776ee7d4061c043d0fbf89aa85f86ec0cf8a2 Author: Greg Kurz <gr...@kaod.org> Date: 2018-02-02 (Fri, 02 Feb 2018) Changed paths: M tests/virtio-9p-test.c Log Message: ----------- tests/virtio-9p: explicitly handle potential integer overflows Signed-off-by: Greg Kurz <gr...@kaod.org> Reviewed-by: Eric Blake <ebl...@redhat.com> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Commit: f74425e267f81f0f94adf47ecbd66224e0461936 https://github.com/qemu/qemu/commit/f74425e267f81f0f94adf47ecbd66224e0461936 Author: Peter Maydell <peter.mayd...@linaro.org> Date: 2018-02-02 (Fri, 02 Feb 2018) Changed paths: M hw/9pfs/9p-synth.c M hw/9pfs/9p-synth.h M hw/9pfs/9p.c M hw/9pfs/9p.h M hw/9pfs/trace-events M hw/9pfs/virtio-9p-device.c M hw/9pfs/xen-9p-backend.c M tests/libqos/virtio.c M tests/libqos/virtio.h M tests/virtio-9p-test.c M tests/virtio-blk-test.c M tests/virtio-net-test.c M tests/virtio-scsi-test.c Log Message: ----------- Merge remote-tracking branch 'remotes/gkurz/tags/for-upstream' into staging This series is mostly about 9p request cancellation. It fixes a long standing bug (read "specification violation") where the server would send an invalid response when the client has cancelled an in-flight request. This was causing annoying spurious EINTR returns in linux. The fix comes with some related testing in QTEST. Other patches are code cleanup and improvements. # gpg: Signature made Fri 02 Feb 2018 10:16:03 GMT # gpg: using RSA key 71D4D5E5822F73D6 # gpg: Good signature from "Greg Kurz <gr...@kaod.org>" # gpg: aka "Gregory Kurz <gregory.k...@free.fr>" # gpg: aka "[jpeg image of size 3330]" # Primary key fingerprint: B482 8BAF 9431 40CE F2A3 4910 71D4 D5E5 822F 73D6 * remotes/gkurz/tags/for-upstream: tests/virtio-9p: explicitly handle potential integer overflows tests: virtio-9p: add FLUSH operation test libqos/virtio: return length written into used descriptor tests: virtio-9p: add WRITE operation test tests: virtio-9p: add LOPEN operation test tests: virtio-9p: use the synth backend tests: virtio-9p: wait for completion in the test code tests: virtio-9p: move request tag to the test functions 9pfs: Correctly handle cancelled requests 9pfs: drop v9fs_register_transport() Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> Compare: https://github.com/qemu/qemu/compare/fabbd691fd7d...f74425e267f8