Re: [Devel] [PATCH hci-8.1 v3 0/6] dm/push_backup: event fd interface and fixes

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

LGTM with minor notes.

Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

On 11/3/26 07:17, Andrey Zhadchenko wrote:
> The push_backup target currently makes the backup tool wait for the next
> cluster range inside the "push_backup_read" target message. dm runs
> target_message() under dm_get_live_table(), i.e. with md->io_barrier SRCU
> read-locked, so while the tool sleeps there anything that needs
> synchronize_srcu(&md->io_barrier) on that device is stuck: suspend, table
> reload/remove and dm_internal_suspend_fast() from "@stats_create". An
> unrelated dm-stats user hangs, and everything that later queues behind
> md->suspend_lock hangs with it.
>
> Patch 6 adds a file descriptor based interface instead. The tool gets an
> anon inode fd via the "push_backup_get_fd" message and then uses plain
> read()/write() of struct dm_push_backup_range to fetch the next range and
> to acknowledge it, without going through dm messages on the data path. The
> fd pins struct push_backup with a kref so it survives target removal and
> simply returns -ENODEV afterwards. The legacy messages are kept for
> compatibility.
>
> Patches 1-5 are fixes found while working on this:
>
>    1. init the lock, waitqueue and timer before anything that can fail so
>       pb_destroy() is safe from any error path;
>    2. clear pb->alive under pb->lock like the other users do;
>    3. drop the redundant mod_timer() in push_backup_write(). The timer
>       re-arms itself from pb->deadline_jiffies, so updating the deadline
>       under the lock is enough. This was also the only place that armed
>       the timer outside the window push_backup_stop() waits for, which
>       matters once writes may arrive via the fd without pb->ctl_rwsem;
>    4. pass absolute expiry to mod_timer(). Both remaining callers passed
>       a relative value, so after ~5 min of uptime the timer fired and
>       re-armed on every tick while a write was delayed;
>    5. arm the timer under pb->lock and wait for its callback properly.
>       pb_timer_func() and postpone_if_required_for_backup() both decided
>       under the lock and armed after dropping it, so a stale S64_MAX could
>       overwrite a fresh deadline and park the timer ~12 days out with
>       delayed writes still pending. Also, timer_delete_sync_try() does not
>       wait for a running callback, so one blocked on pb->lock could outlive
>       push_backup_stop() and expire a later session. With arming done under
>       the lock, the callback checking pb->alive and stop/destroy waiting
>       via timer_delete_sync()/timer_shutdown_sync(), the RCU dance between
>       postpone and stop is no longer needed and is removed.
>
> v3:
>    - 5/6: pb_timer_func() bails out when !pb->alive; push_backup_stop()
>      uses timer_delete_sync() and pb_destroy() timer_shutdown_sync()
>      instead of WARNing on timer_delete_sync_try() < 0, which never waited
>      for a running callback. Commit message corrected accordingly.
>    - 6/6: push_backup_get_fd() installs the fd only after its number fits
>      into the result buffer (reserve -> emit -> check -> install), so a
>      too small buffer no longer leaks the fd with its pb kref and module
>      reference. pb->dead renamed to pb->destroyed. UAPI type namespaced
>      as struct dm_push_backup_range. read() now requires exactly
>      sizeof(range) like write(). Subject prefix aligned with the rest of
>      the series.
>
> v2:
>    - 2/6: reworded the commit message, dropped the KCSAN claim.
>    - 5/6: new patch: arm the deadline timer under pb->lock and drop the
>      rcu_read_lock()/synchronize_rcu() between
>      postpone_if_required_for_backup() and push_backup_stop().
>    - 6/6: resubmit_pbios() now frees the pbio before
>      dm_requeue_original_rq(). The requeue drops the md reference held by
>      the in-flight request, after which __dm_destroy() may run .dtr and
>      destroy pbio_pool under the fd writer's feet.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-152912
>
> Andrey Zhadchenko (6):
>    drivers/md/push_backup: correct initialization
>    drivers/md/push_backup: clear pb->alive under pb->lock
>    drivers/md/push_backup: do not re-arm deadline timer in
>      push_backup_write()
>    drivers/md/push_backup: fix mod_timer() usage
>    drivers/md/push_backup: arm deadline timer under pb->lock
>    drivers/md/push_backup: add event file descriptor interface
>
>   drivers/md/dm-push-backup.c         | 268 +++++++++++++++++++++-------
>   include/uapi/linux/dm-push-backup.h |  39 ++++
>   2 files changed, 246 insertions(+), 61 deletions(-)
>   create mode 100644 include/uapi/linux/dm-push-backup.h
>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel