Re: [Qemu-devel] [PATCH] block: Fix race in gluster_finish_aiocb

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Wed, Aug 21, 2013 at 05:40:11PM +0200, Paolo Bonzini wrote:
> Il 21/08/2013 17:24, Stefan Hajnoczi ha scritto:
> > On Wed, Aug 21, 2013 at 10:02:47AM +0800, Asias He wrote:
> >> In block/gluster.c, we have
> >>
> >> gluster_finish_aiocb
> >> {
> >>    if (retval != sizeof(acb)) {
> >>       qemu_mutex_lock_iothread(); /* We are in gluster thread context */
> >>       ...
> >>       qemu_mutex_unlock_iothread();
> >>    }
> >> }
> >>
> >> qemu tools, e.g. qemu-img, might race here because
> >> qemu_mutex_{lock,unlock}_iothread are a nop operation and
> >> gluster_finish_aiocb is in the gluster thread context.
> >>
> >> To fix, we introduce our own mutex for qemu tools.
> > 
> > I think we need to look more closely at the error code path:
> > 
> > acb->ret = ret;
> > retval = qemu_write_full(s->fds[GLUSTER_FD_WRITE], &acb, sizeof(acb));
> > if (retval != sizeof(acb)) {
> >     /*
> >      * Gluster AIO callback thread failed to notify the waiting
> >      * QEMU thread about IO completion.
> >      *
> >      * Complete this IO request and make the disk inaccessible for
> >      * subsequent reads and writes.
> >      */
> >     error_report("Gluster failed to notify QEMU about IO completion");
> > 
> >     qemu_mutex_lock_iothread(); /* We are in gluster thread context */
> >     acb->common.cb(acb->common.opaque, -EIO);
> >     qemu_aio_release(acb);
> >     close(s->fds[GLUSTER_FD_READ]);
> >     close(s->fds[GLUSTER_FD_WRITE]);
> > 
> > Is it safe to close the fds?  There is a race here:
> > 
> > 1. Another thread opens a new file descriptor and gets GLUSTER_FD_READ or
> >    GLUSTER_FD_WRITE's old fd value.
> > 2. Another gluster thread invokes the callback and does
> >    qemu_write_full(s->fds[GLUSTER_FD_WRITE], ...).
> > 
> > Since the mutex doesn't protect s->fds[] this is possible.
> > 
> > Maybe a simpler solution for request completion is:
> > 
> > 1. Linked list of completed acbs.
> > 2. Mutex to protect the linked list.
> > 3. EventNotifier to signal iothread.
> 
> We could just use a bottom half, too.  Add a bottom half to acb,
> schedule it in gluster_finish_aiocb, delete it in the bottom half's own
> callback.

gluster_finish_aiocb gets called from gluster thread, is it safe to create
and schedule a bh from such a thread ?

In my first implementation (http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg01748.html), I was using a BH from qemu read side thread (the thread
that would respond to pipe write from gluster callback thread). That
implementation was based on rbd and I later dropped the BH part since it
looked like a round about way of completing the aio when we are already using
the pipe mechanism for aio completion.

Regards,
Bharata.