Re: [PATCH 00/17] VFS: Filesystem information and notifications [ver #17]

From: Ian Kent <raven@themaw.net>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Karel Zak <kzak@redhat.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	David Howells <dhowells@redhat.com>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Steven Whitehouse <swhiteho@redhat.com>,
	Miklos Szeredi <mszeredi@redhat.com>,
	viro <viro@zeniv.linux.org.uk>,
	Christian Brauner <christian@brauner.io>,
	Jann Horn <jannh@google.com>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Linux API <linux-api@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 00/17] VFS: Filesystem information and notifications [ver #17]
Date: Wed, 04 Mar 2020 10:01:33 +0800	[thread overview]
Message-ID: <33d900c8061c40f70ba2b9d1855fd6bd1c2b68bb.camel@themaw.net> (raw)
In-Reply-To: <20200303130347.GA2302029@kroah.com>

On Tue, 2020-03-03 at 14:03 +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 03, 2020 at 12:38:14PM +0100, Karel Zak wrote:
> > On Tue, Mar 03, 2020 at 10:26:21AM +0100, Miklos Szeredi wrote:
> > > No, I don't think this is going to be a performance issue at all,
> > > but
> > > if anything we could introduce a syscall
> > > 
> > >   ssize_t readfile(int dfd, const char *path, char *buf, size_t
> > > bufsize, int flags);
> > 
> > off-topic, but I'll buy you many many beers if you implement it ;-
> > ),
> > because open + read + close is pretty common for /sys and /proc in
> > many userspace tools; for example ps, top, lsblk, lsmem, lsns,
> > udevd
> > etc. is all about it.
> 
> Unlimited beers for a 21-line kernel patch?  Sign me up!
> 
> Totally untested, barely compiled patch below.
> 
> Actually, I like this idea (the syscall, not just the unlimited
> beers).
> Maybe this could make a lot of sense, I'll write some actual tests
> for
> it now that syscalls are getting "heavy" again due to CPU vendors
> finally paying the price for their madness...

The problem isn't with open->read->close but with the mount info.
changing between reads (ie. seq file read takes and drops the
needed lock between reads at least once).

The problem is you don't know the buffer size needed to get this
in one hit, how is this different to read(2)?

> 
> thanks,
> 
> greg k-h
> -------------------
> 
> 
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
> b/arch/x86/entry/syscalls/syscall_64.tbl
> index 44d510bc9b78..178cd45340e2 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -359,6 +359,7 @@
>  435	common	clone3			__x64_sys_clone3/ptregs
>  437	common	openat2			__x64_sys_openat2
>  438	common	pidfd_getfd		__x64_sys_pidfd_getfd
> +439	common	readfile		__x86_sys_readfile
>  
>  #
>  # x32-specific system call numbers start at 512 to avoid cache
> impact
> diff --git a/fs/open.c b/fs/open.c
> index 0788b3715731..1a830fada750 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1340,3 +1340,23 @@ int stream_open(struct inode *inode, struct
> file *filp)
>  }
>  
>  EXPORT_SYMBOL(stream_open);
> +
> +SYSCALL_DEFINE5(readfile, int, dfd, const char __user *, filename,
> +		char __user *, buffer, size_t, bufsize, int, flags)
> +{
> +	int retval;
> +	int fd;
> +
> +	if (force_o_largefile())
> +		flags |= O_LARGEFILE;
> +
> +	fd = do_sys_open(dfd, filename, flags, O_RDONLY);
> +	if (fd <= 0)
> +		return fd;
> +
> +	retval = ksys_read(fd, buffer, bufsize);
> +
> +	__close_fd(current->files, fd);
> +
> +	return retval;
> +}