Re: [Devel] [PATCH hci-8.1 v6 12/12] dm-ploop: truncate unused tail of top delta on suspend

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-ploop: ploop_postsuspend() holds ctl_rwsem across unbounded waits and fs I/O
--

commit 1aa3a8ea27bee4e747c238df1ff3b02cb36cb3fd
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: truncate unused tail of top delta on suspend

This patch adds ploop_calc_used_end() and ploop_truncate_unused_tail().
ploop_postsuspend() now truncates the unused tail of the top delta. It
does this under ctl_rwsem, so that ploop messages don't change the image
in parallel.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c

[ ... ]

>  static void ploop_postsuspend(struct dm_target *ti)
>  {
>  	struct ploop *ploop = ti->private;
> +	unsigned int noio_flags;
>  
> +	down_write(&ploop->ctl_rwsem);
>  	/*
>  	 * Let the next resumed ploop (maybe of reloaded table)
>  	 * see final size of the top delta. Don't wait in standby

[ ... ]

>  	wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
> -	ploop_set_suspended(ti, true);
> +	/* The device is suspended, see ploop_preresume() */
> +	noio_flags = memalloc_noio_save();
> +	ploop_truncate_unused_tail(ploop);
> +	memalloc_noio_restore(noio_flags);
> +	ploop->suspended = true;
> +	up_write(&ploop->ctl_rwsem);
>  }

[Severity: Low]
Is it necessary to take ctl_rwsem for write before the waits in
ploop_postsuspend()?

The write lock is now taken before the killable ploop_wait_prealloc() and
the uninterruptible wait_event(ploop->inflight_md_wq, ...), and it stays
held through ploop_truncate_unused_tail(), which calls vfs_iter_read()
(via ploop_calc_used_end()), vfs_fsync() and do_ftruncate().

Before this patch the lock was only taken briefly in ploop_set_suspended(),
after both waits. Those waits could already block for a long time. In
standby, or with a dead or stuck backing file, inflight_md may never reach
zero. Now other ctl_rwsem users block behind them as well:

ploop_status()
    down_read(&ploop->ctl_rwsem);   /* STATUSTYPE_TABLE */

ploop_message()
    if (read)
        down_read(&ploop->ctl_rwsem);
    else
        down_write(&ploop->ctl_rwsem);

Every message handler except get_event would hang while postsuspend is
stalled. The same stall can come from a slow vfs_fsync() or do_ftruncate()
on a stuck filesystem.

I don't see a deadlock. Nothing on the inflight_md completion path or in
the allocator takes ctl_rwsem. dm_suspend() also already hangs in that
state with md->suspend_lock held, so the practical impact looks small.

The two waits don't appear to need the lock. Only
ploop_truncate_unused_tail() and the store to ploop->suspended do. Would
it make sense to take the lock after the waits? That would avoid
extending the blocking to status and message handlers while the wait
phase is stuck.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013111604.142989-1-alexander.ivanov@virtuozzo.com?part=12

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