Re: [Devel] [PATCH hci-8.1 v6 6/7] dm/push_backup: do not re-arm deadline timer in push_backup_write()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm/push_backup: commit message says timer is 'always pending' while delayed
--

commit 5c8b11d5d9c6ee6b26e3830c348cda4ca8f1bbca
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

dm/push_backup: do not re-arm deadline timer in push_backup_write()

This patch removes the mod_timer() call from push_backup_write(). The
commit message says pb_timer_func() and
postpone_if_required_for_backup() keep the deadline timer pending while
writes are delayed, so the extra re-arm is not needed. The remaining code
only updates pb->deadline_jiffies under pb->lock.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-152912

> dm/push_backup: do not re-arm deadline timer in push_backup_write()
>
> pb_timer_func() checks pb->deadline_jiffies every time it fires and
> re-arms itself under pb->lock while requests are delayed, and
> postpone_if_required_for_backup() arms it under pb->lock when the first
> request is delayed. So the timer is always pending while writes are
> delayed and we don't need to re-arm it after updating the field.

[Severity: Low]
This isn't a functional problem, but is the statement "the timer is
always pending while writes are delayed" accurate?

In pb_timer_func(), the expired branch does not re-arm the timer:

drivers/md/dm-push-backup.c:pb_timer_func() {
    ...
	if (unlikely(time_before64(now, deadline)))
		mod_timer(timer, jiffies + (deadline - now) + 1);
	else
		queue_work(pb->wq, &pb->worker);
}

If nr_delayed is non-zero and now >= deadline_jiffies, the timer calls
queue_work() and is not pending. From then until do_pb_work() runs
cleanup_backup(), nr_delayed can be non-zero while the timer is not
pending.

Removing the mod_timer() from push_backup_write() still looks safe in this
window. The queued worker drains every delayed pbio unconditionally, so a
deadline extension made by push_backup_write() cannot prevent expiry,
with or without the removed mod_timer().

Could the message be reworded to say the timer is pending, or the expiry
work is already queued?

> diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
> --- a/drivers/md/dm-push-backup.c
> +++ b/drivers/md/dm-push-backup.c

[ ... ]

> @@ -515,8 +515,7 @@ static int push_backup_write(struct push_backup *pb,
>  		}
>  	}
>  
> -	has_more = (pb->nr_delayed != 0);
> -	if (has_more)
> +	if (pb->nr_delayed)
>  		pb->deadline_jiffies = get_jiffies_64() + pb->timeout_in_jiffies;
>  	else
>  		pb->deadline_jiffies = S64_MAX;

[ ... ]

> -	if (!list_empty(&pbio_list)) {
> +	if (!list_empty(&pbio_list))
>  		resubmit_pbios(pb, &pbio_list);
> -		if (has_more)
> -			mod_timer(&pb->deadline_timer, jiffies + pb->timeout_in_jiffies + 1);
> -	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104083818.856720-1-andrey.zhadchenko@virtuozzo.com?part=6

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