Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dm-ploop: ploop_destroy() cancels event_work once, schedule_work() can still requeue it - [Medium] dm-ploop: ploop_destroy() strands pios parked on enospc_pios after timer shutdown --
commit 3fa591e8ee9efb43d90f0c5c36257c3be76e0d1e Author: Denis V. Lunev <[email protected]> dm-ploop: stop enospc_timer and event_work in ploop_destroy() This patch calls timer_shutdown_sync() on enospc_timer and cancel_work_sync() on event_work at the start of ploop_destroy(). The aim is to stop the timer and work from running on a struct ploop that ploop_dtr() has already freed. > 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 [ ... ] > @@ -164,6 +164,8 @@ static void ploop_destroy(struct ploop *ploop) > int i; > > if (ploop->kt_worker) { > + timer_shutdown_sync(&ploop->enospc_timer); > + cancel_work_sync(&ploop->event_work); [Severity: Medium] Can event_work be queued again after this cancel_work_sync()? ploop_try_delay_enospc() tests wants_suspend with a plain read and no deferred_lock: if (unlikely(ploop->wants_suspend)) { PL_WARN("dropping pio enospc\n"); return false; } ... mod_timer(&ploop->enospc_timer, jiffies + PLOOP_ENOSPC_TIMEOUT); schedule_work(&ploop->event_work); timer_shutdown_sync() turns the late mod_timer() into a no-op, but nothing neutralises the unconditional schedule_work(). Suppose an ENOSPC endio has read wants_suspend == false, and is then delayed across ploop_presuspend() and dm teardown until ploop_destroy() has passed cancel_work_sync(). It then queues event_work, and do_ploop_event_work() reaches dm_table_event(ploop->ti->table) after kfree(ploop). ploop_do_pio_endio() calls ploop_dec_nr_inflight() before endio_cb, and presuspend waits only on kt_worker->inflight_pios, so presuspend does not order against an endio that is still running. __dm_destroy() calls presuspend and postsuspend without waiting for in-flight requests. This window is much narrower than the one the patch closes. Is there something that guarantees no ENOSPC endio can still be running when ploop_destroy() is entered? If not, could the cancel be repeated after the producers are quiesced, for example after the kthreads are stopped? [Severity: Medium] Does shutting down the timer here strand pios parked on enospc_pios? Before this patch, a timer armed late would set submit_enospc and wake kt_worker, and do_ploop_run_work() would call ploop_resubmit_enospc_pios() during the drain in ploop_destroy(). Consider this sequence: ploop_prq_endio() gets BLK_STS_NOSPC ploop_try_delay_enospc() reads wants_suspend == false ploop_presuspend() ploop_resubmit_enospc_pios() <- final drain already done ploop_try_delay_enospc() llist_add(... &ploop->enospc_pios); mod_timer(&ploop->enospc_timer, ...); ploop_destroy() timer_shutdown_sync(&ploop->enospc_timer); With the timer now permanently disabled, nothing on the destroy path calls ploop_resubmit_enospc_pios(). The drain loop in ploop_destroy() covers FLUSH, PREPARE, DEFERRED, DISCARD and COW but not enospc_pios. Can the request and its prq be leaked here without the dm request being ended, and can the WARN_ON(!llist_empty(&ploop->enospc_pios)) later in ploop_destroy() fire? Would calling ploop_resubmit_enospc_pios() after the timer is stopped, or adding enospc_pios to the drain condition, cover this case? > ploop->force_md_writeback = 1; > wake_up_process(ploop->kt_worker->task); > /* try to send all pending - if we have partial io and enospc > end bellow */ -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=1 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
