Re: [Qemu-devel] [PATCH] virtio-9p: add reset handler

[Greg Kurz]

On Thu, 6 Oct 2016 19:11:03 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, Oct 06, 2016 at 03:12:10PM +0200, Greg Kurz wrote:
> > Virtio devices should implement the VirtIODevice->reset() function to
> > perform necessary cleanup actions and to bring the device to a quiescent
> > state.
> > 
> > In the case of the virtio-9p device, this means:
> > - emptying the list of active PDUs (i.e. draining all in-flight I/O)
> > - freeing all fids (i.e. close open file descriptors and free memory)
> > 
> > That's what this patch does.
> > 
> > The reset handler first waits for all active PDUs to complete. Since
> > completion happens in the QEMU global aio context, we just have to
> > loop around aio_poll() until the active list is empty.
> > 
> > The freeing part involves some actions to be performed on the backend,
> > like closing file descriptors or flushing extended attributes to the
> > underlying filesystem. The virtfs_reset() function already does the
> > job: it calls free_fid() for all open fids not involved in an ongoing
> > I/O operation. We are sure this is the case since we have drained
> > the PDU active list.
> > 
> > The current code implements all backend accesses with coroutines, but we
> > want to stay synchronous on the reset path. We can either change the
> > current code to be able to run when not in coroutine context, or create
> > a coroutine context and wait for virtfs_reset() to complete. This patch
> > goes for the latter because it results in simpler code.
> > 
> > Note that we also need to create a dummy PDU because it is also an API
> > to pass the FsContext pointer to all backend callbacks.
> > 
> > Signed-off-by: Greg Kurz <address@hidden>  
> 
> Reviewed-by: Michael S. Tsirkin <address@hidden>
> 

Unless someone objects, this will go through my tree.

> > ---
> >  hw/9pfs/9p.c               |   31 +++++++++++++++++++++++++++++++
> >  hw/9pfs/9p.h               |    1 +
> >  hw/9pfs/virtio-9p-device.c |    8 ++++++++
> >  3 files changed, 40 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 119ee584969b..42137395037e 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -3522,6 +3522,37 @@ void v9fs_device_unrealize_common(V9fsState *s, 
> > Error **errp)
> >      g_free(s->tag);
> >  }
> >  
> > +

And I'll remove the empty line :)

> > +typedef struct VirtfsCoResetData {
> > +    V9fsPDU pdu;
> > +    bool done;
> > +} VirtfsCoResetData;
> > +
> > +static void coroutine_fn virtfs_co_reset(void *opaque)
> > +{
> > +    VirtfsCoResetData *data = opaque;
> > +
> > +    virtfs_reset(&data->pdu);
> > +    data->done = true;
> > +}
> > +
> > +void v9fs_reset(V9fsState *s)
> > +{
> > +    VirtfsCoResetData data = { .pdu = { .s = s }, .done = false };
> > +    Coroutine *co;
> > +
> > +    while (!QLIST_EMPTY(&s->active_list)) {
> > +        aio_poll(qemu_get_aio_context(), true);
> > +    }
> > +
> > +    co = qemu_coroutine_create(virtfs_co_reset, &data);
> > +    qemu_coroutine_enter(co);
> > +
> > +    while (!data.done) {
> > +        aio_poll(qemu_get_aio_context(), true);
> > +    }
> > +}
> > +
> >  static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
> >  {
> >      struct rlimit rlim;
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index d539d2ebe9c0..6b69eaf24614 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -339,5 +339,6 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, 
> > const char *fmt, ...);
> >  V9fsPDU *pdu_alloc(V9fsState *s);
> >  void pdu_free(V9fsPDU *pdu);
> >  void pdu_submit(V9fsPDU *pdu);
> > +void v9fs_reset(V9fsState *s);
> >  
> >  #endif
> > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > index 009b43f6d045..b73d72aceb64 100644
> > --- a/hw/9pfs/virtio-9p-device.c
> > +++ b/hw/9pfs/virtio-9p-device.c
> > @@ -130,6 +130,13 @@ static void virtio_9p_device_unrealize(DeviceState 
> > *dev, Error **errp)
> >      v9fs_device_unrealize_common(s, errp);
> >  }
> >  
> > +static void virtio_9p_reset(VirtIODevice *vdev)
> > +{
> > +    V9fsVirtioState *v = (V9fsVirtioState *)vdev;
> > +
> > +    v9fs_reset(&v->state);
> > +}
> > +
> >  ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
> >                              const char *fmt, va_list ap)
> >  {
> > @@ -188,6 +195,7 @@ static void virtio_9p_class_init(ObjectClass *klass, 
> > void *data)
> >      vdc->unrealize = virtio_9p_device_unrealize;
> >      vdc->get_features = virtio_9p_get_features;
> >      vdc->get_config = virtio_9p_get_config;
> > +    vdc->reset = virtio_9p_reset;
> >  }
> >  
> >  static const TypeInfo virtio_device_info = {