Re: [Devel] [PATCH hci-8.1 v4 6/6] drivers/md/push_backup: add event file descriptor interface

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 11/3/26 11:03, Andrey Zhadchenko wrote:
> The legacy messages keep working unchanged for compatibility but we will
> clean it up later.

Hi Andrey,

I found no locking or lifetime bug here: the kref, pb->destroyed and
the reserve -> emit -> check -> install order in push_backup_get_fd()
hold up. A few things to settle before the UAPI is fixed.

The legacy path does change, for the better: pb_message() parsed u64
and passed it on as unsigned int, so "push_backup_write 4294967296:1"
acked cluster 0 and now gets -E2BIG, and push_backup_read printed "%u"
on devices with 2^32 or more clusters. Please say so instead of
"unchanged", or move these into a separate fix of 62e07487348e.

> Also destroy pbio object first before requeuing in resubmit_pbios():
> in-flight requests hold the target which we may have already stopped,
> so dm_requeue_original_rq() may trigger .dtr and invalidate mempool.

The requests pin the mapped device (dm_start_request() -> dm_get()),
not the target. Only the fd write path needs this, as it holds no md
reference. This could also be a small preparatory patch.

> +	ret = -ESTALE;
> +	if (!pb->map || !pb->alive)

The fd is bound to the target, not to a backup session. After
"push_backup_stop" and a new "push_backup_start" an old fd keeps
working on the new session. A reader blocked across the stop can even
miss -ESTALE: if start has set alive again before it re-checks
pb_has_event(), it sleeps on. A leftover tool then acks ranges of the
new backup, every ack clears map bits, and the new backup silently
loses its point-in-time consistency. The legacy messages behave the
same, so it is not a regression, but a new UAPI cannot be tightened
later. Could the file record a session generation, bumped on start and
stop, and return -ESTALE once it is over? If you keep it as is, please
say in the header that the fd survives stop/start.

> +static const struct file_operations pb_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= pb_fd_read,
> +	.write		= pb_fd_write,
> +	.release	= pb_fd_release,
> +};

read() honours O_NONBLOCK, but without .poll the fd cannot go into
epoll and poll() reports it always ready. pb->waitq and pb_has_event()
are already there, so .poll is a few lines.

On the header: userspace gets -1 and errno, not "-EAGAIN"; read() can
also fail with EBADMSG, EFAULT and EINTR; the fd is O_CLOEXEC. The fd
is installed before dm-ioctl copies the result out, so a failed
copy-out leaves it behind, and "dmsetup message ... push_backup_get_fd"
is of no use. One line on each would help tool authors.

After pb_destroy() the struct may live on through an fd with ti,
origin_dev, wq, pending_htable and pbio_pool dangling. Every fd path
checks pb->destroyed first, so this is fine today; NULLing them would
keep it so.

>  	while ((pbio = list_first_entry_or_null(list, struct pb_bio, list)) != NULL) {
> +		struct request *rq = pbio->rq;
>  		list_del_init(&pbio->list);

checkpatch wants a blank line after the declaration.

Commit message: "This way we will avoid aforementioned problems." can
go. Please also mention the target version bump to 1.1.0. Is that what
the tool checks to find the fd interface, or does it just try
"push_backup_get_fd" and fall back on -ENOTSUPP?

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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