[Devel] [PATCH vz9] dm-ploop: fix requeuing of delayed pios at the end of BAT

[Pavel Tikhomirov]

On 4/25/25 18:12, Alexander Atanasov wrote:
> On 25.04.25 13:05, Pavel Tikhomirov wrote:
>>
>>
>> On 4/25/25 13:35, Alexander Atanasov wrote:
>>> Rework ploop_advance_local_after_bat_wb so that if we have delayed
>>> update to the same page is really executed and don't endup in wait_list.
>>                           ^ missing "it"
>>> Due to releasing and taking locks there was a window that a new update
>>> could go into the wait_list. Both due to WRITEBACK bit cleared after
>>                                 ^ "both" does not sound right, maybe 
>> you meant "also"?
> 
> We have two cases that a pio can get there for the same reason so both 
> not also.

What cases when? It's not clear from the commit message.

> 
> 
>>> retaking the lock.
>>>
>>> To fix this take all pending pios and clear the bit under lock,
>>> using the same schema as in other code that checks the bit.
>>> After releasing the lock displatch delayed pios.
>>                                 ^ excess 'l'
>>
>> Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
>>
>> I would like explanation more specific:
>>
>> Only place we can add to md_page->wait_llist is in 
>> ploop_delay_if_md_busy if md_page->md_status has MD_UPDATING or 
>> MD_WRITEBACK bits (under both md_lock and bat_lock). Previously we 
>> first acquired elements from wait_llist and only then cleared 
>> MD_WRITEBACK bit, thus having a race window where wait_llist can 
>> receive new delayed elements after acquiring elements from it and 
>> before clearing MD_WRITEBACK and those new elements will never be 
>> processed (until the next writeback to the same metadata page).
> 
> 
> I explain what problem we faced and how we fixed it, Why do i have to 
> explain how code works - it is there to be read if one needs the details?
> 
>>
>> Putting bit-clear and elements-acquire under both locks will guarantee 
>> it is atomicity relative to ploop_delay_if_md_busy.
>>
>> One alternative case is when we delay due to MD_UPDATING bit set, and 
>> there is no MD_WRITEBACK at the time. This is also covered though more 
>> trickily. In this case the one who set MD_UPDATING bit, will call 
>> ploop_add_dirty_for_wb, which will be processed later in 
>> do_ploop_run_work -> ploop_submit_metadata_writeback and will 
>> eventually lead to MD_WRITEBACK bit set and writeback submitted via 
>> ploop_index_wb_submit. So it is guaranteed that wait_llist will always 
>> be handled in this case.
> 
> This is again explaining how code works not what the patch does.

Strongly disagree. I believe the whole locking scheme should be 
explained here, as it is the thing that we are fixing. There is no 
description of it anywhere. But we should be able to verify that what is 
written in code corresponds to how we intend it to work.

>>
>>>
>>> https://virtuozzo.atlassian.net/browse/VSTOR-102677
>>> https://virtuozzo.atlassian.net/browse/VSTOR-103768
>>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>>> ---
>>>   drivers/md/dm-ploop-map.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
>>> index 76544cdb476e..8ef0426e7468 100644
>>> --- a/drivers/md/dm-ploop-map.c
>>> +++ b/drivers/md/dm-ploop-map.c
>>> @@ -884,11 +884,15 @@ static void 
>>> ploop_advance_local_after_bat_wb(struct ploop *ploop,
>>>   #endif
>>>       WARN_ON_ONCE(!test_bit(MD_WRITEBACK, &md->status));
>>> +
>>> +    wait_llist_pending = llist_del_all(&md->wait_llist);
>>> +
>>> +    /* Clear flag at the end when all processing related is done */
>>> +    clear_bit(MD_WRITEBACK, &md->status);
>>>       spin_unlock(&md->md_lock);
>>>       spin_unlock_irqrestore(&ploop->bat_lock, flags);
>>>       kunmap_local(dst_clu);
>>> -    wait_llist_pending = llist_del_all(&md->wait_llist);
>>>       if (wait_llist_pending) {
>>>           llist_for_each_safe(pos, t, wait_llist_pending) {
>>>               pio = list_entry((struct list_head *)pos, typeof(*pio), 
>>> list);
>>> @@ -900,10 +904,6 @@ static void 
>>> ploop_advance_local_after_bat_wb(struct ploop *ploop,
>>>       if (!list_empty(&list))
>>>           ploop_dispatch_pios(ploop, NULL, &list);
>>> -    spin_lock_irqsave(&ploop->bat_lock, flags);
>>> -    /* Clear flag at the end when all processing related is done */
>>> -    clear_bit(MD_WRITEBACK, &md->status);
>>> -    spin_unlock_irqrestore(&ploop->bat_lock, flags);
>>>   }
>>>   static void ploop_free_piwb(struct ploop_index_wb *piwb)
>>
> 
> 

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