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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-ploop: ploop_postsuspend() waits uninterruptibly for BAT writeback, even in standby
- [Medium] dm-ploop: new ploop keeps suspended=false after failed preresume on a suspended DM device
- [Medium] dm-ploop: postsuspend ignores killed prealloc wait, old allocator can resize top delta after reload
- [Low] dm-ploop: per-instance standby flag is never cleared, so reload is skipped
--

commit fe4b80dd50a64ca3e5385df8c28c40c9de854171
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, tracks
standby per ploop instance, and makes ploop_postsuspend() wait for
pending preallocation and for inflight BAT writeback.

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

[ ... ]

> +void ploop_wait_prealloc(struct ploop *ploop)
> +{
> +	wait_event_killable(ploop->dispatcher_wq_prealloc,
> +			    ploop_no_pending_prealloc(ploop));
> +}

[Severity: Medium]
ploop_wait_prealloc() discards the return value of wait_event_killable().
If the suspending task is killed while the old allocator is still inside
ploop_preallocate_cluster(), ploop_postsuspend() carries on and marks
the device suspended:

ploop_postsuspend()
    ploop_wait_prealloc()     /* -ERESTARTSYS dropped */
    wait_event(inflight_md_wq, ...)
    ploop_set_suspended(ti, true);

A reloaded table can then resume.  Its ploop_preresume() only checks
ploop_no_pending_prealloc() on the new ploop, so it cannot see the old
instance's in-flight allocation.  When the old allocator finishes, it
runs:

ploop_preallocate_cluster()
    new_len = top->file_size + ploop->prealloc_in_progress;
    ...
    ploop_truncate_prealloc_safe(ploop, top->file_size, new_len, ...)

new_len was computed from the old, stale file_size.  Can this set the
file size below what the new ploop has already reloaded and extended,
cutting off clusters the new ploop allocated?

The comment in ploop_postsuspend() acknowledges this ("a late truncate of
our allocator may shrink the file under it"), but nothing seems to handle
it.  Should the wait result be propagated, or the old allocator otherwise
fenced?

> 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)
> [ ... ]
> +	 */
> +	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]
The comment above says the prealloc wait is skipped in standby and made
killable so that dm_replace() of libploop can suspend a device whose
backing file is dead.  Is the new wait_event() on inflight_md_wq missing
the same escape?

inflight_md is raised in ploop_prepare_bat_update():

    piwb->type = type;
    atomic_inc(&ploop->inflight_md);

and is only dropped through ploop_dec_inflight_md(), from
ploop_put_piwb() or ploop_break_bat_update().  The BAT write is queued
from ploop_index_wb_submit() via ploop_runners_add_work() and completes
only when the backing file completes the I/O.

ploop_check_standby_mode() only reacts to -EBUSY, -ENOTCONN or -EIO, so
entering standby does not guarantee that a pending BAT write completes.
If the backing file stalls without returning an error, can
ploop_postsuspend() sleep forever in D state here?

ploop_presuspend() waits only on kt_worker->inflight_pios, so it does not
drain inflight_md.  A BAT write with no attached data pio would not be
covered by that wait.

Should this wait be skipped in standby and made killable, like the
prealloc wait just above it?

Also, the later patch "dm-ploop: truncate unused tail of top delta on
suspend" keeps this wait_event() and moves it inside
down_write(&ploop->ctl_rwsem).  In that state a stuck wait also blocks
ploop_message() and ploop_preresume() behind ctl_rwsem.

[ ... ]

>  static int ploop_preresume(struct dm_target *ti)
>  {
[ ... ]
> +	down_write(&ploop->ctl_rwsem);
> +	if (ploop->noresume) {
>  		ret = -EAGAIN;
> +	} else if (!ploop_is_standby(ploop) &&
> +		   ploop_no_pending_prealloc(ploop)) {
> +		reload = true;
[ ... ]
> +		ret = ploop_reload_metadata(ploop);
> +		memalloc_noio_restore(noio_flags);
> +	}
> +	up_write(&ploop->ctl_rwsem);
>  
>  	if (ret == 0) {
[ ... ]
>  		ploop_set_suspended(ti, false);
>  		ploop_set_wants_suspend(ti, false);
>  	}
>  	return ret;
>  }

[Severity: Medium]
On a reloaded table, the new ploop never runs ploop_postsuspend(), and
ploop_ctr() never initializes ->suspended, so it stays false from
kzalloc.  If ploop_reload_metadata() fails (-ENOMEM, -EIO, -EBADSLT) or
noresume is set, ploop_preresume() returns an error and skips
ploop_set_suspended(ti, false), while DM keeps the device suspended.

do_resume() then still runs dm_table_destroy(old_map) even though
dm_resume() failed.  That destroys the old ploop together with its
top_flipped state and in-memory IN_USE signature.

The live ploop now reports suspended == false while the DM device is
suspended.  The flip_upper_deltas message handler in dm-ploop-cmd.c
checks:

    if (!ploop->suspended || !ploop->noresume || ploop->maintaince)
        return -EBUSY;

so it would return -EBUSY until a resume succeeds and the device is
suspended again.  Is that intended?  I have not checked how libploop
uses flip_upper_deltas after a failed resume, so this may be harmless in
practice.  Should the new ploop's suspended flag be synchronized with DM
state on this path?

> 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
> @@ -1446,6 +1452,7 @@ static void ploop_check_standby_mode(struct ploop *ploop, long res)
>  	spin_lock_irqsave(&q->queue_lock, flags);
>  	prev = test_and_set_bit(QUEUE_FLAG_STANDBY, &q->queue_flags);
>  	spin_unlock_irqrestore(&q->queue_lock, flags);
> +	WRITE_ONCE(ploop->standby, true);

[Severity: Low]
This is the only place ploop->standby is written, and nothing in
drivers/md/dm-ploop-*.c clears it.  The queue bit, by contrast, is
cleared in ploop_ctr():

    if (blk_queue_standby(ploop_blk_queue(ploop))) {
        blk_queue_flag_clear(QUEUE_FLAG_STANDBY, ...);

so the two can diverge.  Once set, ploop_preresume() permanently skips
ploop_reload_metadata() and the following ploop_should_prealloc() for
this instance:

    } else if (!ploop_is_standby(ploop) &&
               ploop_no_pending_prealloc(ploop)) {

and ploop_postsuspend() permanently skips ploop_wait_prealloc().

Consider this sequence: ploop_check_standby_mode() sets ->standby, a new
table is loaded and ploop_ctr() clears the queue bit, the new table is
abandoned, and userspace resumes the old instance.  The queue is no
longer in standby, but the old ploop skips the reload and runs on
possibly stale BAT and file sizes.

The commit message says userspace must not resume the previous instance
on the same images, but the driver does not enforce that.  Should the
flag be cleared when the instance leaves standby, for example on a
successful resume?

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

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