On 4/18/18 3:18 AM, jiang.biao2@zte.com.cn wrote:

Hi,

quoted

quoted

Il giorno 17 apr 2018, alle ore 09:10, Jiang Biao [off-list ref] ha scritto:

As described in the comment of blkcg_activate_policy(),
*Update of each blkg is protected by both queue and blkcg locks so
that holding either lock and testing blkcg_policy_enabled() is
always enough for dereferencing policy data.*
with queue lock held, there is no need to hold blkcg lock in
blkcg_deactivate_policy(). Similar case is in
blkcg_activate_policy(), which has removed holding of blkcg lock in
commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.

Hi,
by chance, did you check whether this may cause problems with bfq,
being the latter not protected by the queue lock as cfq?

Checked the bfq code, bfq seems never used blkcg lock derectly, and 
update of blkg in the common code is protected by both queue and 
blkcg locks, so IMHO this patch would not introduce any new problem
with bfq, even though bfq is not protected by queue lock.
On the other hand, the locks (queue lock/blkcg lock) used to protected
the update of blkg seems a bit too heavyweight, especially the queue lock 
which is used too widely may cause races with other contexts. I wonder
if there is any way to ease the case? e.g. add a new lock for blkg's own.:) 

It might make sense to lock it separately, but I would not worry
about it unless it shows up as hot in your testing.

I've applied your patch, thanks.

-- 
Jens Axboe