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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

On 10/13/26 13:35, no-reply@sashiko.virtuozzo.com wrote:
> 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?
Known and documented: it needs a reload plus a fatal signal exactly 
during this wait. Postsuspend can't fail, so there's no way to refuse, 
and having no wait at all was strictly worse.
>> 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.
Skipping the wait or making it killable just moves the hang to resume or 
dtr, which must wait for in-flight BAT writes anyway. Unlike the 
allocator, a BAT write is a plain pio, so it completes, with an error if 
need be. If the file is dead enough for it to hang, data IO hangs too 
and suspend never gets this far.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel