Re: [Devel] [PATCH hci-8.1 2/3] dm-ploop: renumber BAT levels when the latest snapshot is merged

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

On 10/27/26 17:32, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> ploop_merge_latest_snapshot() drops the delta below the top one and
> moves the top delta down a level, but leaves bat_levels alone. Every
> cluster of the top delta then sits one level above the new top.
> 
> Checks done with "< top" do not notice, but the flip of the upper
> deltas compares levels for equality and leaves these entries alone.
> After the flip they are still taken for the top delta, so their I/O
> goes to the delta that has just become the top one, at offsets that
> belong to the other file. An ALLOC or RELOC writeback resets the
> level of every entry it writes back, which hid this for the pages
> that happened to get one.
> 
> Lower the levels above the dropped delta, as notify_delta_merged()
> does for a delta merged from userspace.
> 
> Fixes: 6ddad5a4d226 ("dm-ploop: Add ploop target driver")
> https://virtuozzo.atlassian.net/browse/VSTOR-151979
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> 
> Feature: dm-ploop: ploop target driver
> ---
>   drivers/md/dm-ploop-cmd.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
> index 5fa78e6cff9a..cfbba9b3e234 100644
> --- a/drivers/md/dm-ploop-cmd.c
> +++ b/drivers/md/dm-ploop-cmd.c
> @@ -721,6 +721,27 @@ static int ploop_process_merge_latest_snapshot(struct ploop *ploop)
>   }
>   ALLOW_ERROR_INJECTION(ploop_process_merge_latest_snapshot, ERRNO);
>   
> +static void ploop_renumber_levels_above(struct ploop *ploop, u8 level)
> +{
> +	u32 i, end, *bat_entries;
> +	struct md_page *md;
> +	struct rb_node *node;
> +	unsigned long flags;
> +
> +	ploop_for_each_md_page(ploop, md, node) {
> +		ploop_init_be_iter(ploop, md->id, &i, &end);
> +		bat_entries = md->kmpage;
> +
> +		spin_lock_irqsave(&md->md_lock, flags); /* write */

Not blocking, but I don't think we need spinlock here. What actually 
protects us is that pios are suspended. There are no other users if pios 
are suspended and we are in the command already.
If we weren't suspended we would have another problem with half pages 
with correct level and half with the wrong.
So either delete spinlock or take it outside the cycle.

> +		for (; i <= end; i++) {
> +			if (READ_ONCE(bat_entries[i]) != BAT_ENTRY_NONE &&
> +			    READ_ONCE(md->bat_levels[i]) > level)
> +				md->bat_levels[i]--;
> +		}
> +		spin_unlock_irqrestore(&md->md_lock, flags);
> +	}
> +}
> +
>   static int ploop_merge_latest_snapshot(struct ploop *ploop)
>   {
>   	u8 level;
> @@ -748,6 +769,7 @@ static int ploop_merge_latest_snapshot(struct ploop *ploop)
>   
>   	ploop->deltas[level] = ploop->deltas[level + 1];

We can also zero ploop->deltas[level + 1] just in case so we do not have 
any stale info afterwards.

>   	ploop->nr_deltas--;
> +	ploop_renumber_levels_above(ploop, level);
>   
>   	ploop_resume_submitting_pios(ploop);
>   out:

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel