diff mbox series

[v2,1/2] vfs: use READ_ONCE() to access ->i_link

Message ID 20190410202115.64501-2-ebiggers@kernel.org
State Not Applicable
Headers show
Series fscrypt: improve encrypted symlink performance | expand

Commit Message

Eric Biggers April 10, 2019, 8:21 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

Use 'READ_ONCE(inode->i_link)' to explicitly support filesystems caching
the symlink target in ->i_link later if it was unavailable at iget()
time, or wasn't easily available.  I'll be doing this in fscrypt, to
improve the performance of encrypted symlinks on ext4, f2fs, and ubifs.

->i_link will start NULL and may later be set to a non-NULL value by a
smp_store_release() or cmpxchg_release().  READ_ONCE() is needed on the
read side.  smp_load_acquire() is unnecessary because only a data
dependency barrier is required.  (Thanks to Al for pointing this out.)

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Al Viro April 10, 2019, 9:06 p.m. UTC | #1 
On Wed, Apr 10, 2019 at 01:21:14PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Use 'READ_ONCE(inode->i_link)' to explicitly support filesystems caching
> the symlink target in ->i_link later if it was unavailable at iget()
> time, or wasn't easily available.  I'll be doing this in fscrypt, to
> improve the performance of encrypted symlinks on ext4, f2fs, and ubifs.
> 
> ->i_link will start NULL and may later be set to a non-NULL value by a
> smp_store_release() or cmpxchg_release().  READ_ONCE() is needed on the
> read side.  smp_load_acquire() is unnecessary because only a data
> dependency barrier is required.  (Thanks to Al for pointing this out.)

You've missed vfs_readlink()...
Eric Biggers April 10, 2019, 11:15 p.m. UTC | #2 
On Wed, Apr 10, 2019 at 10:06:57PM +0100, Al Viro wrote:
> On Wed, Apr 10, 2019 at 01:21:14PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Use 'READ_ONCE(inode->i_link)' to explicitly support filesystems caching
> > the symlink target in ->i_link later if it was unavailable at iget()
> > time, or wasn't easily available.  I'll be doing this in fscrypt, to
> > improve the performance of encrypted symlinks on ext4, f2fs, and ubifs.
> > 
> > ->i_link will start NULL and may later be set to a non-NULL value by a
> > smp_store_release() or cmpxchg_release().  READ_ONCE() is needed on the
> > read side.  smp_load_acquire() is unnecessary because only a data
> > dependency barrier is required.  (Thanks to Al for pointing this out.)
> 
> You've missed vfs_readlink()...

How so?  The patch already updates vfs_readlink().

- Eric
Al Viro April 11, 2019, 2:25 a.m. UTC | #3 
On Wed, Apr 10, 2019 at 04:15:48PM -0700, Eric Biggers wrote:
> On Wed, Apr 10, 2019 at 10:06:57PM +0100, Al Viro wrote:
> > On Wed, Apr 10, 2019 at 01:21:14PM -0700, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Use 'READ_ONCE(inode->i_link)' to explicitly support filesystems caching
> > > the symlink target in ->i_link later if it was unavailable at iget()
> > > time, or wasn't easily available.  I'll be doing this in fscrypt, to
> > > improve the performance of encrypted symlinks on ext4, f2fs, and ubifs.
> > > 
> > > ->i_link will start NULL and may later be set to a non-NULL value by a
> > > smp_store_release() or cmpxchg_release().  READ_ONCE() is needed on the
> > > read side.  smp_load_acquire() is unnecessary because only a data
> > > dependency barrier is required.  (Thanks to Al for pointing this out.)
> > 
> > You've missed vfs_readlink()...
> 
> How so?  The patch already updates vfs_readlink().

