[Devel] [PATCH VZ9 2/3] block/blk-cbt: allow multiple cbts in a single queue

[Andrey Zhadchenko]

On 1/22/25 11:51, Pavel Tikhomirov wrote:
>> +static struct cbt_info *blk_cbt_find(struct request_queue *q, __u8 
>> *name)
>> +{
>> +    struct cbt_info *cbt;
>> +
>> +    list_for_each_entry(cbt, &q->cbt_list, list)
>> +        if (!memcmp(name, cbt->name, CBT_NAME_LENGTH))
>> +            return cbt;
>> +
>> +    return NULL;
>> +}
>> +
>>   static int blk_cbt_snap_create(struct request_queue *q, __u8 *name,
>>                      struct blk_user_cbt_snap_create __user *arg)
>>   {
>> @@ -382,8 +397,7 @@ static int blk_cbt_snap_create(struct 
>> request_queue *q, __u8 *name,
>>           return -EFAULT;
>>       mutex_lock(&cbt_mutex);
>> -    cbt = q->cbt;
>> -
>> +    cbt = blk_cbt_find(q, name);
>>       if (!cbt) {
>>           mutex_unlock(&cbt_mutex);
>>           return -ENOENT;
>> @@ -392,11 +406,6 @@ static int blk_cbt_snap_create(struct 
>> request_queue *q, __u8 *name,
>>       BUG_ON(!cbt->map);
>>       BUG_ON(!cbt->block_max);
>> -    if (!name || memcmp(name, cbt->name, sizeof(cbt->name))) {
>> -        mutex_unlock(&cbt_mutex);
>> -        return -EINVAL;
>> -    }
>> -
>>       if (cbt->snp_map) {
>>           mutex_unlock(&cbt_mutex);
>>           return -EBUSY;
> ...
>> @@ -667,35 +665,46 @@ static void cbt_release_callback(struct rcu_head 
>> *head)
>>       kfree(cbt);
>>   }
>> -void blk_cbt_release(struct request_queue *q)
>> +static void blk_cbt_del(struct cbt_info *cbt)
>>   {
>> -    struct cbt_info *cbt;
>> -    int in_use = 0;
>>       unsigned long flags;
>> +    int in_use;
>> -    cbt = q->cbt;
>> -    if (!cbt)
>> -        return;
>>       spin_lock_irqsave(&cbt->lock, flags);
>>       set_bit(CBT_DEAD, &cbt->flags);
>> -    rcu_assign_pointer(q->cbt, NULL);
>> +    list_del_rcu(&cbt->list);
>>       in_use = cbt->count;
>>       spin_unlock_irqrestore(&cbt->lock, flags);
>>       if (!in_use)
>>           call_rcu(&cbt->rcu, &cbt_release_callback);
>>   }
> 
> Why we don't take rcu_read_lock around list iteration in blk_cbt_find? 
> If we don't take rcu_read_lock there, the cbt returned from blk_cbt_find 
> can be already freed.

rcu is not taken on a path where mutex is held. cbt_mutex makes user 
operations exclusive, so there won't be any race. Maybe I should add a 
comment then?

> 
> I see other places of _rcu walk without rcu_read_lock, e.g.: 
> cbt_page_alloc (!in_rcu) case, blk_cbt_update_size.
> 
> Also list can be broken probably if blk_cbt_del runs concurrently with 
> itself (blk_cbt_release vs cbt_ioc_stop). I think the whole loop in 
> blk_cbt_release should be protected against concurrent cbt_ioc_stop 
> (that may be true as blk_cbt_release probably runs at the very end).
> 
I don't think blk_cbt_release can be run concurrently win any of blk_cbt 
functions, because they are operation on a file descriptor. I am sure 
that kernel won't call blk_cbt_release() when some fd still refers to 
the device