[Devel] [RFC PATCH vz9 v6 21/62] dm-ploop: introduce per-md page locking

[Alexander Atanasov]

On 13.01.25 13:22, Pavel Tikhomirov wrote:
> On 12/6/24 05:55, Alexander Atanasov wrote:
>> @@ -1045,10 +1054,12 @@ static int 
>> ploop_check_delta_before_flip(struct ploop *ploop, struct file *file)
>>       /* Points to hdr since md_page[0] also contains hdr. */
>>       d_md = ploop_md_first_entry(&md_root);
>> -    write_lock_irq(&ploop->bat_rwlock);
>>       ploop_for_each_md_page(ploop, md, node) {
>>           init_be_iter(nr_be, md->id, &i, &end);
>>           d_bat_entries = d_md->kmpage;
>> +
>> +        read_lock_irqsave(&md->lock, flags);
>> +        read_lock(&d_md->lock);
>>           for (; i <= end; i++) {
>>               if (ploop_md_page_cluster_is_in_top_delta(ploop, md, i) &&
>>                   d_bat_entries[i] != BAT_ENTRY_NONE) {
> 
> Why do we replace _irq locks to _irqsave? 

Here it is not intentional, i will fix it. This is only a debug code.

> The rule of thumb is:
> 
> - lock with no prefix should be used in interrupt context,
> - lock with _irq prefix should be used in non-interrupt context,
> - lock with _irqsave prefix should be used in places where both 
> interrupt and non-interrupt context are possible.
> 
> For instance for the use of _irqsave in ploop_delay_if_md_busy (which 
> exists before this patch) we have both interrupt and process contexts:
> 
>    +-< ploop_delay_if_md_busy
>      +-< ploop_submit_cow_index_wb
>        +-< ploop_process_discard_pios
>          +-< do_ploop_run_work
>      +-< ploop_locate_new_cluster_and_attach_pio
>        +-< ploop_process_one_deferred_bio
>          +-< ploop_process_deferred_pios
>            +-< do_ploop_run_work
>          +-< ploop_submit_embedded_pio
>            +-< ploop_resume_submitting_pios
>              +-< ploop_suspend_submitting_pios
>                +-< ploop_resize
>                +-< ploop_merge_latest_snapshot
>                +-< ploop_delta_clusters_merged
>                +-< ploop_update_delta_index
>              +-< ploop_resize
>                +-< ploop_message
>              +-< ploop_merge_latest_snapshot
>                +-< ploop_message
>              +-< ploop_delta_clusters_merged
>                +-< ploop_notify_merged
>                  +-< ploop_message
>              +-< ploop_update_delta_index
>                +-< ploop_message
>            +-< ploop_enospc_timer # interrupt context from timer
>      +-< ploop_process_one_discard_pio
>        +-< ploop_process_discard_pios
>          +-< do_ploop_run_work
> 
>    +-< ploop_message
>      +-< ploop_target->message
>        +-< target_message # ioctl - process context
> 
>    +-< do_ploop_run_work
>      +-< do_ploop_work # workqueue - process context
>      +-< ploop_worker # kthread - process context
> 
> If we'll change everything to _irqsave we would lose deep understanding 
> why we need to protect against irq context at all, now it looks that we 
> only need it for enospc timers.
> 

I am not sure - we are called from device mapper and i am not aware if 
it does any guarantees about in what context we will be called.

ploop_enospcs_timer can end up executing the whole request -
ploop_submit_embedded_pio and if fast path is enabled it  can go down to 
call into vfs_layer. Besides it also does preparation which uses locks.

But agreed it needs checking. And the knowledge to be created.


-- 
Regards,
Alexander Atanasov