From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:33686)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pradeep.jagadeesh@huawei.com>) id 1bwtlO-0000y0-Mv
	for qemu-devel@nongnu.org; Wed, 19 Oct 2016 12:30:47 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pradeep.jagadeesh@huawei.com>) id 1bwtlL-0004v4-JS
	for qemu-devel@nongnu.org; Wed, 19 Oct 2016 12:30:46 -0400
Received: from lhrrgout.huawei.com ([194.213.3.17]:4269)
	by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71)
	(envelope-from <pradeep.jagadeesh@huawei.com>) id 1bwtlL-0004uP-B8
	for qemu-devel@nongnu.org; Wed, 19 Oct 2016 12:30:43 -0400
References: <1474014817-41572-1-git-send-email-pradeep.jagadeesh@huawei.com>
	<w51k2e7ucc1.fsf@maestria.local.igalia.com>
	<161e02dc-2358-cd62-46b8-5f5d4eca6b67@huawei.com>
	<w51zim1hboy.fsf@maestria.local.igalia.com>
From: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
Message-ID: <37f6452a-aa29-1cac-fde6-31d53e11449f@huawei.com>
Date: Wed, 19 Oct 2016 18:29:45 +0200
MIME-Version: 1.0
In-Reply-To: <w51zim1hboy.fsf@maestria.local.igalia.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev
 devices
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alberto Garcia <berto@igalia.com>, Pradeep Jagadeesh <pradeepkiruvale@gmail.com>, "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>, Greg Kurz <groug@kaod.org>
Cc: qemu-devel@nongnu.org, Claudio Fontana <claudio.fontana@huawei.com>

On 10/18/2016 6:19 PM, Alberto Garcia wrote:
> On Mon 17 Oct 2016 11:19:11 AM CEST, Pradeep Jagadeesh wrote:
>
>>>> +typedef struct FsThrottle {
>>>> +    ThrottleState ts;
>>>> +    ThrottleTimers tt;
>>>> +    AioContext   *aioctx;
>>>> +    ThrottleConfig cfg;
>>>> +    bool enabled;
>>>> +    CoQueue      throttled_reqs[2];
>>>> +    unsigned     pending_reqs[2];
>>>> +    bool any_timer_armed[2];
>>>> +    QemuMutex lock;
>>>> +} FsThrottle;
>>>
>>> You based your implementation on block/throttle-groups.c. I think
>>> yours can be made simpler because one of the problems with mine is
>>> that it needs to support multiple parallel I/O operations on the same
>>> throttling group, and that's why the locking rules are more
>>> complex. With a single user per ThrottleConfig this is not necessary.
>>>
>>> Have you checked if you really need them? My impression is that you
>>> might be able to get rid of the 'lock', 'any_timer_armed' and
>>> 'pending_reqs' fields.
>>>
>>> Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see
>>> how was the transition from single-drive throttling to group
>>> throttling, specifically the bdrv_io_limits_intercept() function. You
>>> will see that it was simpler.
>>
>> I tried removing the lock, I got into rcu issues, and the qemu hangs.
>> Once I put them back, it works fine.
>
> Did you figure out why the lock is needed in this case? In the case of
> disk group throttling, it is because there is data shared by several
> disks which may be running at the same time.
>
One more update is, I had a look at bdrv_io_limits_intercept and  I just 
removed lock and retained other two things pending_reqs,any_timer_armed
It works fine.

-Pradeep

> Berto
>