From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42915 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OxicG-00084V-3G for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:49:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OxicE-0007aM-Bd for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:49:15 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:64505) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OxicE-0007aG-7i for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:49:14 -0400 Received: by gya1 with SMTP id 1so1797676gya.4 for ; Mon, 20 Sep 2010 08:49:13 -0700 (PDT) Message-ID: <4C97826B.4060502@codemonkey.ws> Date: Mon, 20 Sep 2010 10:48:59 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [RFC] block-queue: Delay and batch metadata writes References: <1284991010-10951-1-git-send-email-kwolf@redhat.com> <4C977028.3050602@codemonkey.ws> <4C977626.4040806@codemonkey.ws> <4C977EC1.9010605@redhat.com> In-Reply-To: <4C977EC1.9010605@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org On 09/20/2010 10:33 AM, Kevin Wolf wrote: > Am 20.09.2010 16:56, schrieb Anthony Liguori: > >>>> +void blkqueue_flush(BlockQueue *bq) >>>> +{ >>>> + qemu_mutex_lock(&bq->flush_lock); >>>> + >>>> + /* Process any left over requests */ >>>> + while (QTAILQ_FIRST(&bq->queue)) { >>>> + blkqueue_process_request(bq); >>>> + } >>>> + >>>> + qemu_mutex_unlock(&bq->flush_lock); >>>> +} >>>> + >>>> +static void *blkqueue_thread(void *_bq) >>>> +{ >>>> + BlockQueue *bq = _bq; >>>> +#ifndef RUN_TESTS >>>> + BlockQueueRequest *req; >>>> +#endif >>>> + >>>> + qemu_mutex_lock(&bq->flush_lock); >>>> + while (!bq->thread_done) { >>>> + barrier(); >>>> >> A barrier shouldn't be needed here. >> > It was needed when I started with an empty thread because gcc would > "optimize" while(!bq->thread_done) into an endless loop. I guess there > is enough code added now that gcc won't try to be clever any more, so I > can remove that. > The qemu_cond_wait() will act as a read barrier. >> A less invasive way of doing this (assuming we're okay with it from a >> correctness perspective) is to make use of qemu_aio_wait() as a >> replacement for qemu_mutex_lock() and shift the pread/pwrite calls to >> bdrv_aio_write/bdrv_aio_read. >> >> IOW, blkqueue_pwrite stages a request via bdrv_aio_write(). >> blkqueue_pread() either returns a cached read or it does a >> bdrv_pread(). The blkqueue_flush() call will then do qemu_aio_wait() to >> wait for all pending I/Os to complete. >> > I was actually considering that, but it would have been a bit more > coding to keep track of another queue of in-flight requests, juggling > with some more AIOCBs and implementing an emulation for the missing > bdrv_aio_pwrite. Nothing really dramatic, it just was easier to start > this way. > bdrv_aio_pwritev is definitely useful in other places so it's worth adding. > If we come to the conclusion that bdrv_aio_write is the way to go and > it's worth the work, I'm fine with changing it. > Adding locking to allow bdrv_pwrite/bdrv_pread to be safely called outside of qemu_mutex is going to carry an awful lot of complexity since we can do things like layer qcow2 on top of NBD. That means bdrv_pread() may be repeatedly interacting with the main loop which means that there's no simple place to start. I'm not fundamentally opposed to using threads for concurrency. I think it's going to get super complicated though to do it here. Regards, Anthony Liguori > Kevin >