Re: [Devel] [PATCH hci-8.1 v4 10/11] dm-ploop: reload metadata on resume

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-ploop: ploop_postsuspend() waits uninterruptibly for BAT writeback, even in standby
--

Subject: Re: [PATCH] dm-ploop: reload metadata on resume

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

dm-ploop: reload metadata on resume

This patch reloads the top delta header, the BAT of every delta and the
delta file sizes in ploop_preresume(), and rebuilds the BAT tree and
holes_bitmap. It also moves preallocation from ploop_ctr() to
ploop_preresume(), tracks standby per ploop, and adds waits in
ploop_postsuspend() for pending preallocation and for inflight BAT
writeback.

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;
> +
> +	/*
> +	 * Let the next resumed ploop (maybe of reloaded table)
> +	 * see final size of the top delta. Don't wait in standby
> +	 * mode: the allocator may be stuck on the dead file, while
> +	 * dm_replace() of libploop must be able to suspend the device
> +	 * to recover it. The wait is killable for the same reason.
> +	 * [ ... ]
> +	 */
> +	if (!ploop_is_standby(ploop))
> +		ploop_wait_prealloc(ploop);
> +	/*
> +	 * Data bios are completed before BAT writeback is finished
> +	 * (see ploop_put_piwb()), so wait for it explicitly to make
> +	 * md pages clean for ploop_reload_metadata().
> +	 */
> +	wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
>  	ploop_set_suspended(ti, true);
>  }

[Severity: Medium]
Can this wait_event() hang suspend when the backing file is dead?

The preallocation wait just above is skipped in standby and is killable,
because the allocator may be stuck on the dead file and dm_replace() of
libploop has to be able to suspend the device to recover it.

The inflight_md wait has no such escape. It is uninterruptible, and it is
also reached when ploop_is_standby() is true.

inflight_md is decremented only when the BAT write kiocb completes:

ploop_bat_write_complete()
  ploop_put_piwb()
    ploop_dec_inflight_md()
      atomic_dec_and_test(&ploop->inflight_md)
      wake_up_locked(&ploop->inflight_md_wq)

Standby is set only in ploop_check_standby_mode(), after some IO has
already completed with -EBUSY, -ENOTCONN or -EIO:

ploop_check_standby_mode()
    ...
    WRITE_ONCE(ploop->standby, true);

That does not cancel or complete other BAT writes already in flight. If one
of them is stuck on the same dead file, inflight_md stays above zero and the
suspending task sits in D state.

ploop_postsuspend() returns void, so the wait cannot be turned into an
error. Whether a BAT write to a dead file hangs or fails depends on the
backing filesystem, so I could not confirm this from the code alone.

The follow-up patch "dm-ploop: truncate unused tail of top delta on
suspend" keeps this as a plain wait_event(), and moves it to run under
ctl_rwsem held for write. Other paths that take ctl_rwsem would then also
block behind it.

Would it make sense to make this wait killable, or to skip or bound it in
standby, as is done for the preallocation wait?

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

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