Re: [Devel] [PATCH hci-8.1 v4 11/11] 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:
- [Medium] dm-ploop: ctl_rwsem held across unbounded waits in ploop_postsuspend()
--

commit d2ae283df5af32ea17061c2b914cc8df355ea0dd
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 uses it from ploop_postsuspend()
to truncate the unused tail of the top delta file. The truncation runs
under ctl_rwsem, which is now taken at the start of ploop_postsuspend().

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

[ ... ]

> +	if (!ploop_is_standby(ploop))
> +		ploop_wait_prealloc(ploop);
>  	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: Medium]
Can ctl_rwsem being held across these waits cause a hang?

ploop_postsuspend() now takes down_write(&ploop->ctl_rwsem) before
ploop_wait_prealloc() and before the wait_event() on inflight_md_wq.
Previously those waits ran without the lock, and ploop_set_suspended()
took it only briefly.

ploop_wait_prealloc() is only killable, and the wait_event() on
inflight_md_wq is not interruptible at all. ploop_calc_used_end() does
a vfs_iter_read() of the BAT, and do_ftruncate() is also IO-bound. If
the top delta stalls before ploop->standby has been set, none of these
waits may finish while the write lock is held.

In that state, ploop_status() for STATUSTYPE_TABLE blocks in
down_read(&ploop->ctl_rwsem). ploop_message() blocks in
down_read()/down_write() as well. Only get_event does not take the lock.
Recovery messages such as set_noresume and flip_upper_deltas are also
blocked.

The commit message says the standby skip exists so that dm_replace() of
libploop can still replace or remove a device stuck on a dead file. Does
holding the write lock across the earlier waits undermine that goal?

From the code, ctl_rwsem only needs to cover ploop_truncate_unused_tail()
and the update of ploop->suspended. Could the lock be taken after the two
waits instead?

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

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