LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] fs: optimise generic_write_check_limits()
@ 2021-08-06 11:22 Pavel Begunkov
  2021-08-06 13:28 ` Matthew Wilcox
  2021-08-06 13:46 ` Al Viro
  0 siblings, 2 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-08-06 11:22 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel, linux-kernel

Even though ->s_maxbytes is used by generic_write_check_limits() only in
case of O_LARGEFILE, the value is loaded unconditionally, which is heavy
and takes 4 indirect loads. Optimise it by not touching ->s_maxbytes,
if it's not going to be used.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/read_write.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 9db7adf160d2..db662d0c3cfa 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1609,9 +1609,8 @@ SYSCALL_DEFINE6(copy_file_range, int, fd_in, loff_t __user *, off_in,
  */
 int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)
 {
-	struct inode *inode = file->f_mapping->host;
-	loff_t max_size = inode->i_sb->s_maxbytes;
 	loff_t limit = rlimit(RLIMIT_FSIZE);
+	loff_t max_size = MAX_NON_LFS;
 
 	if (limit != RLIM_INFINITY) {
 		if (pos >= limit) {
@@ -1621,8 +1620,11 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)
 		*count = min(*count, limit - pos);
 	}
 
-	if (!(file->f_flags & O_LARGEFILE))
-		max_size = MAX_NON_LFS;
+	if (file->f_flags & O_LARGEFILE) {
+		struct inode *inode = file->f_mapping->host;
+
+		max_size = inode->i_sb->s_maxbytes;
+	}
 
 	if (unlikely(pos >= max_size))
 		return -EFBIG;
-- 
2.32.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs: optimise generic_write_check_limits()
  2021-08-06 11:22 [PATCH] fs: optimise generic_write_check_limits() Pavel Begunkov
@ 2021-08-06 13:28 ` Matthew Wilcox
  2021-08-07 10:05   ` Pavel Begunkov
  2021-08-08 14:41   ` David Laight
  2021-08-06 13:46 ` Al Viro
  1 sibling, 2 replies; 6+ messages in thread
From: Matthew Wilcox @ 2021-08-06 13:28 UTC (permalink / raw)
  To: Pavel Begunkov; +Cc: Alexander Viro, linux-fsdevel, linux-kernel

On Fri, Aug 06, 2021 at 12:22:10PM +0100, Pavel Begunkov wrote:
> Even though ->s_maxbytes is used by generic_write_check_limits() only in
> case of O_LARGEFILE, the value is loaded unconditionally, which is heavy
> and takes 4 indirect loads. Optimise it by not touching ->s_maxbytes,
> if it's not going to be used.

Is this "optimisation" actually worth anything?  Look at how
force_o_largefile() is used.  I would suggest that on the vast majority
of machines, O_LARGEFILE is always set.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs: optimise generic_write_check_limits()
  2021-08-06 11:22 [PATCH] fs: optimise generic_write_check_limits() Pavel Begunkov
  2021-08-06 13:28 ` Matthew Wilcox
@ 2021-08-06 13:46 ` Al Viro
  1 sibling, 0 replies; 6+ messages in thread
From: Al Viro @ 2021-08-06 13:46 UTC (permalink / raw)
  To: Pavel Begunkov; +Cc: linux-fsdevel, linux-kernel

On Fri, Aug 06, 2021 at 12:22:10PM +0100, Pavel Begunkov wrote:
> Even though ->s_maxbytes is used by generic_write_check_limits() only in
> case of O_LARGEFILE, the value is loaded unconditionally, which is heavy
> and takes 4 indirect loads. Optimise it by not touching ->s_maxbytes,
> if it's not going to be used.

Out of curiosity - how much of improvement have you actually seen on that?
I can't say I hate the patch, but I'd like to see the data...

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs: optimise generic_write_check_limits()
  2021-08-06 13:28 ` Matthew Wilcox
@ 2021-08-07 10:05   ` Pavel Begunkov
  2021-08-08 14:41   ` David Laight
  1 sibling, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-08-07 10:05 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Alexander Viro, linux-fsdevel, linux-kernel

On 8/6/21 2:28 PM, Matthew Wilcox wrote:
> On Fri, Aug 06, 2021 at 12:22:10PM +0100, Pavel Begunkov wrote:
>> Even though ->s_maxbytes is used by generic_write_check_limits() only in
>> case of O_LARGEFILE, the value is loaded unconditionally, which is heavy
>> and takes 4 indirect loads. Optimise it by not touching ->s_maxbytes,
>> if it's not going to be used.
> 
> Is this "optimisation" actually worth anything?  Look at how
> force_o_largefile() is used.  I would suggest that on the vast majority
> of machines, O_LARGEFILE is always set.

Makes sense to leave it alone then, thanks

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] fs: optimise generic_write_check_limits()
  2021-08-06 13:28 ` Matthew Wilcox
  2021-08-07 10:05   ` Pavel Begunkov
@ 2021-08-08 14:41   ` David Laight
  2021-08-08 15:34     ` Matthew Wilcox
  1 sibling, 1 reply; 6+ messages in thread
From: David Laight @ 2021-08-08 14:41 UTC (permalink / raw)
  To: 'Matthew Wilcox', Pavel Begunkov
  Cc: Alexander Viro, linux-fsdevel, linux-kernel

From: Matthew Wilcox
> Sent: 06 August 2021 14:28
> 
> On Fri, Aug 06, 2021 at 12:22:10PM +0100, Pavel Begunkov wrote:
> > Even though ->s_maxbytes is used by generic_write_check_limits() only in
> > case of O_LARGEFILE, the value is loaded unconditionally, which is heavy
> > and takes 4 indirect loads. Optimise it by not touching ->s_maxbytes,
> > if it's not going to be used.
> 
> Is this "optimisation" actually worth anything?  Look at how
> force_o_largefile() is used.  I would suggest that on the vast majority
> of machines, O_LARGEFILE is always set.

An option would be to only determine ->s_maxbytes when the size
if larger than MAX_NON_LFS.

So you'd end up with something like:

	if (pos >= max_size) {
		if (!(file->f_flags & O_LARGEFILE))
			return -EFBIG;
		inode = file->f_mapping->host;
		if (pos >= inode->i_sb->s_maxbytes)
			return -EFBIG;
	}

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs: optimise generic_write_check_limits()
  2021-08-08 14:41   ` David Laight
@ 2021-08-08 15:34     ` Matthew Wilcox
  0 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2021-08-08 15:34 UTC (permalink / raw)
  To: David Laight; +Cc: Pavel Begunkov, Alexander Viro, linux-fsdevel, linux-kernel

On Sun, Aug 08, 2021 at 02:41:13PM +0000, David Laight wrote:
> From: Matthew Wilcox
> > Sent: 06 August 2021 14:28
> > 
> > On Fri, Aug 06, 2021 at 12:22:10PM +0100, Pavel Begunkov wrote:
> > > Even though ->s_maxbytes is used by generic_write_check_limits() only in
> > > case of O_LARGEFILE, the value is loaded unconditionally, which is heavy
> > > and takes 4 indirect loads. Optimise it by not touching ->s_maxbytes,
> > > if it's not going to be used.
> > 
> > Is this "optimisation" actually worth anything?  Look at how
> > force_o_largefile() is used.  I would suggest that on the vast majority
> > of machines, O_LARGEFILE is always set.
> 
> An option would be to only determine ->s_maxbytes when the size
> if larger than MAX_NON_LFS.
> 
> So you'd end up with something like:
> 
> 	if (pos >= max_size) {
> 		if (!(file->f_flags & O_LARGEFILE))
> 			return -EFBIG;
> 		inode = file->f_mapping->host;
> 		if (pos >= inode->i_sb->s_maxbytes)
> 			return -EFBIG;
> 	}

You're optimising the part of the function that you can see in the
diff instead of the whole function.  And there's no evidence that
there's much win to be had here ...

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-08 15:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-06 11:22 [PATCH] fs: optimise generic_write_check_limits() Pavel Begunkov
2021-08-06 13:28 ` Matthew Wilcox
2021-08-07 10:05   ` Pavel Begunkov
2021-08-08 14:41   ` David Laight
2021-08-08 15:34     ` Matthew Wilcox
2021-08-06 13:46 ` Al Viro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).