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

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

On 11/3/26 11:11, Konstantin Khorenko wrote:
> 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.

Hypothetical situation that has little to do with kernel code in my 
opinion. Surprisingly the userspace code must also be competent to not 
blow itself. I do not believe this justifies extra complexity in 
handling this fds.

> 
>> +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.

It is the few lines no one will use. We have a separate threads and this 
is better already: poll adds one more sycall; polling different disks in 
one consumer increases latency for everyone.

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

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