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.