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
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
