From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKcl8-0003mL-GL for qemu-devel@nongnu.org; Wed, 29 Jul 2015 21:35:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKcl7-0007Hg-Ip for qemu-devel@nongnu.org; Wed, 29 Jul 2015 21:35:46 -0400 Date: Thu, 30 Jul 2015 09:35:37 +0800 From: Fam Zheng Message-ID: <20150730013537.GA23500@ad.nay.redhat.com> References: <1438144934-23619-1-git-send-email-famz@redhat.com> <1438144934-23619-12-git-send-email-famz@redhat.com> <55B882C1.8070500@redhat.com> <20150729105725.GA10376@ad.nay.redhat.com> <55B8B2C3.9050307@redhat.com> <20150729115333.GA15022@ad.nay.redhat.com> <55B8C12D.2070105@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55B8C12D.2070105@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org On Wed, 07/29 14:03, Paolo Bonzini wrote: > > > On 29/07/2015 13:53, Fam Zheng wrote: > >> > Yes, though I think you'd end up reverting patches 10 and 11 in the end. > > We will add outer disable/enable pairs to prevent another threads's aio_poll > > from sneaking in between bdrv_aio_poll calls, but we needn't obsolete > > bdrv_aio_poll() because of that - it can be useful by itself. For example > > bdrv_aio_cancel shouldn't look at ioeventfd, otherwise it could spin for too > > long on high load. Does that make sense? > > Did you mean bdrv_drain() (when it is not already surrounded by > disable/enable pairs in the caller)? But yes, that makes sense. > > I'm not sure that it makes sense to disable/enable in places such as > bdrv_pread. The caller's block, if any, should suffice. In this sense > you'd end up reverting large parts of patch 10. > > Then you would have to see how many calls to bdrv_aio_poll are still > there, and how many can be converted with no semantic change to aio_poll > (e.g. there's no difference in qemu-img.c), and you'd end up reverting > patches 9 and 11 too. But we can look at that later. > Okay. There 19 bdrv_aio_poll()'s after this series: block.c bdrv_create block/curl.c curl_init_state block/io.c bdrv_drain block/io.c bdrv_drain_all block/io.c bdrv_prwv_co block/io.c bdrv_get_block_status_above block/io.c bdrv_aio_cancel block/io.c bdrv_flush block/io.c bdrv_discard block/io.c bdrv_flush_io_queue block/nfs.c nfs_get_allocated_file_size block/qed-table.c qed_read_l1_table_sync block/qed-table.c qed_write_l1_table_sync block/qed-table.c qed_read_l2_table_sync block/qed-table.c qed_write_l2_table_sync blockjob.c block_job_finish_sync include/block/block.h bdrv_get_stats qemu-img.c run_block_job qemu-io-cmds.c do_co_write_zeroes qemu-io-cmds.c wait_break_f Most of them make some sense to me, but not many make a real difference. The most important ones should be bdrv_drain* and bdrv_flush, and can be taken care of from caller side. Fam