<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div id="appendonsend"></div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof">
<br>
</div>
<hr style="display: inline-block; width: 98%;">
<div id="divRplyFwdMsg">
<div style="direction: ltr; font-family: Calibri, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);">
<b>From:</b> Pavel Tikhomirov <ptikhomirov@virtuozzo.com><br>
<b>Sent:</b> Thursday, September 18, 2025 8:34 AM<br>
<b>To:</b> Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>; Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>; Konstantin Khorenko <khorenko@virtuozzo.com><br>
<b>Cc:</b> devel@openvz.org <devel@openvz.org><br>
<b>Subject:</b> Re: [Devel] [PATCH vz9/vz10] dm-ploop: fix concurrent COW operations on same cluster</div>
<div style="direction: ltr;"> </div>
</div>
<div style="font-size: 11pt;" class="elementToProof"><br>
<br>
On 9/16/25 22:02, Vasileios Almpanis wrote:<br>
> When multiple threads attempt operations on the same cluster simultaneously,<br>
> the current code logs warnings but continues with potentially conflicting<br>
> operations, leading to WARN_ON triggers.<br>
><br>
> Fix by implementing proper deferral when cluster locks fail:<br>
><br>
> 1. Change ploop_link_pio() to add identical pios to the lock holder's<br>
> endio list instead of issuing warnings and continuing<br>
> 2. Rename ploop_add_cluster_lk() to ploop_try_add_cluster_lk() to reflect<br>
> the new behavior<br>
> 3. Update COW and discard paths to abort operations when cluster locking<br>
> fails, allowing proper deferral.<br>
><br>
> This ensures that conflicting operations are serialized rather than<br>
> corrupting shared data structures.<br>
><br>
> Observed during snapshot creation where filesystem metadata updates<br>
> trigger concurrent operations on the same clusters.<br>
><br>
> <a data-auth="NotApplicable" class="OWAAutoLink" id="OWA3765b225-abec-a2a4-55f6-9eb5749d7678" href="https://virtuozzo.atlassian.net/browse/VSTOR-115446">
https://virtuozzo.atlassian.net/browse/VSTOR-115446</a><br>
> Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com><br>
><br>
> Feature: dm-ploop: ploop target driver<br>
> ---<br>
> drivers/md/dm-ploop-map.c | 33 +++++++++++++++++++--------------<br>
> 1 file changed, 19 insertions(+), 14 deletions(-)<br>
><br>
> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c<br>
> index 56481d0f8323..db89a668e766 100644<br>
> --- a/drivers/md/dm-ploop-map.c<br>
> +++ b/drivers/md/dm-ploop-map.c<br>
> @@ -535,10 +535,10 @@ static int ploop_link_pio(struct hlist_head head[], struct pio *pio,<br>
> if (pe == pio)<br>
> return 1;<br>
> <br>
> - WARN_ON_ONCE(pe != NULL);<br>
> -<br>
> - if (pe)<br>
> - PL_ERR("clu:%u already exclusively locked\n", clu);<br>
> + if (pe) {<br>
> + ploop_add_endio_pio(pe, pio);<br>
> + return 0;<br>
> + }<br>
> }<br>
> <br>
> if (!hlist_unhashed_lockless(&pio->hlist_node)) {<br>
> @@ -569,7 +569,7 @@ static int ploop_unlink_pio(struct ploop *ploop, struct pio *pio,<br>
> return 1;<br>
> }<br>
> <br>
> -static int ploop_add_cluster_lk(struct ploop *ploop, struct pio *pio, u32 clu)<br>
> +static int ploop_try_add_cluster_lk(struct ploop *ploop, struct pio *pio, u32 clu)<br>
> {<br>
> unsigned long flags;<br>
> int ret;<br>
> @@ -741,6 +741,10 @@ static int ploop_handle_discard_pio(struct ploop *ploop, struct pio *pio,<br>
> if (ploop->nr_deltas != 1)<br>
> goto punch_hole;<br>
> <br>
> + if (!ploop_try_add_cluster_lk(ploop, pio, clu)) {<br>
> + return 1;<br>
> + }<br>
<br>
Please remove excess {} brackets.<br>
<br>
> Do not unnecessarily use braces where a single statement will do.<br>
<br>
<a data-auth="NotApplicable" class="OWAAutoLink" id="OWA0937d644-a30d-2367-6c2b-3b53dc1b0e7b" href="https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces">https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces</a><br>
<br>
> +<br>
> spin_lock_irqsave(&ploop->inflight_lock, flags);<br>
> inflight_h = ploop_find_inflight_bio(ploop, clu);<br>
> if (inflight_h)<br>
> @@ -754,8 +758,6 @@ static int ploop_handle_discard_pio(struct ploop *ploop, struct pio *pio,<br>
> return 0;<br>
> }<br>
> <br>
> - if (!ploop_add_cluster_lk(ploop, pio, clu))<br>
> - PL_ERR("dis clu %d already locked\n", clu);<br>
<br>
Why do we move it above inflight-delay code? Is there a specific reason?<br>
Except that code looks good.<br>
<br>
You are right, we should leave it as is. There is no particular reason for moving it.<br>
<br>
> pio->wants_discard_index_cleanup = true;<br>
> <br>
> punch_hole:<br>
> @@ -1585,12 +1587,18 @@ static int ploop_submit_cluster_cow(struct ploop *ploop, unsigned int level,<br>
> {<br>
> struct ploop_cow *cow = NULL;<br>
> struct pio *aux_pio = NULL;<br>
> + int ret = 0;<br>
> <br>
> /* Prepare new delta read */<br>
> aux_pio = ploop_alloc_pio_with_pages(ploop);<br>
> cow = kmem_cache_alloc(cow_cache, GFP_ATOMIC);<br>
> - if (!aux_pio || !cow)<br>
> - goto err;<br>
> + if (!aux_pio || !cow) {<br>
> + ret = -ENOMEM;<br>
> + goto cleanup;<br>
> + }<br>
<br>
Please add newline here to separate allocation checks from cluster<br>
locking to improve code readability.<br>
<br>
> + if (!ploop_try_add_cluster_lk(ploop, cow_pio, clu))<br>
> + goto cleanup;<br>
> +<br>
> ploop_init_pio(ploop, REQ_OP_READ, aux_pio);<br>
> ploop_pio_prepare_offsets(ploop, aux_pio, clu);<br>
> aux_pio->css = cow_pio->css;<br>
> @@ -1602,17 +1610,14 @@ static int ploop_submit_cluster_cow(struct ploop *ploop, unsigned int level,<br>
> cow->aux_pio = aux_pio;<br>
> cow->cow_pio = cow_pio;<br>
> <br>
> - if (!ploop_add_cluster_lk(ploop, cow_pio, clu))<br>
> - PL_ERR("cowclu %d already locked\n", clu);<br>
> -<br>
> /* Stage #0: read secondary delta full clu */<br>
> ploop_map_and_submit_rw(ploop, dst_clu, aux_pio, level);<br>
> return 0;<br>
> -err:<br>
> +cleanup:<br>
> if (aux_pio)<br>
> ploop_free_pio_with_pages(ploop, aux_pio);<br>
> kfree(cow);<br>
> - return -ENOMEM;<br>
> + return ret;<br>
> }<br>
> <br>
> static int ploop_initiate_cluster_cow(struct ploop *ploop, unsigned int level,<br>
<br>
--<br>
Best regards, Pavel Tikhomirov<br>
Senior Software Developer, Virtuozzo.<br>
<br>
</div>
</body>
</html>