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

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

On 11/3/26 07:31, no-reply@sashiko.virtuozzo.com wrote:
> 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

We are not planning to use it in non-blocking mode, so this is fine. If 
we really need that we will add this later

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

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