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

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Aug 22, 2013 at 11:29:47AM +0530, Bharata B Rao wrote:
> 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.

Recent patches made creating and scheduling a BH thread-safe.

I think Paolo's idea is better than mine.

Stefan