Re: [PATCH 05/12] test-bdrv-drain.c: adapt test to the coming subtree drains

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-devel@nongnu.org, Hanna Reitz <hreitz@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	John Snow <jsnow@redhat.com>
Subject: Re: [PATCH 05/12] test-bdrv-drain.c: adapt test to the coming subtree drains
Date: Thu, 3 Feb 2022 12:41:12 +0100	[thread overview]
Message-ID: <a8c8bb77-0c6d-8240-1847-6c8ce561c0d7@redhat.com> (raw)
In-Reply-To: <53900301-5b8c-a724-4e48-ba3fa003cc1e@redhat.com>

On 19/01/2022 10:18, Paolo Bonzini wrote:
> On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote:
>> - First of all, inconsistency between block_job_create under
>> aiocontext lock that internally calls blk_insert_bs and therefore
>> bdrv_replace_child_noperm, and blk_insert_bs that is called two lines
>> above in the same test without aiocontext. There seems to be no
>> reason on why we need the lock there, so move the aiocontext lock further
>> down.
>>
>> - test_detach_indirect: here it is simply a matter of wrong callbacks
>> used. In the original test, there was only a subtree drain, so
>> overriding .drained_begin was not a problem. Now that we want to have
>> additional subtree drains, we risk to call the test callback
>> to early, or multiple times. We do not want that, so override
>> the callback only when we actually want to use it.
> 
> The language here is a bit overcomplicated.  Don't think that you're
> writing Italian, instead use simple sentences.
> 
> First, the test is inconsistent about taking the AioContext lock when
> calling bdrv_replace_child_noperm.  bdrv_replace_child_noperm is reached
> in two places: from blk_insert_bs directly, and via block_job_create.
> Only the second does it with the AioContext lock taken, and there seems
> to be no reason why the lock is needed.  Move aio_context_acquire
> further down.  [Any reason why you don't move it even further down, to
> cover only job_start?]

The lock is left just to cover block_job_add_bdrv, since it internally
releases and then acquires the lock.
Fixing that is a future TODO.

job_start did not and does not need the AioContext lock :)

> 
> Second, test_detach_indirect is only interested in observing the first
> call to .drained_begin.  In the original test, there was only a single
> subtree drain; however, with additional drains introduced in
> bdrv_replace_child_noperm, the test callback would be called too early
> and/or multiple times.  Override the callback only when we actually want
> to use it, and put back the original after it's been invoked.
> 
> This could also be split in two commits.
> 

Will update the commit message, thank you!

Emanuele