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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-ploop: postsuspend waits unkillably on inflight_md even in standby
- [Medium] dm-ploop: killed wait in postsuspend() leaves prealloc pending after device is marked suspended
--

commit 881f9bc269db1bbaa88bf766f12f3d91d98c7d92
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 to after the reload and makes
postsuspend wait for pending preallocation and for inflight_md. Standby
state is now tracked per ploop instead of through the queue bit.

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

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

[ ... ]

> @@ -1259,6 +1259,12 @@ static int ploop_no_pending_prealloc(struct ploop *ploop)
>  	return ret;
>  }
>  
> +void ploop_wait_prealloc(struct ploop *ploop)
> +{
> +	wait_event_killable(ploop->dispatcher_wq_prealloc,
> +			    ploop_no_pending_prealloc(ploop));
> +}
> +

[Severity: Medium]
The return value of wait_event_killable() is discarded here, so a killed
wait looks the same as a completed one to the caller.

Can this leave a prealloc pending after the device is marked suspended?

Suppose the dmsetup task gets a fatal signal while ploop_postsuspend() is
in ploop_wait_prealloc(), and prealloc_size or prealloc_in_progress is
still non-zero. Then:

ploop_postsuspend()
    ploop_wait_prealloc()          /* returns -ERESTARTSYS, ignored */
    wait_event(ploop->inflight_md_wq, ...);
    ploop_set_suspended(ti, true);

The postsuspend callback returns void, so dm_suspend() cannot abort. The
old allocator thread keeps running and can still extend or truncate the top
delta.

The new ploop's ploop_preresume() checks only its own prealloc state, which
is zero:

ploop_preresume()
    } else if (!ploop_is_standby(ploop) &&
               ploop_no_pending_prealloc(ploop)) {
        reload = true;
        ...
        ret = ploop_reload_metadata(ploop);

It therefore reloads and caches a file size that the old allocator is still
changing. Can the old allocator's later truncate then cut off clusters that
the new ploop has already written?

The new comment in ploop_postsuspend() says "a ploop of a reloaded table may
see a stale size, and a late truncate of our allocator may shrink the file
under it", but nothing in the code prevents that outcome. Is there a way to
detect the killed wait, for example by checking the return value and
refusing to proceed?

> 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 the wait is skipped or killed, a ploop of a reloaded
> +	 * table may see a stale size, and a late truncate of our
> +	 * allocator may shrink the file under it.
> +	 */
> +	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));

[Severity: Medium]
The prealloc wait above is skipped in standby and is killable, because the
comment says the allocator may be stuck on a dead file. The inflight_md
wait here is neither.

Can this wait_event() hang suspend in uninterruptible sleep on a failing
device?

inflight_md only drops when the BAT write pio completes:

ploop_index_wb_submit()
    ploop_runners_add_work(ploop, pio);   /* file IO on the top delta */
...
ploop_put_piwb()
    ploop_dec_inflight_md()
        if (atomic_dec_and_test(&ploop->inflight_md))
            wake_up_locked(&ploop->inflight_md_wq);

If the device is in standby because of a hung backing file and a BAT write
is in flight on that same file, inflight_md never reaches zero.
ploop_presuspend() only waits for kt_worker->inflight_pios, so it does not
drain this.

Before this patch postsuspend never waited on inflight_md, so dm_replace()
could suspend such a device. Should this wait also be skipped in standby
and made killable, like ploop_wait_prealloc()?

A follow-up patch in the series, 'dm-ploop: truncate unused tail of top
delta on suspend', keeps this wait_event() unchanged and moves the call
under ctl_rwsem, so a hang there would hold ctl_rwsem for write while
waiting.

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

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