Re: [PATCH] bcache: lock in btree_flush_write() to avoid races

[Coly Li <colyli@suse.de>]

On 24/01/2018 5:45 PM, tang.junhui@zte.com.cn wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
>> On 24/01/2018 2:54 PM, tang.junhui@zte.com.cn wrote:
>>> From: Tang Junhui <tang.junhui@zte.com.cn>
>>>
>>> In btree_flush_write(), two places need to take a locker to
>>> avoid races:
>>>
>>> Firstly, we need take rcu read locker to protect the bucket_hash
>>> traverse, since hlist_for_each_entry_rcu() must be called under
>>> the protection of rcu read locker.
>>>
>>> Secondly, we need take b->write_lock locker to protect journal
>>> of the btree node, otherwise, the btree node may have been
>>> written, and the journal have been assign to NULL.
>>>
>>> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
>>> ---
>>>  drivers/md/bcache/journal.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
>>> index 02a98dd..505f9f3 100644
>>> --- a/drivers/md/bcache/journal.c
>>> +++ b/drivers/md/bcache/journal.c
>>> @@ -375,7 +375,9 @@ static void btree_flush_write(struct cache_set *c)
>>>  retry:
>>>      best = NULL;
>>>  
>>> -    for_each_cached_btree(b, c, i)
>>> +    rcu_read_lock();
>>> +    for_each_cached_btree(b, c, i) {
>>> +        mutex_lock(&b->write_lock);
>>>          if (btree_current_write(b)->journal) {
>>>              if (!best)
>>>                  best = b;
>>> @@ -385,6 +387,9 @@ static void btree_flush_write(struct cache_set *c)
>>>                  best = b;
>>>              }
>>>          }
>>> +        mutex_unlock(&b->write_lock);
>>> +    }
>>> +    rcu_read_unlock();
>>>  
>>>      b = best;
>>>      if (b) {
>>>
>>
>> Hi Junhui,
>>
>> Do you run into some real problem ? The patch looks good to me at first
>> glance, but I need time to dig it more before I provide my response.
>>
>> Thanks.
> 
> No real problem run out, I find it by code reading, I think we'd better
> lock it for safety. 

I see, then I will take it as a lower priority and finish my current
task firstly. I will provide my review comment 1 week later, if no one
else does it then.

Thanks.

Coly Li