On Wed, Apr 10, 2019 at 02:39:34AM +0100, Al Viro wrote: > On Tue, Apr 09, 2019 at 06:22:49PM -0700, Eric Biggers wrote: > > > > Non-NULL ->get_link() => DCACHE_SYMLINK_TYPE in ->d_flags => > > > d_is_symlink() true => step_into() progresses to pick_link(). > > > > > > IOW, non-NULL ->get_link() is what tells you that we have > > > a symlink there. > > > > I think that's pretty unintuitive. The fact that multiple filesystems including > > ext4 set ->i_link on fast symlinks, then set ->get_link() to a function that > > returns ->i_link, made me assume that's the mechanism by which such symlink > > targets are returned to the VFS. When in fact fs/namei.c just uses ->i_link, > > and never calls ->get_link(). > > > > Is there any reason why d_flags_for_inode() doesn't check S_ISLNK() instead, and > > then fs/namei.c would call ->get_link() if non-NULL, otherwise use ->i_link? > > Extra check and dereference on hot path with no visible benefits of doing it > that way, for starters. Really, what _is_ the benefit of pessimizing that? > Most of the symlinks we run into will have ->i_link set; checking ->i_op->get_link > first is extra work for no good reason... > > What's more, ->get_link is visible in inode_operations; ->i_link (let alone ->i_mode) > isn't. As it is, we can easily tell symlink inode_operations from everything else > on the source level. With your scheme we won't. It could check a flag IOP_GET_LINK in ->i_opflags instead, so it would be the same number of checks. See patch below. Benefits are that we get code that isn't actively misleading (via simple_get_link() existing but actually never being called), and filesystems can cache a symlink target in ->i_link if it becomes available later, i.e. if it's not immediately available at iget() time. Otherwise a filesystem-private field has to be used instead. (For fscrypt, I'd probably use fscrypt_info::ci_link.) Anyway, if we're going to stick with the current approach we should at least add a comment to simple_get_link() explaining what it's really for... diff --git a/fs/dcache.c b/fs/dcache.c index aac41adf47433..df0e2f092a481 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1820,12 +1820,11 @@ static unsigned d_flags_for_inode(struct inode *inode) goto type_determined; } - if (unlikely(!(inode->i_opflags & IOP_NOFOLLOW))) { - if (unlikely(inode->i_op->get_link)) { - add_flags = DCACHE_SYMLINK_TYPE; - goto type_determined; - } - inode->i_opflags |= IOP_NOFOLLOW; + if (unlikely(S_ISLNK(inode->i_mode))) { + add_flags = DCACHE_SYMLINK_TYPE; + if (inode->i_op->get_link) + inode->i_opflags |= IOP_GET_LINK; + goto type_determined; } if (unlikely(!S_ISREG(inode->i_mode))) diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c index dd05af983092d..315e4622db3d2 100644 --- a/fs/ext4/symlink.c +++ b/fs/ext4/symlink.c @@ -67,7 +67,6 @@ const struct inode_operations ext4_symlink_inode_operations = { }; const struct inode_operations ext4_fast_symlink_inode_operations = { - .get_link = simple_get_link, .setattr = ext4_setattr, .getattr = ext4_getattr, .listxattr = ext4_listxattr, diff --git a/fs/namei.c b/fs/namei.c index dede0147b3f6e..d99275f0cd3d7 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1067,7 +1067,7 @@ const char *get_link(struct nameidata *nd) nd->last_type = LAST_BIND; res = inode->i_link; - if (!res) { + if (inode->i_opflags & IOP_GET_LINK) { const char * (*get)(struct dentry *, struct inode *, struct delayed_call *); get = inode->i_op->get_link; @@ -4730,7 +4730,7 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) } link = inode->i_link; - if (!link) { + if (inode->i_opflags & IOP_GET_LINK) { link = inode->i_op->get_link(dentry, inode, &done); if (IS_ERR(link)) return PTR_ERR(link); diff --git a/include/linux/fs.h b/include/linux/fs.h index dd28e76790891..f6353aa40355b 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -604,7 +604,7 @@ is_uncached_acl(struct posix_acl *acl) #define IOP_FASTPERM 0x0001 #define IOP_LOOKUP 0x0002 -#define IOP_NOFOLLOW 0x0004 +#define IOP_GET_LINK 0x0004 #define IOP_XATTR 0x0008 #define IOP_DEFAULT_READLINK 0x0010 - Eric
![]() |