[Devel] [PATCH vz9 v1 06/63] dm-ploop: convert enospc handling to use lockless lists

[Pavel Tikhomirov]

On 2/3/25 15:42, Alexander Atanasov wrote:
> On 3.02.25 9:27, Pavel Tikhomirov wrote:
>>
>>
>> On 2/3/25 14:45, Alexander Atanasov wrote:
>>> On 3.02.25 8:01, Pavel Tikhomirov wrote:
>>>>
>>>>> @@ -166,7 +171,6 @@ static bool ploop_try_delay_enospc(struct 
>>>>> ploop_rq *prq, struct pio *pio)
>>>>>       bool delayed = true;
>>>>>       unsigned long flags;
>>>>> -    spin_lock_irqsave(&ploop->deferred_lock, flags);
>>>>>       if (unlikely(ploop->wants_suspend)) {
>>>>>           delayed = false;
>>>>>           goto unlock;
>>>>> @@ -176,10 +180,11 @@ static bool ploop_try_delay_enospc(struct 
>>>>> ploop_rq *prq, struct pio *pio)
>>>>>       pr_err_once(PL_FMT("underlying disk is almost full"),
>>>>>           ploop_device_name(ploop));
>>>>> +    spin_lock_irqsave(&ploop->deferred_lock, flags);
>>>>>       ploop->event_enospc = true;
>>>>> -    list_add_tail(&pio->list, &ploop->enospc_pios);
>>>>> -unlock:
>>>>>       spin_unlock_irqrestore(&ploop->deferred_lock, flags);
>>>>> +    llist_add((struct llist_node *)(&pio->list), &ploop- 
>>>>> >enospc_pios);
>>>>> +unlock:
>>>>>       if (delayed)
>>>>>           mod_timer(&ploop->enospc_timer, jiffies + 
>>>>> PLOOP_ENOSPC_TIMEOUT);
>>>>
>>>> Can you please explain why we need to take defered_lock around 
>>>> ploop- >event_enospc setting after your patch? (It looks that this 
>>>> lock does not do anything now.)
>>>>
>>>
>>> see static int ploop_get_event(...), without lock event can be missed
>>
>> That is not an explanation. How exactly can it be missed?
> 
> 

Other cpu can set "ploop->event_enospc = true" here before lock (i.e. it 
was set to true twice), that would lead to emiting only one event for 
two sets.

>          spin_lock_irq(&ploop->deferred_lock);
>          if (ploop->event_enospc) {                    <- while emit 
> other cpu can set
>                  ret = (DMEMIT("event_ENOSPC\n")) ? 1 : 0;
>                  if (ret)
>                          ploop->event_enospc = false; <- next cleaars 
> here -> second event lost

Not lost, you just emit once for two sets, which as you can see from 
above comment is possible even with locks everywhere. So your 
explanation does not prove that we need this lock AFAICS.

>          }
>          spin_unlock_irq(&ploop->deferred_lock);



> 
> 
> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.