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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 11/3/26 11:03, Andrey Zhadchenko wrote:
> Patches 1-5 are fixes found while working on this:

Hi Andrey,

Thanks, the direction is right and the fd interface itself holds up.
Remarks are inline in the patches, the main ones:

- 5/6: the !pb->alive bail-out in pb_timer_func() can leave guest
  writes delayed for good after an interrupted "push_backup_stop".
  Checking pb->nr_delayed instead keeps everything the patch needs.
- 3/6 opens a window that only 5/6 closes, so please put 5/6 first or
  fold 3/6 into it.
- 6/6: the fd is not bound to a backup session; please decide this
  before the UAPI is fixed.
- pb_ctr() returns 0 with a freed pb when alloc_ordered_workqueue()
  fails. It is older than the series, but 1/6 is about exactly these
  error paths.

Series-wide:
- Every vz commit needs a "Feature:" line. The earlier dm-push-backup
  commits carry "Feature: dm-backup: push backup target driver".
- The subject prefix of the driver history and of this cover letter is
  "dm/push_backup:", the patches use "drivers/md/push_backup:". The 3/6
  subject is 75 characters.

>   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;

Same as in 3/6: pb_timer_func() also re-arms outside that window, so
"the only place" does not hold.

>   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;

On 64-bit this happens from boot, not after 5 minutes, see 4/6.

>   5. arm the timer under pb->lock and wait for its callback properly.

This item is accurate and is the description 5/6 itself should carry.
It is one paragraph of six sentences though; splitting it into the
race, the stop race and the RCU removal would read easier.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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