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.

Berto

Reply via email to