Re: [Qemu-devel] [PATCH v3 1/2] 9pfs: allow PDU to complete without sending a reply

[Greg Kurz <groug@kaod.org>]

Stefano, ping ?

On Wed, 20 Dec 2017 13:59:41 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Mon, 18 Dec 2017 11:41:23 +0100
> Greg Kurz <groug@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 <groug@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)
> >   
> 
>