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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-push-backup: event fd pb_fops has no .poll, so epoll/poll always report ready
--

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

drivers/md/push_backup: add event file descriptor interface

This adds a "push_backup_get_fd" target message that returns an anon inode
fd. The backup tool can read() that fd to get the next cluster range and
write() it to acknowledge the range. This avoids sleeping inside a dm
message with md->io_barrier held. The patch also adds a kref on struct
push_backup to tie its lifetime to the open fds.

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

> 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

[ ... ]

> +static bool pb_has_event(struct push_backup *pb)
> +{
> +	return READ_ONCE(pb->destroyed) || !READ_ONCE(pb->map) ||
> +	       !READ_ONCE(pb->alive) || !READ_ONCE(pb->map_bits) ||
> +	       !list_empty_careful(&pb->pending);
> +}

[ ... ]

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

[Severity: Medium]
Is the lack of a .poll method in pb_fops intentional?

Without a ->poll handler, poll/select/epoll always report this file as
readable and writable. A tool that adds the fd to an epoll set would get an
immediate EPOLLIN. Its O_NONBLOCK read() would then return -EAGAIN, which
the UAPI header documents:

 *   -EAGAIN       - O_NONBLOCK is set and no writes are pending.

Polling again would return immediately, so the tool would spin. The only
usable way to wait is a blocking read() in a dedicated thread.

pb_has_event() already defines the readiness condition, and pb->waitq is
already woken on the relevant state changes. Would it make sense to add a
.poll method built on pb_has_event() and pb->waitq? Adding it later would
change how this fd behaves for userspace that already uses it.

Also, pb_fops has no .llseek = no_llseek, and the file is not opened with
stream_open() or nonseekable_open() semantics. Should this stream-like fd
be marked non-seekable?

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

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