On Mon, 18 Dec 2017 11:41:23 +0100
Greg Kurz <gr...@kaod.org> wrote:
> The 9p protocol mostly relies on a request/reply dialog between the
> client and the server. A notable exception to this rule is request
> cancellation (ie, flush in 9p wording): the server shouldn't send a
> reply when the request was flushed.
>
> This patch changes the transport API so that the core 9p code may
> choose to discard a reply instead of pushing it back to the client.
> As a consequence, the push_and_notify naming isn't quite appropriate
> anymore. And by the way, this operation also frees ring descriptors
> allocated by the transport, ie, finalizes the completion of the PDU
> in the transport layer. It is hence renamed to pdu_complete.
>
> This will be used by a subsequent patch to fix request cancellation.
>
> Signed-off-by: Greg Kurz <gr...@kaod.org>
> ---
> hw/9pfs/9p.c | 10 +++++-----
> hw/9pfs/9p.h | 2 +-
> hw/9pfs/virtio-9p-device.c | 11 +++++++----
> hw/9pfs/xen-9p-backend.c | 14 ++++++++------
> 4 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 1e4ebbe57687..dd8b7c6d69c6 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -642,7 +642,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> ssize_t len)
>
> ret = pdu_marshal(pdu, len, "s", &str);
> if (ret < 0) {
> - goto out_notify;
> + goto out_complete;
> }
> len += ret;
> id = P9_RERROR;
> @@ -650,7 +650,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> ssize_t len)
>
> ret = pdu_marshal(pdu, len, "d", err);
> if (ret < 0) {
> - goto out_notify;
> + goto out_complete;
> }
> len += ret;
>
> @@ -662,15 +662,15 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> ssize_t len)
>
> /* fill out the header */
> if (pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag) < 0) {
> - goto out_notify;
> + goto out_complete;
> }
>
> /* keep these in sync */
> pdu->size = len;
> pdu->id = id;
>
> -out_notify:
> - pdu->s->transport->push_and_notify(pdu);
> +out_complete:
> + pdu->s->transport->pdu_complete(pdu, false);
>
> /* Now wakeup anybody waiting in flush for this request */
> if (!qemu_co_queue_next(&pdu->complete)) {
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index ffe658ab8975..ea7657837784 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -363,7 +363,7 @@ struct V9fsTransport {
> unsigned int *pniov, size_t size);
> void (*init_out_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov,
> unsigned int *pniov, size_t size);
> - void (*push_and_notify)(V9fsPDU *pdu);
> + void (*pdu_complete)(V9fsPDU *pdu, bool discard);
> };
>
> static inline int v9fs_register_transport(V9fsState *s, const V9fsTransport
> *t)
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 43f4e53f336f..083df987fd93 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -20,14 +20,17 @@
> #include "hw/virtio/virtio-access.h"
> #include "qemu/iov.h"
>
> -static void virtio_9p_push_and_notify(V9fsPDU *pdu)
> +static void virtio_pdu_complete(V9fsPDU *pdu, bool discard)
> {
> V9fsState *s = pdu->s;
> V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> VirtQueueElement *elem = v->elems[pdu->idx];
>
> - /* push onto queue and notify */
> - virtqueue_push(v->vq, elem, pdu->size);
> + if (discard) {
> + virtqueue_detach_element(v->vq, elem, 0);
> + } else {
> + virtqueue_push(v->vq, elem, pdu->size);
We should only notify the client if we actually push something
back, ie, only call virtio_notify() if discard is false...
> + }
> g_free(elem);
> v->elems[pdu->idx] = NULL;
>
> @@ -189,7 +192,7 @@ static const V9fsTransport virtio_9p_transport = {
> .pdu_vunmarshal = virtio_pdu_vunmarshal,
> .init_in_iov_from_pdu = virtio_init_in_iov_from_pdu,
> .init_out_iov_from_pdu = virtio_init_out_iov_from_pdu,
> - .push_and_notify = virtio_9p_push_and_notify,
> + .pdu_complete = virtio_pdu_complete,
> };
>
> static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index df2a4100bf55..dc1e6c88885d 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -210,7 +210,7 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
> *pniov = num;
> }
>
> -static void xen_9pfs_push_and_notify(V9fsPDU *pdu)
> +static void xen_9pfs_pdu_complete(V9fsPDU *pdu, bool discard)
> {
> RING_IDX prod;
> Xen9pfsDev *priv = container_of(pdu->s, Xen9pfsDev, state);
> @@ -222,10 +222,12 @@ static void xen_9pfs_push_and_notify(V9fsPDU *pdu)
> ring->intf->out_cons = ring->out_cons;
> xen_wmb();
>
> - prod = ring->intf->in_prod;
> - xen_rmb();
> - ring->intf->in_prod = prod + pdu->size;
> - xen_wmb();
> + if (!discard) {
> + prod = ring->intf->in_prod;
> + xen_rmb();
> + ring->intf->in_prod = prod + pdu->size;
> + xen_wmb();
> + }
>
> ring->inprogress = false;
> xenevtchn_notify(ring->evtchndev, ring->local_port);
... should xenevtchn_notify() be moved to the !discard block ?
> @@ -238,7 +240,7 @@ static const V9fsTransport xen_9p_transport = {
> .pdu_vunmarshal = xen_9pfs_pdu_vunmarshal,
> .init_in_iov_from_pdu = xen_9pfs_init_in_iov_from_pdu,
> .init_out_iov_from_pdu = xen_9pfs_init_out_iov_from_pdu,
> - .push_and_notify = xen_9pfs_push_and_notify,
> + .pdu_complete = xen_9pfs_pdu_complete,
> };
>
> static int xen_9pfs_init(struct XenDevice *xendev)
>
