Re: [PATCH] kernel: relay: remove relay_file_splice_read ‒ dead code, doesn't work

[Andrew Morton]

On Tue, 19 Dec 2023 23:24:14 +0100 Ahelenia Ziemiańska <nabijaczleweli@xxxxxxxxxxxxxxxxxx> wrote:

> Documentation/filesystems/relay.rst says to use
> 	return debugfs_create_file(filename, mode, parent, buf,
> 	                           &relay_file_operations);
> and this is the only way relay_file_operations is used.
> 
> Thus: debugfs_create_file(&relay_file_operations)
>    -> __debugfs_create_file(&debugfs_full_proxy_file_operations,
>                             &relay_file_operations)
>    -> dentry{inode: {i_fop: &debugfs_full_proxy_file_operations},
>              d_fsdata: &relay_file_operations
>                        | DEBUGFS_FSDATA_IS_REAL_FOPS_BIT}
> 
> debugfs_full_proxy_file_operations.open is full_proxy_open, which
> extracts the &relay_file_operations from the dentry, and allocates
> via __full_proxy_fops_init() new fops, with trivial wrappers around
> release, llseek, read, write, poll, and unlocked_ioctl, then replaces
> the fops on the opened file therewith.
> 
> Naturally, all thusly-created debugfs files have .splice_read = NULL.
> This was introduced in
> commit 49d200deaa680501f19a247b1fffb29301e51d2b ("debugfs: prevent
>  access to removed files' private data") from 2016-03-22.
> 
> AFAICT, relay_file_operations is the only struct file_operations
> used for debugfs which defines a .splice_read callback.
> Hooking it up with
> >	diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> >	index 5063434be0fc..952fcf5b2afa 100644
> >	--- a/fs/debugfs/file.c
> >	+++ b/fs/debugfs/file.c
> >	@@ -328,6 +328,11 @@ FULL_PROXY_FUNC(write, ssize_t, filp,
> >	 			loff_t *ppos),
> >	 		ARGS(filp, buf, size, ppos));
> >
> >	+FULL_PROXY_FUNC(splice_read, long, in,
> >	+		PROTO(struct file *in, loff_t *ppos, struct pipe_inode_info *pipe,
> >	+			size_t len, unsigned int flags),
> >	+		ARGS(in, ppos, pipe, len, flags));
> >	+
> >	 FULL_PROXY_FUNC(unlocked_ioctl, long, filp,
> >	 		PROTO(struct file *filp, unsigned int cmd, unsigned long arg),
> >	 		ARGS(filp, cmd, arg));
> >	@@ -382,6 +387,8 @@ static void __full_proxy_fops_init(struct file_operations *proxy_fops,
> >	 		proxy_fops->write = full_proxy_write;
> >	 	if (real_fops->poll)
> >	 		proxy_fops->poll = full_proxy_poll;
> >	+	if (real_fops->splice_read)
> >	+		proxy_fops->splice_read = full_proxy_splice_read;
> >	 	if (real_fops->unlocked_ioctl)
> >	 		proxy_fops->unlocked_ioctl = full_proxy_unlocked_ioctl;
> >	 }
> shows it just doesn't work, and splicing always instantly returns empty
> (subsequent reads actually return the contents).
> 
> No-one noticed it became dead code in 2016, who knows if it worked back
> then. Clearly no-one cares; just delete it.
> 

All checks out for me.  How on earth did you notice this?

> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -1073,167 +1073,6 @@ static ssize_t relay_file_read(struct file *filp,
>  	return written;
>  }
>  
> -static void relay_consume_bytes(struct rchan_buf *rbuf, int bytes_consumed)

And all this goop wasn't even inside #ifdef DEBUF_FS.