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
