From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> To: Kevin Wolf <kwolf@redhat.com> Cc: "qemu-block@nongnu.org" <qemu-block@nongnu.org>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org> Subject: Re: [Qemu-devel] [PULL 16/71] nbd: Increase bs->in_flight during AioContext switch Date: Thu, 11 Apr 2019 17:13:31 +0000 [thread overview] Message-ID: <2558f6fa-03d5-9483-07ff-7ec0a4d37835@virtuozzo.com> (raw) In-Reply-To: <20190411164803.GF5694@linux.fritz.box> 11.04.2019 19:48, Kevin Wolf wrote: > Am 11.04.2019 um 16:48 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 11.04.2019 17:15, Kevin Wolf wrote: >>> Am 11.04.2019 um 15:40 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> 25.02.2019 18:19, Kevin Wolf wrote: >>>>> bdrv_drain() must not leave connection_co scheduled, so bs->in_flight >>>>> needs to be increased while the coroutine is waiting to be scheduled >>>>> in the new AioContext after nbd_client_attach_aio_context(). >>>> >>>> Hi! >>>> >>>> I have some questions, could you explain, please? >>>> >>>> "bdrv_drain() must not leave connection_co scheduled" - it's because we want to be >>>> sure that connection_co yielded from nbd_read_eof, yes? >>>> >>>> But it is guaranteed by aio_wait_bh_oneshot.. Why do we need additioinally inc/dec >>>> bs->in_flight ? >>> >>> Without incrementing bs->in_flight, nothing would guarantee that >>> aio_poll() is called and the BH is actually executed before bdrv_drain() >>> returns. >> >> Don't follow.. Don't we want exactly this, we want BH to be executed while node is still >> drained, as you write in comment? > > Yes, exactly. But if bs->in_flight == 0, the AIO_WAIT_WHILE() condition > in the drain code could become false, so aio_poll() would not be called > again and drain would return even if the BH is still pending. > Ah, oops, sorry my English, I read it like "nothing would prevent". Understand now, thanks. >>> >>>>> >>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>>> --- >>>>> block/nbd-client.c | 20 ++++++++++++++++++-- >>>>> 1 file changed, 18 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/block/nbd-client.c b/block/nbd-client.c >>>>> index 60f38f0320..bfbaf7ebe9 100644 >>>>> --- a/block/nbd-client.c >>>>> +++ b/block/nbd-client.c >>>>> @@ -977,14 +977,30 @@ void nbd_client_detach_aio_context(BlockDriverState *bs) >>>>> qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc)); >>>>> } >>>>> >>>>> +static void nbd_client_attach_aio_context_bh(void *opaque) >>>>> +{ >>>>> + BlockDriverState *bs = opaque; >>>>> + NBDClientSession *client = nbd_get_client_session(bs); >>>>> + >>>>> + /* The node is still drained, so we know the coroutine has yielded in >>>>> + * nbd_read_eof(), the only place where bs->in_flight can reach 0, or it is >>>>> + * entered for the first time. Both places are safe for entering the >>>>> + * coroutine.*/ >>>>> + qemu_aio_coroutine_enter(bs->aio_context, client->connection_co); >>>>> + bdrv_dec_in_flight(bs); >>>>> +} >>>>> + >>>>> void nbd_client_attach_aio_context(BlockDriverState *bs, >>>>> AioContext *new_context) >>>>> { >>>>> NBDClientSession *client = nbd_get_client_session(bs); >>>>> qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context); >>>>> >>>>> - /* FIXME Really need a bdrv_inc_in_flight() here */ >>>>> - aio_co_schedule(new_context, client->connection_co); >>>>> + bdrv_inc_in_flight(bs); >>>>> + >>>>> + /* Need to wait here for the BH to run because the BH must run while the >>>>> + * node is still drained. */ >>>>> + aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs); >>>>> } >>>>> >>>>> void nbd_client_close(BlockDriverState *bs) >>>>> >>>> >>>> >>>> -- >>>> Best regards, >>>> Vladimir >> >> >> -- >> Best regards, >> Vladimir -- Best regards, Vladimir
WARNING: multiple messages have this Message-ID (diff)
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> To: Kevin Wolf <kwolf@redhat.com> Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>, "qemu-block@nongnu.org" <qemu-block@nongnu.org> Subject: Re: [Qemu-devel] [PULL 16/71] nbd: Increase bs->in_flight during AioContext switch Date: Thu, 11 Apr 2019 17:13:31 +0000 [thread overview] Message-ID: <2558f6fa-03d5-9483-07ff-7ec0a4d37835@virtuozzo.com> (raw) Message-ID: <20190411171331.L8fVijY1QwvKWAg3iCnC-4SGJLPtIyZT6uYAurRbehk@z> (raw) In-Reply-To: <20190411164803.GF5694@linux.fritz.box> 11.04.2019 19:48, Kevin Wolf wrote: > Am 11.04.2019 um 16:48 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 11.04.2019 17:15, Kevin Wolf wrote: >>> Am 11.04.2019 um 15:40 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> 25.02.2019 18:19, Kevin Wolf wrote: >>>>> bdrv_drain() must not leave connection_co scheduled, so bs->in_flight >>>>> needs to be increased while the coroutine is waiting to be scheduled >>>>> in the new AioContext after nbd_client_attach_aio_context(). >>>> >>>> Hi! >>>> >>>> I have some questions, could you explain, please? >>>> >>>> "bdrv_drain() must not leave connection_co scheduled" - it's because we want to be >>>> sure that connection_co yielded from nbd_read_eof, yes? >>>> >>>> But it is guaranteed by aio_wait_bh_oneshot.. Why do we need additioinally inc/dec >>>> bs->in_flight ? >>> >>> Without incrementing bs->in_flight, nothing would guarantee that >>> aio_poll() is called and the BH is actually executed before bdrv_drain() >>> returns. >> >> Don't follow.. Don't we want exactly this, we want BH to be executed while node is still >> drained, as you write in comment? > > Yes, exactly. But if bs->in_flight == 0, the AIO_WAIT_WHILE() condition > in the drain code could become false, so aio_poll() would not be called > again and drain would return even if the BH is still pending. > Ah, oops, sorry my English, I read it like "nothing would prevent". Understand now, thanks. >>> >>>>> >>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>>> --- >>>>> block/nbd-client.c | 20 ++++++++++++++++++-- >>>>> 1 file changed, 18 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/block/nbd-client.c b/block/nbd-client.c >>>>> index 60f38f0320..bfbaf7ebe9 100644 >>>>> --- a/block/nbd-client.c >>>>> +++ b/block/nbd-client.c >>>>> @@ -977,14 +977,30 @@ void nbd_client_detach_aio_context(BlockDriverState *bs) >>>>> qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc)); >>>>> } >>>>> >>>>> +static void nbd_client_attach_aio_context_bh(void *opaque) >>>>> +{ >>>>> + BlockDriverState *bs = opaque; >>>>> + NBDClientSession *client = nbd_get_client_session(bs); >>>>> + >>>>> + /* The node is still drained, so we know the coroutine has yielded in >>>>> + * nbd_read_eof(), the only place where bs->in_flight can reach 0, or it is >>>>> + * entered for the first time. Both places are safe for entering the >>>>> + * coroutine.*/ >>>>> + qemu_aio_coroutine_enter(bs->aio_context, client->connection_co); >>>>> + bdrv_dec_in_flight(bs); >>>>> +} >>>>> + >>>>> void nbd_client_attach_aio_context(BlockDriverState *bs, >>>>> AioContext *new_context) >>>>> { >>>>> NBDClientSession *client = nbd_get_client_session(bs); >>>>> qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context); >>>>> >>>>> - /* FIXME Really need a bdrv_inc_in_flight() here */ >>>>> - aio_co_schedule(new_context, client->connection_co); >>>>> + bdrv_inc_in_flight(bs); >>>>> + >>>>> + /* Need to wait here for the BH to run because the BH must run while the >>>>> + * node is still drained. */ >>>>> + aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs); >>>>> } >>>>> >>>>> void nbd_client_close(BlockDriverState *bs) >>>>> >>>> >>>> >>>> -- >>>> Best regards, >>>> Vladimir >> >> >> -- >> Best regards, >> Vladimir -- Best regards, Vladimir
next prev parent reply other threads:[~2019-04-11 17:13 UTC|newest] Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-25 15:19 [Qemu-devel] [PULL 00/71] Block layer patches Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 01/71] MAINTAINERS: Replace myself with John Snow for block jobs Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 02/71] MAINTAINERS: Remove myself as block maintainer Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 03/71] block/snapshot.c: eliminate use of ID input in snapshot operations Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 04/71] block/snapshot: remove bdrv_snapshot_delete_by_id_or_name Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 05/71] qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 06/71] block: don't set the same context Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 07/71] commit: Replace commit_top_bs on failure after deleting the block job Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 08/71] qemu-img: fix error reporting for -object Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 09/71] block-backend: Make blk_inc/dec_in_flight public Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 10/71] virtio-blk: Increase in_flight for request restart BH Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 11/71] nbd: Restrict connection_co reentrance Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 12/71] io: Make qio_channel_yield() interruptible Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 13/71] io: Remove redundant read/write_coroutine assignments Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 14/71] nbd: Move nbd_read_eof() to nbd/client.c Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 15/71] nbd: Use low-level QIOChannel API in nbd_read_eof() Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 16/71] nbd: Increase bs->in_flight during AioContext switch Kevin Wolf 2019-04-11 13:40 ` Vladimir Sementsov-Ogievskiy 2019-04-11 14:15 ` Kevin Wolf 2019-04-11 14:15 ` Kevin Wolf 2019-04-11 14:48 ` Vladimir Sementsov-Ogievskiy 2019-04-11 14:48 ` Vladimir Sementsov-Ogievskiy 2019-04-11 16:48 ` Kevin Wolf 2019-04-11 16:48 ` Kevin Wolf 2019-04-11 17:13 ` Vladimir Sementsov-Ogievskiy [this message] 2019-04-11 17:13 ` Vladimir Sementsov-Ogievskiy 2019-04-11 17:20 ` Vladimir Sementsov-Ogievskiy 2019-04-11 17:20 ` Vladimir Sementsov-Ogievskiy 2019-04-12 11:11 ` Kevin Wolf 2019-04-12 11:11 ` Kevin Wolf 2019-02-25 15:19 ` [Qemu-devel] [PULL 17/71] block: Don't poll in bdrv_set_aio_context() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 18/71] block: Fix AioContext switch for drained node Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 19/71] test-bdrv-drain: AioContext switch in drained section Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 20/71] block: Use normal drain for bdrv_set_aio_context() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 21/71] aio-posix: Assert that aio_poll() is always called in home thread Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 22/71] block: improve should_update_child Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 23/71] block: fix bdrv_check_perm for non-tree subgraph Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 24/71] tests: add test-bdrv-graph-mod Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 25/71] qcow2: Assert that L2 table offsets fit in the L1 table Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 26/71] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 27/71] block: Use bdrv_refresh_filename() to pull Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 28/71] block: Use children list in bdrv_refresh_filename Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 29/71] block: Skip implicit nodes for filename info Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 30/71] block: Add BDS.auto_backing_file Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 31/71] block: Respect backing bs in bdrv_refresh_filename Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 32/71] iotests.py: Add filter_imgfmt() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 33/71] iotests.py: Add node_info() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 34/71] iotests: Add test for backing file overrides Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 35/71] block: Make path_combine() return the path Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 36/71] block: bdrv_get_full_backing_filename_from_...'s ret. val Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 37/71] block: bdrv_get_full_backing_filename's " Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 38/71] block: Add bdrv_make_absolute_filename() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 39/71] block: Fix bdrv_find_backing_image() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 40/71] block: Add bdrv_dirname() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 41/71] blkverify: Make bdrv_dirname() return NULL Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 42/71] quorum: " Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 43/71] block/nbd: " Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 44/71] block/nfs: Implement bdrv_dirname() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 45/71] block: Use bdrv_dirname() for relative filenames Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 46/71] iotests: Add quorum case to test 110 Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 47/71] block: Add strong_runtime_opts to BlockDriver Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 48/71] block: Add BlockDriver.bdrv_gather_child_options Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 49/71] block: Generically refresh runtime options Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 50/71] block: Purify .bdrv_refresh_filename() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 51/71] block: Do not copy exact_filename from format file Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 52/71] block/nvme: Fix bdrv_refresh_filename() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 53/71] block/curl: Harmonize option defaults Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 54/71] block/curl: Implement bdrv_refresh_filename() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 55/71] block/null: Generate filename even with latency-ns Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 56/71] block: BDS options may lack the "driver" option Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 57/71] iotests: Test json:{} filenames of internal BDSs Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 58/71] iotests: Re-add filename filters Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 59/71] iotests: Fix 237 for Python 2.x Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 60/71] iotests: Remove superfluous rm from 232 Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 61/71] iotests: Fix 232 for LUKS Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 62/71] iotests: Fix 207 to use QMP filters for qmp_log Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 63/71] iotests.py: Add is_str() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 64/71] iotests.py: Filter filename in any string value Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 65/71] iotests: Filter SSH paths Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 66/71] iotests: Let 045 be run concurrently Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 67/71] iotests.py: s/_/-/g on keys in qmp_log() Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 68/71] qcow2: include LUKS payload overhead in qemu-img measure Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 69/71] iotests: add LUKS payload overhead to 178 qemu-img measure test Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 70/71] vmdk: false positive of compat6 with hwversion not set Kevin Wolf 2019-02-25 15:20 ` [Qemu-devel] [PULL 71/71] iotests: Skip 211 on insufficient memory Kevin Wolf 2019-02-27 17:43 ` [Qemu-devel] [PULL 00/71] Block layer patches no-reply 2019-02-28 9:42 ` Peter Maydell
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2558f6fa-03d5-9483-07ff-7ec0a4d37835@virtuozzo.com \ --to=vsementsov@virtuozzo.com \ --cc=kwolf@redhat.com \ --cc=qemu-block@nongnu.org \ --cc=qemu-devel@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.