[Devel] [PATCH vz9] dm-ploop: fix and rework md updates

[Alexander Atanasov]

On 12.02.25 11:52, Alexander Atanasov wrote:
> On 12.02.25 11:50, Pavel Tikhomirov wrote:
>>
>>
>> On 2/11/25 22:25, Alexander Atanasov wrote:
>>> @@ -2067,21 +2109,46 @@ static inline int 
>>> ploop_submit_metadata_writeback(struct ploop *ploop, int force
>>>        */
>>>       llist_for_each_safe(pos, t, ll_wb_batch) {
>>>           md = list_entry((struct list_head *)pos, typeof(*md), 
>>> wb_link);
>>> +        INIT_LIST_HEAD(&md->wb_link);
>>>           /* XXX: fixme delay results in a hang - TBD */
>>>           if (1 || !llist_empty(&md->wait_llist) || force ||
>>>               test_bit(MD_HIGHPRIO, &md->status) ||
>>>               time_before(md->dirty_timeout, timeout) ||
>>>               ploop->force_md_writeback) {
>>> +            spin_lock_irqsave(&ploop->bat_lock, flags);
>>> +            if (test_bit(MD_UPDATING, &md->status) ||
>>> +                test_bit(MD_WRITEBACK, &md->status)) {
>>> +                /* if updating or there is a writeback then delay */
>>> +                spin_unlock_irqrestore(&ploop->bat_lock, flags);
>>> +                llist_add((struct llist_node *)&md->wb_link,
>>> +                      &ploop->wb_batch_llist);
>>> +                continue;
>>> +            }
>>> +
>>>               /* L1L2 mustn't be redirtyed, when wb in-flight! */
>>> -            WARN_ON_ONCE(!test_bit(MD_DIRTY, &md->status));
>>> -            WARN_ON_ONCE(test_bit(MD_WRITEBACK, &md->status));
>>> -            set_bit(MD_WRITEBACK, &md->status);
>>> -            clear_bit(MD_DIRTY, &md->status);
>>> +            if (WARN_ON_ONCE(!test_bit(MD_DIRTY, &md->status)))
>>> +                PL_ERR("wb but dirty not set\n");
>>
>> Maybe:
>>
>> WARN_ONCE(!test_bit(MD_DIRTY, &md->status), PL_FMT("wb but dirty not 
>> set"), ploop_device_name(ploop));
> 
> That may work, i will check.
> 
>>
>>> +
>>> +            if (WARN_ON_ONCE(test_bit(MD_WRITEBACK, &md->status)))
>>> +                PL_ERR("wb but writeback already set\n");
>>
>> Having both WARN and PL_ERR is a bit ugly.
> 
> Problem is they are on consecutive lines and it is hard to tell which 
> one triggered after compiler optimistations.


-                       if (WARN_ON_ONCE(!test_bit(MD_DIRTY, &md->status)))
-                               PL_ERR("wb but dirty not set\n");
+                       WARN_ONCE(!test_bit(MD_DIRTY, &md->status),
+                             PL_FMT("wb but dirty not set"),
+                             ploop_device_name(ploop));

-                       if (WARN_ON_ONCE(test_bit(MD_WRITEBACK, 
&md->status)))
-                               PL_ERR("wb but writeback already set\n");
+                       WARN_ONCE(test_bit(MD_WRITEBACK, &md->status),
+                             PL_FMT("wb but writeback already set"),
+                             ploop_device_name(ploop));

-- 
Regards,
Alexander Atanasov