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.