Huh?  Right you are - fsck knows how have I managed to misread that...
Anyway, I can pick that READ_ONCE() patch through vfs.git if you wish,
or it can go through your tree with my Acked-by - up to you...
Eric Biggers April 11, 2019, 5:28 p.m. UTC | #4 
On Thu, Apr 11, 2019 at 03:25:15AM +0100, Al Viro wrote:
> On Wed, Apr 10, 2019 at 04:15:48PM -0700, Eric Biggers wrote:
> > On Wed, Apr 10, 2019 at 10:06:57PM +0100, Al Viro wrote:
> > > On Wed, Apr 10, 2019 at 01:21:14PM -0700, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > Use 'READ_ONCE(inode->i_link)' to explicitly support filesystems caching
> > > > the symlink target in ->i_link later if it was unavailable at iget()
> > > > time, or wasn't easily available.  I'll be doing this in fscrypt, to
> > > > improve the performance of encrypted symlinks on ext4, f2fs, and ubifs.
> > > > 
> > > > ->i_link will start NULL and may later be set to a non-NULL value by a
> > > > smp_store_release() or cmpxchg_release().  READ_ONCE() is needed on the
> > > > read side.  smp_load_acquire() is unnecessary because only a data
> > > > dependency barrier is required.  (Thanks to Al for pointing this out.)
> > > 
> > > You've missed vfs_readlink()...
> > 
> > How so?  The patch already updates vfs_readlink().
> 
> Huh?  Right you are - fsck knows how have I managed to misread that...
> Anyway, I can pick that READ_ONCE() patch through vfs.git if you wish,
> or it can go through your tree with my Acked-by - up to you...

Unless you expect merge conflicts, it's easier for me to take it through the
fscrypt tree, since it's a small patch and the second patch technically isn't
correct without it.  I'll add your Acked-by.

Are you planning to review the second patch too?

Thanks!

- Eric
Theodore Ts'o April 17, 2019, 4:41 p.m. UTC | #5 
On Thu, Apr 11, 2019 at 03:25:15AM +0100, Al Viro wrote:
> On Wed, Apr 10, 2019 at 04:15:48PM -0700, Eric Biggers wrote:
> > On Wed, Apr 10, 2019 at 10:06:57PM +0100, Al Viro wrote:
> > > On Wed, Apr 10, 2019 at 01:21:14PM -0700, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > Use 'READ_ONCE(inode->i_link)' to explicitly support filesystems caching
> > > > the symlink target in ->i_link later if it was unavailable at iget()
> > > > time, or wasn't easily available.  I'll be doing this in fscrypt, to
> > > > improve the performance of encrypted symlinks on ext4, f2fs, and ubifs.
> > > > 
> > > > ->i_link will start NULL and may later be set to a non-NULL value by a
> > > > smp_store_release() or cmpxchg_release().  READ_ONCE() is needed on the
> > > > read side.  smp_load_acquire() is unnecessary because only a data
> > > > dependency barrier is required.  (Thanks to Al for pointing this out.)
> > > 
> > > You've missed vfs_readlink()...
> > 
> > How so?  The patch already updates vfs_readlink().
> 
> Huh?  Right you are - fsck knows how have I managed to misread that...
> Anyway, I can pick that READ_ONCE() patch through vfs.git if you wish,
> or it can go through your tree with my Acked-by - up to you...

Al, I'll take it through the fscrypt tree with your Acked-by, if you
don't mind.

					- Ted
diff mbox series

Patch

diff --git a/fs/namei.c b/fs/namei.c
index dede0147b3f6e..2855de004c1a9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1066,7 +1066,7 @@  const char *get_link(struct nameidata *nd)
 		return ERR_PTR(error);
 
 	nd->last_type = LAST_BIND;
-	res = inode->i_link;
+	res = READ_ONCE(inode->i_link);
 	if (!res) {
 		const char * (*get)(struct dentry *, struct inode *,
 				struct delayed_call *);
@@ -4729,7 +4729,7 @@  int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 		spin_unlock(&inode->i_lock);
 	}
 
-	link = inode->i_link;
+	link = READ_ONCE(inode->i_link);
 	if (!link) {
 		link = inode->i_op->get_link(dentry, inode, &done);
 		if (IS_ERR(link))