* [RFC][PATCHES] reducing d_add() use, part 1 @ 2018-05-13 21:26 Al Viro 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro 2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro 0 siblings, 2 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:26 UTC (permalink / raw) To: linux-fsdevel A lot of d_add() uses are in ->lookup() instances; those really should be d_splice_alias() - that's mandatory for anything exported (and we'd seen people not bothering to convert when adding exports/open-by-fhandle support) *and* it's not costlier than d_add() anyway, in all cases when d_add() wouldn't have caused instant breakage. What's more, d_splice_alias() makes for simpler life in callers - it does the right thing when given ERR_PTR(), which simplifies the logics in quite a few ->lookup() instances. There are trickier cases, but a bunch of those call sites are completely straightforward. See followups, or vfs.git#work.lookup ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 01/15] bfs_lookup(): use d_splice_alias() 2018-05-13 21:26 [RFC][PATCHES] reducing d_add() use, part 1 Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer Al Viro ` (14 more replies) 2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro 1 sibling, 15 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Tigran A. Aivazian From: Al Viro <viro@zeniv.linux.org.uk> code is actually simpler that way. Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/bfs/dir.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c index ee832ca5f734..facf9614a381 100644 --- a/fs/bfs/dir.c +++ b/fs/bfs/dir.c @@ -141,14 +141,9 @@ static struct dentry *bfs_lookup(struct inode *dir, struct dentry *dentry, unsigned long ino = (unsigned long)le16_to_cpu(de->ino); brelse(bh); inode = bfs_iget(dir->i_sb, ino); - if (IS_ERR(inode)) { - mutex_unlock(&info->bfs_lock); - return ERR_CAST(inode); - } } mutex_unlock(&info->bfs_lock); - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } static int bfs_link(struct dentry *old, struct inode *dir, -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 03/15] bfs_add_entry: " Al Viro ` (13 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Tigran A. Aivazian From: Al Viro <viro@zeniv.linux.org.uk> all callers feed something->name/something->len anyway Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/bfs/dir.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c index facf9614a381..528c69746f7d 100644 --- a/fs/bfs/dir.c +++ b/fs/bfs/dir.c @@ -24,7 +24,7 @@ static int bfs_add_entry(struct inode *dir, const unsigned char *name, int namelen, int ino); static struct buffer_head *bfs_find_entry(struct inode *dir, - const unsigned char *name, int namelen, + const struct qstr *child, struct bfs_dirent **res_dir); static int bfs_readdir(struct file *f, struct dir_context *ctx) @@ -136,7 +136,7 @@ static struct dentry *bfs_lookup(struct inode *dir, struct dentry *dentry, return ERR_PTR(-ENAMETOOLONG); mutex_lock(&info->bfs_lock); - bh = bfs_find_entry(dir, dentry->d_name.name, dentry->d_name.len, &de); + bh = bfs_find_entry(dir, &dentry->d_name, &de); if (bh) { unsigned long ino = (unsigned long)le16_to_cpu(de->ino); brelse(bh); @@ -178,7 +178,7 @@ static int bfs_unlink(struct inode *dir, struct dentry *dentry) struct bfs_sb_info *info = BFS_SB(inode->i_sb); mutex_lock(&info->bfs_lock); - bh = bfs_find_entry(dir, dentry->d_name.name, dentry->d_name.len, &de); + bh = bfs_find_entry(dir, &dentry->d_name, &de); if (!bh || (le16_to_cpu(de->ino) != inode->i_ino)) goto out_brelse; @@ -223,18 +223,14 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry, info = BFS_SB(old_inode->i_sb); mutex_lock(&info->bfs_lock); - old_bh = bfs_find_entry(old_dir, - old_dentry->d_name.name, - old_dentry->d_name.len, &old_de); + old_bh = bfs_find_entry(old_dir, &old_dentry->d_name, &old_de); if (!old_bh || (le16_to_cpu(old_de->ino) != old_inode->i_ino)) goto end_rename; error = -EPERM; new_inode = d_inode(new_dentry); - new_bh = bfs_find_entry(new_dir, - new_dentry->d_name.name, - new_dentry->d_name.len, &new_de); + new_bh = bfs_find_entry(new_dir, &new_dentry->d_name, &new_de); if (new_bh && !new_inode) { brelse(new_bh); @@ -327,12 +323,14 @@ static inline int bfs_namecmp(int len, const unsigned char *name, } static struct buffer_head *bfs_find_entry(struct inode *dir, - const unsigned char *name, int namelen, + const struct qstr *child, struct bfs_dirent **res_dir) { unsigned long block = 0, offset = 0; struct buffer_head *bh = NULL; struct bfs_dirent *de; + const unsigned char *name = child->name; + int namelen = child->len; *res_dir = NULL; if (namelen > BFS_NAMELEN) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 03/15] bfs_add_entry: pass name/len as qstr pointer 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro 2018-05-13 21:30 ` [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 04/15] cramfs_lookup(): use d_splice_alias() Al Viro ` (12 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Tigran A. Aivazian From: Al Viro <viro@zeniv.linux.org.uk> same story as with bfs_find_entry() Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/bfs/dir.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c index 528c69746f7d..f32f21c3bbc7 100644 --- a/fs/bfs/dir.c +++ b/fs/bfs/dir.c @@ -21,8 +21,7 @@ #define dprintf(x...) #endif -static int bfs_add_entry(struct inode *dir, const unsigned char *name, - int namelen, int ino); +static int bfs_add_entry(struct inode *dir, const struct qstr *child, int ino); static struct buffer_head *bfs_find_entry(struct inode *dir, const struct qstr *child, struct bfs_dirent **res_dir); @@ -111,8 +110,7 @@ static int bfs_create(struct inode *dir, struct dentry *dentry, umode_t mode, mark_inode_dirty(inode); bfs_dump_imap("create", s); - err = bfs_add_entry(dir, dentry->d_name.name, dentry->d_name.len, - inode->i_ino); + err = bfs_add_entry(dir, &dentry->d_name, inode->i_ino); if (err) { inode_dec_link_count(inode); mutex_unlock(&info->bfs_lock); @@ -154,8 +152,7 @@ static int bfs_link(struct dentry *old, struct inode *dir, int err; mutex_lock(&info->bfs_lock); - err = bfs_add_entry(dir, new->d_name.name, new->d_name.len, - inode->i_ino); + err = bfs_add_entry(dir, &new->d_name, inode->i_ino); if (err) { mutex_unlock(&info->bfs_lock); return err; @@ -237,9 +234,7 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry, new_bh = NULL; } if (!new_bh) { - error = bfs_add_entry(new_dir, - new_dentry->d_name.name, - new_dentry->d_name.len, + error = bfs_add_entry(new_dir, &new_dentry->d_name, old_inode->i_ino); if (error) goto end_rename; @@ -269,9 +264,10 @@ const struct inode_operations bfs_dir_inops = { .rename = bfs_rename, }; -static int bfs_add_entry(struct inode *dir, const unsigned char *name, - int namelen, int ino) +static int bfs_add_entry(struct inode *dir, const struct qstr *child, int ino) { + const unsigned char *name = child->name; + int namelen = child->len; struct buffer_head *bh; struct bfs_dirent *de; int block, sblock, eblock, off, pos; -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 04/15] cramfs_lookup(): use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro 2018-05-13 21:30 ` [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer Al Viro 2018-05-13 21:30 ` [PATCH 03/15] bfs_add_entry: " Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 05/15] freevxfs_lookup(): " Al Viro ` (11 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Nicolas Pitre From: Al Viro <viro@zeniv.linux.org.uk> simpler code that way, actually Cc: Nicolas Pitre <nico@linaro.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/cramfs/inode.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c index 017b0ab19bc4..4756e9daa0b2 100644 --- a/fs/cramfs/inode.c +++ b/fs/cramfs/inode.c @@ -808,10 +808,7 @@ static struct dentry *cramfs_lookup(struct inode *dir, struct dentry *dentry, un } out: mutex_unlock(&read_mutex); - if (IS_ERR(inode)) - return ERR_CAST(inode); - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } static int cramfs_readpage(struct file *file, struct page *page) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 05/15] freevxfs_lookup(): use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (2 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 04/15] cramfs_lookup(): use d_splice_alias() Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-14 8:41 ` Christoph Hellwig 2018-05-13 21:30 ` [PATCH 06/15] minix_lookup: " Al Viro ` (10 subsequent siblings) 14 siblings, 1 reply; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Christoph Hellwig From: Al Viro <viro@zeniv.linux.org.uk> Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/freevxfs/vxfs_lookup.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/fs/freevxfs/vxfs_lookup.c b/fs/freevxfs/vxfs_lookup.c index ce4785fd81c6..a51425634f65 100644 --- a/fs/freevxfs/vxfs_lookup.c +++ b/fs/freevxfs/vxfs_lookup.c @@ -193,13 +193,9 @@ vxfs_lookup(struct inode *dip, struct dentry *dp, unsigned int flags) return ERR_PTR(-ENAMETOOLONG); ino = vxfs_inode_by_name(dip, dp); - if (ino) { + if (ino) ip = vxfs_iget(dip->i_sb, ino); - if (IS_ERR(ip)) - return ERR_CAST(ip); - } - d_add(dp, ip); - return NULL; + return d_splice_alias(ip, dp); } /** -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 05/15] freevxfs_lookup(): use d_splice_alias() 2018-05-13 21:30 ` [PATCH 05/15] freevxfs_lookup(): " Al Viro @ 2018-05-14 8:41 ` Christoph Hellwig 2018-05-14 15:26 ` Al Viro 0 siblings, 1 reply; 46+ messages in thread From: Christoph Hellwig @ 2018-05-14 8:41 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, Christoph Hellwig On Sun, May 13, 2018 at 10:30:07PM +0100, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> It looks a little cleaner, so this is fine, but is there also any other reason? A little changelog would be nice. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 05/15] freevxfs_lookup(): use d_splice_alias() 2018-05-14 8:41 ` Christoph Hellwig @ 2018-05-14 15:26 ` Al Viro 0 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-14 15:26 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-fsdevel On Mon, May 14, 2018 at 10:41:02AM +0200, Christoph Hellwig wrote: > On Sun, May 13, 2018 at 10:30:07PM +0100, Al Viro wrote: > > From: Al Viro <viro@zeniv.linux.org.uk> > > > > Cc: Christoph Hellwig <hch@lst.de> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > It looks a little cleaner, so this is fine, but is there also any > other reason? A little changelog would be nice. The same changelog would be duplicated in a bunch of commits in that series and I'm not sure how much is too much. Let me put it that way: * d_splice_alias() has better calling conventions for use in ->lookup() - it tends to reduce the number of branches nicely. * d_add() in ->lookup() is no-go for anything exportable; we did conversion for exported stuff back then, and it's documented in Documentation/filesystems/nfs/Exporting, but people fail to RTFM when converting more filesystems to exportability. Preempting that kind of bugs is a good idea. * the fewer stray d_add() we have sitting around, the better - each needs a proof that this particular invocation won't end up with multiple aliases of a directory inode. d_splice_alias() is *NOT* a universal replacement, but in a lot of ->lookup() instances we can use it sanely; it can't be done quite blindly (the stuff that hangs something off the dentry needs a careful look), but most of the time it's fine - on any filesystem that doesn't play with ->d_fsdata, for starters. And in all cases such replacement in ->lookup() is "it's no worse than before" - it's just that some cases need more than just that conversion before they can be made exportable (see e.g. 9p for example of that kind of extra work; res = d_splice_alias(inode, dentry); if (!res) v9fs_fid_add(dentry, fid); else if (!IS_ERR(res)) v9fs_fid_add(res, fid); else p9_client_clunk(fid); in there, when we need to decide which dentry to slap the fid onto) The bunch in this series is the trivial part of conversions. Next group is where a bit more attention is needed and the last is procfs nest of horrors. I'm honestly not sure how much of the above makes sense as a boilerplate to go into all those commits. Suggestions? ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 06/15] minix_lookup: use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (3 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 05/15] freevxfs_lookup(): " Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 07/15] qnx4_lookup: " Al Viro ` (9 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/minix/namei.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/fs/minix/namei.c b/fs/minix/namei.c index ccf0f00030bf..1a6084d2b02e 100644 --- a/fs/minix/namei.c +++ b/fs/minix/namei.c @@ -28,13 +28,9 @@ static struct dentry *minix_lookup(struct inode * dir, struct dentry *dentry, un return ERR_PTR(-ENAMETOOLONG); ino = minix_inode_by_name(dentry); - if (ino) { + if (ino) inode = minix_iget(dir->i_sb, ino); - if (IS_ERR(inode)) - return ERR_CAST(inode); - } - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } static int minix_mknod(struct inode * dir, struct dentry *dentry, umode_t mode, dev_t rdev) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 07/15] qnx4_lookup: use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (4 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 06/15] minix_lookup: " Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-14 10:48 ` Anders Larsen 2018-05-13 21:30 ` [PATCH 08/15] sysv_lookup: " Al Viro ` (8 subsequent siblings) 14 siblings, 1 reply; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Anders Larsen From: Al Viro <viro@zeniv.linux.org.uk> Cc: Anders Larsen <al@alarsen.net> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/qnx4/namei.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c index eca27878079d..8d72221735d7 100644 --- a/fs/qnx4/namei.c +++ b/fs/qnx4/namei.c @@ -114,13 +114,9 @@ struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, unsigned i brelse(bh); foundinode = qnx4_iget(dir->i_sb, ino); - if (IS_ERR(foundinode)) { + if (IS_ERR(foundinode)) QNX4DEBUG((KERN_ERR "qnx4: lookup->iget -> error %ld\n", PTR_ERR(foundinode))); - return ERR_CAST(foundinode); - } out: - d_add(dentry, foundinode); - - return NULL; + return d_splice_alias(foundinode, dentry); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 07/15] qnx4_lookup: use d_splice_alias() 2018-05-13 21:30 ` [PATCH 07/15] qnx4_lookup: " Al Viro @ 2018-05-14 10:48 ` Anders Larsen 0 siblings, 0 replies; 46+ messages in thread From: Anders Larsen @ 2018-05-14 10:48 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel On Sunday, 13 May 2018 22:30 Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Cc: Anders Larsen <al@alarsen.net> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/qnx4/namei.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) Acked-by: Anders Larsen <al@alarsen.net> > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > index eca27878079d..8d72221735d7 100644 > --- a/fs/qnx4/namei.c > +++ b/fs/qnx4/namei.c > @@ -114,13 +114,9 @@ struct dentry * qnx4_lookup(struct inode *dir, struct > dentry *dentry, unsigned i brelse(bh); > > foundinode = qnx4_iget(dir->i_sb, ino); > - if (IS_ERR(foundinode)) { > + if (IS_ERR(foundinode)) > QNX4DEBUG((KERN_ERR "qnx4: lookup->iget -> error %ld\n", > PTR_ERR(foundinode))); > - return ERR_CAST(foundinode); > - } > out: > - d_add(dentry, foundinode); > - > - return NULL; > + return d_splice_alias(foundinode, dentry); > } ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 08/15] sysv_lookup: use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (5 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 07/15] qnx4_lookup: " Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-14 8:41 ` Christoph Hellwig 2018-05-13 21:30 ` [PATCH 09/15] ubifs_lookup: " Al Viro ` (7 subsequent siblings) 14 siblings, 1 reply; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Christoph Hellwig From: Al Viro <viro@zeniv.linux.org.uk> Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/sysv/namei.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/fs/sysv/namei.c b/fs/sysv/namei.c index 250b0755b908..4d5d20491ffd 100644 --- a/fs/sysv/namei.c +++ b/fs/sysv/namei.c @@ -51,14 +51,9 @@ static struct dentry *sysv_lookup(struct inode * dir, struct dentry * dentry, un if (dentry->d_name.len > SYSV_NAMELEN) return ERR_PTR(-ENAMETOOLONG); ino = sysv_inode_by_name(dentry); - - if (ino) { + if (ino) inode = sysv_iget(dir->i_sb, ino); - if (IS_ERR(inode)) - return ERR_CAST(inode); - } - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } static int sysv_mknod(struct inode * dir, struct dentry * dentry, umode_t mode, dev_t rdev) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 08/15] sysv_lookup: use d_splice_alias() 2018-05-13 21:30 ` [PATCH 08/15] sysv_lookup: " Al Viro @ 2018-05-14 8:41 ` Christoph Hellwig 0 siblings, 0 replies; 46+ messages in thread From: Christoph Hellwig @ 2018-05-14 8:41 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, Christoph Hellwig Same here, looks sane, but a little changelog would be nice. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 09/15] ubifs_lookup: use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (6 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 08/15] sysv_lookup: " Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-14 6:48 ` Richard Weinberger 2018-05-13 21:30 ` [PATCH 10/15] qnx6_lookup: switch to d_splice_alias() Al Viro ` (6 subsequent siblings) 14 siblings, 1 reply; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Richard Weinberger From: Al Viro <viro@zeniv.linux.org.uk> Cc: Richard Weinberger <richard@nod.at> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/ubifs/dir.c | 43 +++++++++++++++---------------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c index 9d7fb88e172e..4e267cc21c77 100644 --- a/fs/ubifs/dir.c +++ b/fs/ubifs/dir.c @@ -214,7 +214,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, int err; union ubifs_key key; struct inode *inode = NULL; - struct ubifs_dent_node *dent; + struct ubifs_dent_node *dent = NULL; struct ubifs_info *c = dir->i_sb->s_fs_info; struct fscrypt_name nm; @@ -229,14 +229,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, return ERR_PTR(err); if (fname_len(&nm) > UBIFS_MAX_NLEN) { - err = -ENAMETOOLONG; - goto out_fname; + inode = ERR_PTR(-ENAMETOOLONG); + goto done; } dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS); if (!dent) { - err = -ENOMEM; - goto out_fname; + inode = ERR_PTR(-ENOMEM); + goto done; } if (nm.hash) { @@ -250,16 +250,16 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, } if (err) { - if (err == -ENOENT) { + if (err == -ENOENT) dbg_gen("not found"); - goto done; - } - goto out_dent; + else + inode = ERR_PTR(err); + goto done; } if (dbg_check_name(c, dent, &nm)) { - err = -EINVAL; - goto out_dent; + inode = ERR_PTR(-EINVAL); + goto done; } inode = ubifs_iget(dir->i_sb, le64_to_cpu(dent->inum)); @@ -272,7 +272,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, ubifs_err(c, "dead directory entry '%pd', error %d", dentry, err); ubifs_ro_mode(c, err); - goto out_dent; + goto done; } if (ubifs_crypt_is_encrypted(dir) && @@ -280,27 +280,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, !fscrypt_has_permitted_context(dir, inode)) { ubifs_warn(c, "Inconsistent encryption contexts: %lu/%lu", dir->i_ino, inode->i_ino); - err = -EPERM; - goto out_inode; + iput(inode); + inode = ERR_PTR(-EPERM); } done: kfree(dent); fscrypt_free_filename(&nm); - /* - * Note, d_splice_alias() would be required instead if we supported - * NFS. - */ - d_add(dentry, inode); - return NULL; - -out_inode: - iput(inode); -out_dent: - kfree(dent); -out_fname: - fscrypt_free_filename(&nm); - return ERR_PTR(err); + return d_splice_alias(inode, dentry); } static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode, -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 09/15] ubifs_lookup: use d_splice_alias() 2018-05-13 21:30 ` [PATCH 09/15] ubifs_lookup: " Al Viro @ 2018-05-14 6:48 ` Richard Weinberger 0 siblings, 0 replies; 46+ messages in thread From: Richard Weinberger @ 2018-05-14 6:48 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, Linux MTD Am Sonntag, 13. Mai 2018, 23:30:11 CEST schrieb Al Viro: > From: Al Viro <viro@zeniv.linux.org.uk> > > Cc: Richard Weinberger <richard@nod.at> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/ubifs/dir.c | 43 +++++++++++++++---------------------------- > 1 file changed, 15 insertions(+), 28 deletions(-) > > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c > index 9d7fb88e172e..4e267cc21c77 100644 > --- a/fs/ubifs/dir.c > +++ b/fs/ubifs/dir.c > @@ -214,7 +214,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > int err; > union ubifs_key key; > struct inode *inode = NULL; > - struct ubifs_dent_node *dent; > + struct ubifs_dent_node *dent = NULL; > struct ubifs_info *c = dir->i_sb->s_fs_info; > struct fscrypt_name nm; > > @@ -229,14 +229,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > return ERR_PTR(err); > > if (fname_len(&nm) > UBIFS_MAX_NLEN) { > - err = -ENAMETOOLONG; > - goto out_fname; > + inode = ERR_PTR(-ENAMETOOLONG); > + goto done; > } > > dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS); > if (!dent) { > - err = -ENOMEM; > - goto out_fname; > + inode = ERR_PTR(-ENOMEM); > + goto done; > } > > if (nm.hash) { > @@ -250,16 +250,16 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > } > > if (err) { > - if (err == -ENOENT) { > + if (err == -ENOENT) > dbg_gen("not found"); > - goto done; > - } > - goto out_dent; > + else > + inode = ERR_PTR(err); > + goto done; > } > > if (dbg_check_name(c, dent, &nm)) { > - err = -EINVAL; > - goto out_dent; > + inode = ERR_PTR(-EINVAL); > + goto done; > } > > inode = ubifs_iget(dir->i_sb, le64_to_cpu(dent->inum)); > @@ -272,7 +272,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > ubifs_err(c, "dead directory entry '%pd', error %d", > dentry, err); > ubifs_ro_mode(c, err); > - goto out_dent; > + goto done; > } > > if (ubifs_crypt_is_encrypted(dir) && > @@ -280,27 +280,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > !fscrypt_has_permitted_context(dir, inode)) { > ubifs_warn(c, "Inconsistent encryption contexts: %lu/%lu", > dir->i_ino, inode->i_ino); > - err = -EPERM; > - goto out_inode; > + iput(inode); > + inode = ERR_PTR(-EPERM); > } > > done: > kfree(dent); > fscrypt_free_filename(&nm); > - /* > - * Note, d_splice_alias() would be required instead if we supported > - * NFS. > - */ > - d_add(dentry, inode); > - return NULL; > - > -out_inode: > - iput(inode); > -out_dent: > - kfree(dent); > -out_fname: > - fscrypt_free_filename(&nm); > - return ERR_PTR(err); > + return d_splice_alias(inode, dentry); > } > > static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode, > Acked-by: Richard Weinberger <richard@nod.at> Thanks, //richard ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 10/15] qnx6_lookup: switch to d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (7 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 09/15] ubifs_lookup: " Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias() Al Viro ` (5 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> ... and hash negative lookups Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/qnx6/namei.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/fs/qnx6/namei.c b/fs/qnx6/namei.c index 72c2770830be..e2e98e653b8d 100644 --- a/fs/qnx6/namei.c +++ b/fs/qnx6/namei.c @@ -29,15 +29,11 @@ struct dentry *qnx6_lookup(struct inode *dir, struct dentry *dentry, if (ino) { foundinode = qnx6_iget(dir->i_sb, ino); qnx6_put_page(page); - if (IS_ERR(foundinode)) { + if (IS_ERR(foundinode)) pr_debug("lookup->iget -> error %ld\n", PTR_ERR(foundinode)); - return ERR_CAST(foundinode); - } } else { pr_debug("%s(): not found %s\n", __func__, name); - return NULL; } - d_add(dentry, foundinode); - return NULL; + return d_splice_alias(foundinode, dentry); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (8 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 10/15] qnx6_lookup: switch to d_splice_alias() Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 12/15] adfs_lookup_byname: .. *is* taken care of in fs/namei.c Al Viro ` (4 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/romfs/super.c | 36 +++++++++--------------------------- 1 file changed, 9 insertions(+), 27 deletions(-) diff --git a/fs/romfs/super.c b/fs/romfs/super.c index 8f06fd1f3d69..6ccb51993a76 100644 --- a/fs/romfs/super.c +++ b/fs/romfs/super.c @@ -213,7 +213,7 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) { unsigned long offset, maxoff; - struct inode *inode; + struct inode *inode = NULL; struct romfs_inode ri; const char *name; /* got from dentry */ int len, ret; @@ -233,7 +233,7 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry, for (;;) { if (!offset || offset >= maxoff) - goto out0; + break; ret = romfs_dev_read(dir->i_sb, offset, &ri, sizeof(ri)); if (ret < 0) @@ -244,37 +244,19 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry, len); if (ret < 0) goto error; - if (ret == 1) + if (ret == 1) { + /* Hard link handling */ + if ((be32_to_cpu(ri.next) & ROMFH_TYPE) == ROMFH_HRD) + offset = be32_to_cpu(ri.spec) & ROMFH_MASK; + inode = romfs_iget(dir->i_sb, offset); break; + } /* next entry */ offset = be32_to_cpu(ri.next) & ROMFH_MASK; } - /* Hard link handling */ - if ((be32_to_cpu(ri.next) & ROMFH_TYPE) == ROMFH_HRD) - offset = be32_to_cpu(ri.spec) & ROMFH_MASK; - - inode = romfs_iget(dir->i_sb, offset); - if (IS_ERR(inode)) { - ret = PTR_ERR(inode); - goto error; - } - goto outi; - - /* - * it's a bit funky, _lookup needs to return an error code - * (negative) or a NULL, both as a dentry. ENOENT should not - * be returned, instead we need to create a negative dentry by - * d_add(dentry, NULL); and return 0 as no error. - * (Although as I see, it only matters on writable file - * systems). - */ -out0: - inode = NULL; -outi: - d_add(dentry, inode); - ret = 0; + return d_splice_alias(inode, dentry); error: return ERR_PTR(ret); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 12/15] adfs_lookup_byname: .. *is* taken care of in fs/namei.c 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (9 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias() Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 13/15] adfs_lookup: do not fail with ENOENT on negatives, use d_splice_alias() Al Viro ` (3 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Russell King From: Al Viro <viro@zeniv.linux.org.uk> Cc: Russell King <linux@armlinux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/adfs/dir.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c index 29444c83da48..77a2d6ca3c60 100644 --- a/fs/adfs/dir.c +++ b/fs/adfs/dir.c @@ -146,20 +146,6 @@ adfs_dir_lookup_byname(struct inode *inode, const struct qstr *name, struct obje obj->parent_id = inode->i_ino; - /* - * '.' is handled by reserved_lookup() in fs/namei.c - */ - if (name->len == 2 && name->name[0] == '.' && name->name[1] == '.') { - /* - * Currently unable to fill in the rest of 'obj', - * but this is better than nothing. We need to - * ascend one level to find it's parent. - */ - obj->name_len = 0; - obj->file_id = obj->parent_id; - goto free_out; - } - read_lock(&adfs_dir_lock); ret = ops->setpos(&dir, 0); -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 13/15] adfs_lookup: do not fail with ENOENT on negatives, use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (10 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 12/15] adfs_lookup_byname: .. *is* taken care of in fs/namei.c Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-13 21:30 ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro ` (2 subsequent siblings) 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Russell King From: Al Viro <viro@zeniv.linux.org.uk> Cc: Russell King <linux@armlinux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/adfs/dir.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c index 77a2d6ca3c60..e18eff854e1a 100644 --- a/fs/adfs/dir.c +++ b/fs/adfs/dir.c @@ -252,17 +252,17 @@ adfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) error = adfs_dir_lookup_byname(dir, &dentry->d_name, &obj); if (error == 0) { - error = -EACCES; /* * This only returns NULL if get_empty_inode * fails. */ inode = adfs_iget(dir->i_sb, &obj); - if (inode) - error = 0; + if (!inode) + inode = ERR_PTR(-EACCES); + } else if (error != -ENOENT) { + inode = ERR_PTR(error); } - d_add(dentry, inode); - return ERR_PTR(error); + return d_splice_alias(inode, dentry); } /* -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 14/15] xfs_vn_lookup: simplify a bit 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (11 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 13/15] adfs_lookup: do not fail with ENOENT on negatives, use d_splice_alias() Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-14 8:41 ` Christoph Hellwig 2018-05-13 21:30 ` [PATCH 15/15] openpromfs: switch to d_splice_alias() Al Viro 2018-05-16 9:45 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Tigran Aivazian 14 siblings, 1 reply; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel; +Cc: Darrick J . Wong From: Al Viro <viro@zeniv.linux.org.uk> have all post-xfs_lookup() branches converge on d_splice_alias() Cc: Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/xfs/xfs_iops.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index a3ed3c811dfa..df42e4cb4dc4 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -260,6 +260,7 @@ xfs_vn_lookup( struct dentry *dentry, unsigned int flags) { + struct inode *inode; struct xfs_inode *cip; struct xfs_name name; int error; @@ -269,14 +270,13 @@ xfs_vn_lookup( xfs_dentry_to_name(&name, dentry); error = xfs_lookup(XFS_I(dir), &name, &cip, NULL); - if (unlikely(error)) { - if (unlikely(error != -ENOENT)) - return ERR_PTR(error); - d_add(dentry, NULL); - return NULL; - } - - return d_splice_alias(VFS_I(cip), dentry); + if (likely(!error)) + inode = VFS_I(cip); + else if (likely(error == -ENOENT)) + inode = NULL; + else + inode = ERR_PTR(error); + return d_splice_alias(inode, dentry); } STATIC struct dentry * -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 14/15] xfs_vn_lookup: simplify a bit 2018-05-13 21:30 ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro @ 2018-05-14 8:41 ` Christoph Hellwig 0 siblings, 0 replies; 46+ messages in thread From: Christoph Hellwig @ 2018-05-14 8:41 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, Darrick J . Wong On Sun, May 13, 2018 at 10:30:16PM +0100, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > have all post-xfs_lookup() branches converge on d_splice_alias() I think this would be even better if you'd switch xfs_lookup so that it returns a struct inode instead or ERR_PTR instead of an int and a struct xfs_inode in the argument first. That is what all callers want anyway. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 15/15] openpromfs: switch to d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (12 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro @ 2018-05-13 21:30 ` Al Viro 2018-05-16 9:45 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Tigran Aivazian 14 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/openpromfs/inode.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/openpromfs/inode.c b/fs/openpromfs/inode.c index 2200662a9bf1..607092f367ad 100644 --- a/fs/openpromfs/inode.c +++ b/fs/openpromfs/inode.c @@ -256,8 +256,7 @@ static struct dentry *openpromfs_lookup(struct inode *dir, struct dentry *dentry break; } - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } static int openpromfs_readdir(struct file *file, struct dir_context *ctx) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 01/15] bfs_lookup(): use d_splice_alias() 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro ` (13 preceding siblings ...) 2018-05-13 21:30 ` [PATCH 15/15] openpromfs: switch to d_splice_alias() Al Viro @ 2018-05-16 9:45 ` Tigran Aivazian 14 siblings, 0 replies; 46+ messages in thread From: Tigran Aivazian @ 2018-05-16 9:45 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel Hi Al, Thank you. This feels a bit like a "time machine", i.e. being instantly transferred to the almost forgotten distant past for the joy of seeing familiar faces. Well, past, present or future --- Al Viro is always right (especially as far as filesystems' code is concerned) and I safely defer to you --- if you say it is better, then it most certainly is, dear friend. And to me it looks better too. Kind regards, Tigran On 13 May 2018 at 22:30, Al Viro <viro@zeniv.linux.org.uk> wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > code is actually simpler that way. > > Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/bfs/dir.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c > index ee832ca5f734..facf9614a381 100644 > --- a/fs/bfs/dir.c > +++ b/fs/bfs/dir.c > @@ -141,14 +141,9 @@ static struct dentry *bfs_lookup(struct inode *dir, struct dentry *dentry, > unsigned long ino = (unsigned long)le16_to_cpu(de->ino); > brelse(bh); > inode = bfs_iget(dir->i_sb, ino); > - if (IS_ERR(inode)) { > - mutex_unlock(&info->bfs_lock); > - return ERR_CAST(inode); > - } > } > mutex_unlock(&info->bfs_lock); > - d_add(dentry, inode); > - return NULL; > + return d_splice_alias(inode, dentry); > } > > static int bfs_link(struct dentry *old, struct inode *dir, > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC][PATCHES] reducing d_add() use, part 2 2018-05-13 21:26 [RFC][PATCHES] reducing d_add() use, part 1 Al Viro 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro @ 2018-05-25 23:53 ` Al Viro 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro 2018-05-26 0:03 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro 1 sibling, 2 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:53 UTC (permalink / raw) To: linux-fsdevel On Sun, May 13, 2018 at 10:26:12PM +0100, Al Viro wrote: > A lot of d_add() uses are in ->lookup() instances; > those really should be d_splice_alias() - that's mandatory > for anything exported (and we'd seen people not bothering > to convert when adding exports/open-by-fhandle support) *and* > it's not costlier than d_add() anyway, in all cases when > d_add() wouldn't have caused instant breakage. > > What's more, d_splice_alias() makes for simpler life > in callers - it does the right thing when given ERR_PTR(), > which simplifies the logics in quite a few ->lookup() instances. > > There are trickier cases, but a bunch of those call > sites are completely straightforward. See followups, or > vfs.git#work.lookup Continuation of the series - more simple cases: openpromfs: switch to d_splice_alias() orangefs_lookup: simplify omfs_lookup(): report IO errors, use d_splice_alias() hfs: use d_splice_alias() hfs: don't allow mounting over .../rsrc hfsplus: switch to d_splice_alias() ncp_lookup(): use d_splice_alias() 9p: unify paths in v9fs_vfs_lookup() cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL cifs_lookup(): switch to d_splice_alias() ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 01/10] openpromfs: switch to d_splice_alias() 2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 02/10] orangefs_lookup: simplify Al Viro ` (8 more replies) 2018-05-26 0:03 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro 1 sibling, 9 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/openpromfs/inode.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/openpromfs/inode.c b/fs/openpromfs/inode.c index 2200662a9bf1..607092f367ad 100644 --- a/fs/openpromfs/inode.c +++ b/fs/openpromfs/inode.c @@ -256,8 +256,7 @@ static struct dentry *openpromfs_lookup(struct inode *dir, struct dentry *dentry break; } - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } static int openpromfs_readdir(struct file *file, struct dir_context *ctx) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 02/10] orangefs_lookup: simplify 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-31 18:23 ` Mike Marshall 2018-05-25 23:54 ` [PATCH 03/10] omfs_lookup(): report IO errors, use d_splice_alias() Al Viro ` (7 subsequent siblings) 8 siblings, 1 reply; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> d_splice_alias() can handle NULL and ERR_PTR() for inode just fine... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/orangefs/namei.c | 64 +++++++---------------------------------------------- 1 file changed, 8 insertions(+), 56 deletions(-) diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c index 1b5707c44c3f..365cd73d9109 100644 --- a/fs/orangefs/namei.c +++ b/fs/orangefs/namei.c @@ -110,7 +110,6 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry, struct orangefs_inode_s *parent = ORANGEFS_I(dir); struct orangefs_kernel_op_s *new_op; struct inode *inode; - struct dentry *res; int ret = -EINVAL; /* @@ -158,65 +157,18 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry, new_op->downcall.resp.lookup.refn.fs_id, ret); - if (ret < 0) { - if (ret == -ENOENT) { - /* - * if no inode was found, add a negative dentry to - * dcache anyway; if we don't, we don't hold expected - * lookup semantics and we most noticeably break - * during directory renames. - * - * however, if the operation failed or exited, do not - * add the dentry (e.g. in the case that a touch is - * issued on a file that already exists that was - * interrupted during this lookup -- no need to add - * another negative dentry for an existing file) - */ - - gossip_debug(GOSSIP_NAME_DEBUG, - "orangefs_lookup: Adding *negative* dentry " - "%p for %pd\n", - dentry, - dentry); - - d_add(dentry, NULL); - res = NULL; - goto out; - } - + if (ret >= 0) { + orangefs_set_timeout(dentry); + inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn); + } else if (ret == -ENOENT) { + inode = NULL; + } else { /* must be a non-recoverable error */ - res = ERR_PTR(ret); - goto out; - } - - orangefs_set_timeout(dentry); - - inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn); - if (IS_ERR(inode)) { - gossip_debug(GOSSIP_NAME_DEBUG, - "error %ld from iget\n", PTR_ERR(inode)); - res = ERR_CAST(inode); - goto out; + inode = ERR_PTR(ret); } - gossip_debug(GOSSIP_NAME_DEBUG, - "%s:%s:%d " - "Found good inode [%lu] with count [%d]\n", - __FILE__, - __func__, - __LINE__, - inode->i_ino, - (int)atomic_read(&inode->i_count)); - - /* update dentry/inode pair into dcache */ - res = d_splice_alias(inode, dentry); - - gossip_debug(GOSSIP_NAME_DEBUG, - "Lookup success (inode ct = %d)\n", - (int)atomic_read(&inode->i_count)); -out: op_release(new_op); - return res; + return d_splice_alias(inode, dentry); } /* return 0 on success; non-zero otherwise */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 02/10] orangefs_lookup: simplify 2018-05-25 23:54 ` [PATCH 02/10] orangefs_lookup: simplify Al Viro @ 2018-05-31 18:23 ` Mike Marshall 0 siblings, 0 replies; 46+ messages in thread From: Mike Marshall @ 2018-05-31 18:23 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel Thanks for the cleanup. This runs through xfstests with no regressions on 4.17-rc7. Please add: Tested-by: Mike Marshall <hubcap@omnibond.com> -Mike On Fri, May 25, 2018 at 7:54 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > d_splice_alias() can handle NULL and ERR_PTR() for inode just fine... > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/orangefs/namei.c | 64 +++++++---------------------------------------------- > 1 file changed, 8 insertions(+), 56 deletions(-) > > diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c > index 1b5707c44c3f..365cd73d9109 100644 > --- a/fs/orangefs/namei.c > +++ b/fs/orangefs/namei.c > @@ -110,7 +110,6 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry, > struct orangefs_inode_s *parent = ORANGEFS_I(dir); > struct orangefs_kernel_op_s *new_op; > struct inode *inode; > - struct dentry *res; > int ret = -EINVAL; > > /* > @@ -158,65 +157,18 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry, > new_op->downcall.resp.lookup.refn.fs_id, > ret); > > - if (ret < 0) { > - if (ret == -ENOENT) { > - /* > - * if no inode was found, add a negative dentry to > - * dcache anyway; if we don't, we don't hold expected > - * lookup semantics and we most noticeably break > - * during directory renames. > - * > - * however, if the operation failed or exited, do not > - * add the dentry (e.g. in the case that a touch is > - * issued on a file that already exists that was > - * interrupted during this lookup -- no need to add > - * another negative dentry for an existing file) > - */ > - > - gossip_debug(GOSSIP_NAME_DEBUG, > - "orangefs_lookup: Adding *negative* dentry " > - "%p for %pd\n", > - dentry, > - dentry); > - > - d_add(dentry, NULL); > - res = NULL; > - goto out; > - } > - > + if (ret >= 0) { > + orangefs_set_timeout(dentry); > + inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn); > + } else if (ret == -ENOENT) { > + inode = NULL; > + } else { > /* must be a non-recoverable error */ > - res = ERR_PTR(ret); > - goto out; > - } > - > - orangefs_set_timeout(dentry); > - > - inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn); > - if (IS_ERR(inode)) { > - gossip_debug(GOSSIP_NAME_DEBUG, > - "error %ld from iget\n", PTR_ERR(inode)); > - res = ERR_CAST(inode); > - goto out; > + inode = ERR_PTR(ret); > } > > - gossip_debug(GOSSIP_NAME_DEBUG, > - "%s:%s:%d " > - "Found good inode [%lu] with count [%d]\n", > - __FILE__, > - __func__, > - __LINE__, > - inode->i_ino, > - (int)atomic_read(&inode->i_count)); > - > - /* update dentry/inode pair into dcache */ > - res = d_splice_alias(inode, dentry); > - > - gossip_debug(GOSSIP_NAME_DEBUG, > - "Lookup success (inode ct = %d)\n", > - (int)atomic_read(&inode->i_count)); > -out: > op_release(new_op); > - return res; > + return d_splice_alias(inode, dentry); > } > > /* return 0 on success; non-zero otherwise */ > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 03/10] omfs_lookup(): report IO errors, use d_splice_alias() 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro 2018-05-25 23:54 ` [PATCH 02/10] orangefs_lookup: simplify Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 04/10] hfs: " Al Viro ` (6 subsequent siblings) 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/omfs/dir.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fs/omfs/dir.c b/fs/omfs/dir.c index b7146526afff..4bee3a72b9f3 100644 --- a/fs/omfs/dir.c +++ b/fs/omfs/dir.c @@ -305,11 +305,10 @@ static struct dentry *omfs_lookup(struct inode *dir, struct dentry *dentry, ino_t ino = be64_to_cpu(oi->i_head.h_self); brelse(bh); inode = omfs_iget(dir->i_sb, ino); - if (IS_ERR(inode)) - return ERR_CAST(inode); + } else if (bh != ERR_PTR(-ENOENT)) { + inode = ERR_CAST(bh); } - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } /* sanity check block's self pointer */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 04/10] hfs: use d_splice_alias() 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro 2018-05-25 23:54 ` [PATCH 02/10] orangefs_lookup: simplify Al Viro 2018-05-25 23:54 ` [PATCH 03/10] omfs_lookup(): report IO errors, use d_splice_alias() Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 05/10] hfs: don't allow mounting over .../rsrc Al Viro ` (5 subsequent siblings) 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> code is simpler that way Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/hfs/dir.c | 20 +++++++------------- fs/hfs/inode.c | 3 +-- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c index 75b254280ff6..3bf2ae0e467c 100644 --- a/fs/hfs/dir.c +++ b/fs/hfs/dir.c @@ -31,21 +31,15 @@ static struct dentry *hfs_lookup(struct inode *dir, struct dentry *dentry, hfs_cat_build_key(dir->i_sb, fd.search_key, dir->i_ino, &dentry->d_name); res = hfs_brec_read(&fd, &rec, sizeof(rec)); if (res) { - hfs_find_exit(&fd); - if (res == -ENOENT) { - /* No such entry */ - inode = NULL; - goto done; - } - return ERR_PTR(res); + if (res != -ENOENT) + inode = ERR_PTR(res); + } else { + inode = hfs_iget(dir->i_sb, &fd.search_key->cat, &rec); + if (!inode) + inode = ERR_PTR(-EACCES); } - inode = hfs_iget(dir->i_sb, &fd.search_key->cat, &rec); hfs_find_exit(&fd); - if (!inode) - return ERR_PTR(-EACCES); -done: - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } /* diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c index 2538b49cc349..0612fa367bd1 100644 --- a/fs/hfs/inode.c +++ b/fs/hfs/inode.c @@ -544,8 +544,7 @@ static struct dentry *hfs_file_lookup(struct inode *dir, struct dentry *dentry, hlist_add_fake(&inode->i_hash); mark_inode_dirty(inode); out: - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } void hfs_evict_inode(struct inode *inode) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 05/10] hfs: don't allow mounting over .../rsrc 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro ` (2 preceding siblings ...) 2018-05-25 23:54 ` [PATCH 04/10] hfs: " Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 06/10] hfsplus: switch to d_splice_alias() Al Viro ` (4 subsequent siblings) 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> That's one case when unlink() destroys a subtree, thanks to "resource fork" idiocy. We might forcibly evict that shit on unlink(2), but for now let's just disallow overmounting; as it is, anything that plays games with those would leak mounts. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/hfs/inode.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c index 0612fa367bd1..b3309b83371a 100644 --- a/fs/hfs/inode.c +++ b/fs/hfs/inode.c @@ -543,6 +543,7 @@ static struct dentry *hfs_file_lookup(struct inode *dir, struct dentry *dentry, igrab(dir); hlist_add_fake(&inode->i_hash); mark_inode_dirty(inode); + dont_mount(dentry); out: return d_splice_alias(inode, dentry); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 06/10] hfsplus: switch to d_splice_alias() 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro ` (3 preceding siblings ...) 2018-05-25 23:54 ` [PATCH 05/10] hfs: don't allow mounting over .../rsrc Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 07/10] ncp_lookup(): use d_splice_alias() Al Viro ` (3 subsequent siblings) 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/hfsplus/dir.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c index 15e06fb552da..b5254378f011 100644 --- a/fs/hfsplus/dir.c +++ b/fs/hfsplus/dir.c @@ -122,8 +122,7 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry, if (S_ISREG(inode->i_mode)) HFSPLUS_I(inode)->linkid = linkid; out: - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); fail: hfs_find_exit(&fd); return ERR_PTR(err); -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 07/10] ncp_lookup(): use d_splice_alias() 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro ` (4 preceding siblings ...) 2018-05-25 23:54 ` [PATCH 06/10] hfsplus: switch to d_splice_alias() Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup() Al Viro ` (2 subsequent siblings) 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/ncpfs/dir.c | 42 +++++++++++++++--------------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/drivers/staging/ncpfs/dir.c b/drivers/staging/ncpfs/dir.c index 0c57c5c5d40a..072bcb12898f 100644 --- a/drivers/staging/ncpfs/dir.c +++ b/drivers/staging/ncpfs/dir.c @@ -823,12 +823,11 @@ static struct dentry *ncp_lookup(struct inode *dir, struct dentry *dentry, unsig struct ncp_server *server = NCP_SERVER(dir); struct inode *inode = NULL; struct ncp_entry_info finfo; - int error, res, len; + int res, len; __u8 __name[NCP_MAXPATHLEN + 1]; - error = -EIO; if (!ncp_conn_valid(server)) - goto finished; + return ERR_PTR(-EIO); ncp_vdbg("server lookup for %pd2\n", dentry); @@ -847,31 +846,20 @@ static struct dentry *ncp_lookup(struct inode *dir, struct dentry *dentry, unsig res = ncp_obtain_info(server, dir, __name, &(finfo.i)); } ncp_vdbg("looked for %pd2, res=%d\n", dentry, res); - /* - * If we didn't find an entry, make a negative dentry. - */ - if (res) - goto add_entry; - - /* - * Create an inode for the entry. - */ - finfo.opened = 0; - finfo.ino = iunique(dir->i_sb, 2); - finfo.volume = finfo.i.volNumber; - error = -EACCES; - inode = ncp_iget(dir->i_sb, &finfo); - - if (inode) { - ncp_new_dentry(dentry); -add_entry: - d_add(dentry, inode); - error = 0; + if (!res) { + /* + * Entry found; create an inode for it. + */ + finfo.opened = 0; + finfo.ino = iunique(dir->i_sb, 2); + finfo.volume = finfo.i.volNumber; + inode = ncp_iget(dir->i_sb, &finfo); + if (unlikely(!inode)) + inode = ERR_PTR(-EACCES); + else + ncp_new_dentry(dentry); } - -finished: - ncp_vdbg("result=%d\n", error); - return ERR_PTR(error); + return d_splice_alias(inode, dentry); } /* -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup() 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro ` (5 preceding siblings ...) 2018-05-25 23:54 ` [PATCH 07/10] ncp_lookup(): use d_splice_alias() Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 09/10] cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL Al Viro 2018-05-25 23:54 ` [PATCH 10/10] cifs_lookup(): switch to d_splice_alias() Al Viro 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/9p/vfs_inode.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 9ee534159cc6..42e102e2e74a 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -823,28 +823,21 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry, if (IS_ERR(dfid)) return ERR_CAST(dfid); - name = dentry->d_name.name; - fid = p9_client_walk(dfid, 1, &name, 1); - if (IS_ERR(fid)) { - if (fid == ERR_PTR(-ENOENT)) { - d_add(dentry, NULL); - return NULL; - } - return ERR_CAST(fid); - } /* * Make sure we don't use a wrong inode due to parallel * unlink. For cached mode create calls request for new * inode. But with cache disabled, lookup should do this. */ - if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) + name = dentry->d_name.name; + fid = p9_client_walk(dfid, 1, &name, 1); + if (fid == ERR_PTR(-ENOENT)) + inode = NULL; + else if (IS_ERR(fid)) + inode = ERR_CAST(fid); + else if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) inode = v9fs_get_inode_from_fid(v9ses, fid, dir->i_sb); else inode = v9fs_get_new_inode_from_fid(v9ses, fid, dir->i_sb); - if (IS_ERR(inode)) { - p9_client_clunk(fid); - return ERR_CAST(inode); - } /* * If we had a rename on the server and a parallel lookup * for the new name, then make sure we instantiate with @@ -853,12 +846,14 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry, * k/b. */ res = d_splice_alias(inode, dentry); - if (!res) - v9fs_fid_add(dentry, fid); - else if (!IS_ERR(res)) - v9fs_fid_add(res, fid); - else - p9_client_clunk(fid); + if (!IS_ERR(fid)) { + if (!res) + v9fs_fid_add(dentry, fid); + else if (!IS_ERR(res)) + v9fs_fid_add(res, fid); + else + p9_client_clunk(fid); + } return res; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 09/10] cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro ` (6 preceding siblings ...) 2018-05-25 23:54 ` [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup() Al Viro @ 2018-05-25 23:54 ` Al Viro 2018-05-25 23:54 ` [PATCH 10/10] cifs_lookup(): switch to d_splice_alias() Al Viro 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> not since 2004... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/cifs/dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 81ba6e0d88d8..ecbf36c459ea 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -812,7 +812,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, parent_dir_inode->i_sb, xid, NULL); } - if ((rc == 0) && (newInode != NULL)) { + if (rc == 0) { d_add(direntry, newInode); /* since paths are not looked up by component - the parent directories are presumed to be good here */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 10/10] cifs_lookup(): switch to d_splice_alias() 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro ` (7 preceding siblings ...) 2018-05-25 23:54 ` [PATCH 09/10] cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL Al Viro @ 2018-05-25 23:54 ` Al Viro 8 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw) To: linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/cifs/dir.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index ecbf36c459ea..751b8c9998ca 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -779,21 +779,25 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, tlink = cifs_sb_tlink(cifs_sb); if (IS_ERR(tlink)) { free_xid(xid); - return (struct dentry *)tlink; + return ERR_CAST(tlink); } pTcon = tlink_tcon(tlink); rc = check_name(direntry, pTcon); - if (rc) - goto lookup_out; + if (unlikely(rc)) { + cifs_put_tlink(tlink); + free_xid(xid); + return ERR_PTR(rc); + } /* can not grab the rename sem here since it would deadlock in the cases (beginning of sys_rename itself) in which we already have the sb rename sem */ full_path = build_path_from_dentry(direntry); if (full_path == NULL) { - rc = -ENOMEM; - goto lookup_out; + cifs_put_tlink(tlink); + free_xid(xid); + return ERR_PTR(-ENOMEM); } if (d_really_is_positive(direntry)) { @@ -813,28 +817,24 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, } if (rc == 0) { - d_add(direntry, newInode); /* since paths are not looked up by component - the parent directories are presumed to be good here */ renew_parental_timestamps(direntry); - } else if (rc == -ENOENT) { - rc = 0; cifs_set_time(direntry, jiffies); - d_add(direntry, NULL); - /* if it was once a directory (but how can we tell?) we could do - shrink_dcache_parent(direntry); */ - } else if (rc != -EACCES) { - cifs_dbg(FYI, "Unexpected lookup error %d\n", rc); - /* We special case check for Access Denied - since that - is a common return code */ + newInode = NULL; + } else { + if (rc != -EACCES) { + cifs_dbg(FYI, "Unexpected lookup error %d\n", rc); + /* We special case check for Access Denied - since that + is a common return code */ + } + newInode = ERR_PTR(rc); } - -lookup_out: kfree(full_path); cifs_put_tlink(tlink); free_xid(xid); - return ERR_PTR(rc); + return d_splice_alias(newInode, direntry); } static int -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [RFC][PATCHES] reducing d_add() use, part 3 (procfs) 2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro @ 2018-05-26 0:03 ` Al Viro 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro 2018-05-26 13:07 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Alexey Dobriyan 1 sibling, 2 replies; 46+ messages in thread From: Al Viro @ 2018-05-26 0:03 UTC (permalink / raw) To: linux-fsdevel; +Cc: Alexey Dobriyan On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote: > Continuation of the series - more simple cases: > openpromfs: switch to d_splice_alias() > orangefs_lookup: simplify > omfs_lookup(): report IO errors, use d_splice_alias() > hfs: use d_splice_alias() > hfs: don't allow mounting over .../rsrc > hfsplus: switch to d_splice_alias() > ncp_lookup(): use d_splice_alias() > 9p: unify paths in v9fs_vfs_lookup() > cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL > cifs_lookup(): switch to d_splice_alias() ... and a lot more convoluted ones - procfs side. There we have a bloody misleading games around pid_revalidate() and friends; we do *NOT* need those for any kind of race protection anymore (and hadn't for years). However, these calls cannot be simply dropped - some of their side effects (namely, setting ownership and permissions) are needed. Separating those into common helpers used by revalidate and by lookups allows to untangle that mess. I would really like an ACK/NAK from Alexey on that part... ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses 2018-05-26 0:03 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro @ 2018-05-26 0:04 ` Al Viro 2018-05-26 0:04 ` [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open Al Viro ` (4 more replies) 2018-05-26 13:07 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Alexey Dobriyan 1 sibling, 5 replies; 46+ messages in thread From: Al Viro @ 2018-05-26 0:04 UTC (permalink / raw) To: linux-fsdevel; +Cc: Alexey Dobriyan From: Al Viro <viro@zeniv.linux.org.uk> First of all, calling pid_revalidate() in the end of <pid>/* lookups is *not* about closing any kind of races; that used to be true once upon a time, but these days those comments are actively misleading. Especially since pid_revalidate() doesn't even do d_drop() on failure anymore. It doesn't matter, anyway, since once pid_revalidate() starts returning false, ->d_delete() of those dentries starts saying "don't keep"; they won't get stuck in dcache any longer than they are pinned. These calls cannot be just removed, though - the side effect of pid_revalidate() (updating i_uid/i_gid/etc.) is what we are calling it for here. Let's separate the "update ownership" into a new helper (pid_update_inode()) and use it, both in lookups and in pid_revalidate() itself. The comments in pid_revalidate() are also out of date - they refer to the time when pid_revalidate() used to call d_drop() directly... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/proc/base.c | 55 +++++++++++++++++++++++----------------------------- fs/proc/internal.h | 2 +- fs/proc/namespaces.c | 9 +++------ 3 files changed, 28 insertions(+), 38 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index eafa39a3a88c..6e0875505898 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1803,15 +1803,22 @@ int pid_getattr(const struct path *path, struct kstat *stat, /* dentry stuff */ /* - * Exceptional case: normally we are not allowed to unhash a busy - * directory. In this case, however, we can do it - no aliasing problems - * due to the way we treat inodes. - * + * Set <pid>/... inode ownership (can change due to setuid(), etc.) + */ +void pid_update_inode(struct task_struct *task, struct inode *inode) +{ + task_dump_owner(task, inode->i_mode, &inode->i_uid, &inode->i_gid); + + inode->i_mode &= ~(S_ISUID | S_ISGID); + security_task_to_inode(task, inode); +} + +/* * Rewrite the inode's ownerships here because the owning task may have * performed a setuid(), etc. * */ -int pid_revalidate(struct dentry *dentry, unsigned int flags) +static int pid_revalidate(struct dentry *dentry, unsigned int flags) { struct inode *inode; struct task_struct *task; @@ -1823,10 +1830,7 @@ int pid_revalidate(struct dentry *dentry, unsigned int flags) task = get_proc_task(inode); if (task) { - task_dump_owner(task, inode->i_mode, &inode->i_uid, &inode->i_gid); - - inode->i_mode &= ~(S_ISUID | S_ISGID); - security_task_to_inode(task, inode); + pid_update_inode(task, inode); put_task_struct(task); return 1; } @@ -2438,7 +2442,7 @@ static int proc_pident_instantiate(struct inode *dir, inode = proc_pid_make_inode(dir->i_sb, task, p->mode); if (!inode) - goto out; + return -ENOENT; ei = PROC_I(inode); if (S_ISDIR(inode->i_mode)) @@ -2448,13 +2452,10 @@ static int proc_pident_instantiate(struct inode *dir, if (p->fop) inode->i_fop = p->fop; ei->op = p->op; + pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); d_add(dentry, inode); - /* Close the race of the process dying before we return the dentry */ - if (pid_revalidate(dentry, 0)) - return 0; -out: - return -ENOENT; + return 0; } static struct dentry *proc_pident_lookup(struct inode *dir, @@ -3140,22 +3141,18 @@ static int proc_pid_instantiate(struct inode *dir, inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); if (!inode) - goto out; + return -ENOENT; inode->i_op = &proc_tgid_base_inode_operations; inode->i_fop = &proc_tgid_base_operations; inode->i_flags|=S_IMMUTABLE; set_nlink(inode, nlink_tgid); + pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); - d_add(dentry, inode); - /* Close the race of the process dying before we return the dentry */ - if (pid_revalidate(dentry, 0)) - return 0; -out: - return -ENOENT; + return 0; } struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags) @@ -3434,23 +3431,19 @@ static int proc_task_instantiate(struct inode *dir, { struct inode *inode; inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); - if (!inode) - goto out; + return -ENOENT; + inode->i_op = &proc_tid_base_inode_operations; inode->i_fop = &proc_tid_base_operations; - inode->i_flags|=S_IMMUTABLE; + inode->i_flags |= S_IMMUTABLE; set_nlink(inode, nlink_tid); + pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); - d_add(dentry, inode); - /* Close the race of the process dying before we return the dentry */ - if (pid_revalidate(dentry, 0)) - return 0; -out: - return -ENOENT; + return 0; } static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags) diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 0f1692e63cb6..04a455b9ae69 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -147,7 +147,7 @@ extern const struct dentry_operations pid_dentry_operations; extern int pid_getattr(const struct path *, struct kstat *, u32, unsigned int); extern int proc_setattr(struct dentry *, struct iattr *); extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *, umode_t); -extern int pid_revalidate(struct dentry *, unsigned int); +extern void pid_update_inode(struct task_struct *, struct inode *); extern int pid_delete_dentry(const struct dentry *); extern int proc_pid_readdir(struct file *, struct dir_context *); extern struct dentry *proc_pid_lookup(struct inode *, struct dentry *, unsigned int); diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c index 59b17e509f46..ad1adce6541d 100644 --- a/fs/proc/namespaces.c +++ b/fs/proc/namespaces.c @@ -96,19 +96,16 @@ static int proc_ns_instantiate(struct inode *dir, inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | S_IRWXUGO); if (!inode) - goto out; + return -ENOENT; ei = PROC_I(inode); inode->i_op = &proc_ns_link_inode_operations; ei->ns_ops = ns_ops; + pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); d_add(dentry, inode); - /* Close the race of the process dying before we return the dentry */ - if (pid_revalidate(dentry, 0)) - return 0; -out: - return -ENOENT; + return 0; } static int proc_ns_dir_readdir(struct file *file, struct dir_context *ctx) -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro @ 2018-05-26 0:04 ` Al Viro 2018-05-26 0:04 ` [PATCH 3/5] don't bother with tid_fd_revalidate() in lookups Al Viro ` (3 subsequent siblings) 4 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-26 0:04 UTC (permalink / raw) To: linux-fsdevel; +Cc: Alexey Dobriyan From: Al Viro <viro@zeniv.linux.org.uk> ... and take the "check if file is open, pick ->f_mode" into a helper; tid_fd_revalidate() can use it. The next patch will get rid of tid_fd_revalidate() calls in instantiate callbacks. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/proc/fd.c | 63 ++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 6b80cd1e419a..d38845ecc408 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -81,12 +81,29 @@ static const struct file_operations proc_fdinfo_file_operations = { .release = single_release, }; +static bool tid_fd_mode(struct task_struct *task, unsigned fd, fmode_t *mode) +{ + struct files_struct *files = get_files_struct(task); + struct file *file; + + if (!files) + return false; + + rcu_read_lock(); + file = fcheck_files(files, fd); + if (file) + *mode = file->f_mode; + rcu_read_unlock(); + put_files_struct(files); + return !!file; +} + static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags) { - struct files_struct *files; struct task_struct *task; struct inode *inode; unsigned int fd; + fmode_t f_mode; if (flags & LOOKUP_RCU) return -ECHILD; @@ -96,35 +113,20 @@ static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags) fd = proc_fd(inode); if (task) { - files = get_files_struct(task); - if (files) { - struct file *file; - - rcu_read_lock(); - file = fcheck_files(files, fd); - if (file) { - unsigned f_mode = file->f_mode; - - rcu_read_unlock(); - put_files_struct(files); - - task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); - - if (S_ISLNK(inode->i_mode)) { - unsigned i_mode = S_IFLNK; - if (f_mode & FMODE_READ) - i_mode |= S_IRUSR | S_IXUSR; - if (f_mode & FMODE_WRITE) - i_mode |= S_IWUSR | S_IXUSR; - inode->i_mode = i_mode; - } - - security_task_to_inode(task, inode); - put_task_struct(task); - return 1; + if (tid_fd_mode(task, fd, &f_mode)) { + task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); + + if (S_ISLNK(inode->i_mode)) { + unsigned i_mode = S_IFLNK; + if (f_mode & FMODE_READ) + i_mode |= S_IRUSR | S_IXUSR; + if (f_mode & FMODE_WRITE) + i_mode |= S_IWUSR | S_IXUSR; + inode->i_mode = i_mode; } - rcu_read_unlock(); - put_files_struct(files); + security_task_to_inode(task, inode); + put_task_struct(task); + return 1; } put_task_struct(task); } @@ -203,11 +205,14 @@ static struct dentry *proc_lookupfd_common(struct inode *dir, struct task_struct *task = get_proc_task(dir); int result = -ENOENT; unsigned fd = name_to_int(&dentry->d_name); + fmode_t f_mode; if (!task) goto out_no_task; if (fd == ~0U) goto out; + if (!tid_fd_mode(task, fd, &f_mode)) + goto out; result = instantiate(dir, dentry, task, (void *)(unsigned long)fd); out: -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 3/5] don't bother with tid_fd_revalidate() in lookups 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro 2018-05-26 0:04 ` [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open Al Viro @ 2018-05-26 0:04 ` Al Viro 2018-05-26 0:04 ` [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias() Al Viro ` (2 subsequent siblings) 4 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-26 0:04 UTC (permalink / raw) To: linux-fsdevel; +Cc: Alexey Dobriyan From: Al Viro <viro@zeniv.linux.org.uk> what we want it for is actually updating inode metadata; take _that_ into a separate helper and use it. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/proc/fd.c | 81 +++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 44 insertions(+), 37 deletions(-) diff --git a/fs/proc/fd.c b/fs/proc/fd.c index d38845ecc408..694faeacf42f 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -98,12 +98,27 @@ static bool tid_fd_mode(struct task_struct *task, unsigned fd, fmode_t *mode) return !!file; } +static void tid_fd_update_inode(struct task_struct *task, struct inode *inode, + fmode_t f_mode) +{ + task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); + + if (S_ISLNK(inode->i_mode)) { + unsigned i_mode = S_IFLNK; + if (f_mode & FMODE_READ) + i_mode |= S_IRUSR | S_IXUSR; + if (f_mode & FMODE_WRITE) + i_mode |= S_IWUSR | S_IXUSR; + inode->i_mode = i_mode; + } + security_task_to_inode(task, inode); +} + static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags) { struct task_struct *task; struct inode *inode; unsigned int fd; - fmode_t f_mode; if (flags & LOOKUP_RCU) return -ECHILD; @@ -113,18 +128,9 @@ static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags) fd = proc_fd(inode); if (task) { + fmode_t f_mode; if (tid_fd_mode(task, fd, &f_mode)) { - task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); - - if (S_ISLNK(inode->i_mode)) { - unsigned i_mode = S_IFLNK; - if (f_mode & FMODE_READ) - i_mode |= S_IRUSR | S_IXUSR; - if (f_mode & FMODE_WRITE) - i_mode |= S_IWUSR | S_IXUSR; - inode->i_mode = i_mode; - } - security_task_to_inode(task, inode); + tid_fd_update_inode(task, inode, f_mode); put_task_struct(task); return 1; } @@ -168,34 +174,35 @@ static int proc_fd_link(struct dentry *dentry, struct path *path) return ret; } +struct fd_data { + fmode_t mode; + unsigned fd; +}; + static int proc_fd_instantiate(struct inode *dir, struct dentry *dentry, struct task_struct *task, const void *ptr) { - unsigned fd = (unsigned long)ptr; + const struct fd_data *data = ptr; struct proc_inode *ei; struct inode *inode; inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK); if (!inode) - goto out; + return -ENOENT; ei = PROC_I(inode); - ei->fd = fd; + ei->fd = data->fd; inode->i_op = &proc_pid_link_inode_operations; inode->i_size = 64; ei->op.proc_get_link = proc_fd_link; + tid_fd_update_inode(task, inode, data->mode); d_set_d_op(dentry, &tid_fd_dentry_operations); d_add(dentry, inode); - - /* Close the race of the process dying before we return the dentry */ - if (tid_fd_revalidate(dentry, 0)) - return 0; - out: - return -ENOENT; + return 0; } static struct dentry *proc_lookupfd_common(struct inode *dir, @@ -204,17 +211,16 @@ static struct dentry *proc_lookupfd_common(struct inode *dir, { struct task_struct *task = get_proc_task(dir); int result = -ENOENT; - unsigned fd = name_to_int(&dentry->d_name); - fmode_t f_mode; + struct fd_data data = {.fd = name_to_int(&dentry->d_name)}; if (!task) goto out_no_task; - if (fd == ~0U) + if (data.fd == ~0U) goto out; - if (!tid_fd_mode(task, fd, &f_mode)) + if (!tid_fd_mode(task, data.fd, &data.mode)) goto out; - result = instantiate(dir, dentry, task, (void *)(unsigned long)fd); + result = instantiate(dir, dentry, task, &data); out: put_task_struct(task); out_no_task: @@ -241,17 +247,22 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx, for (fd = ctx->pos - 2; fd < files_fdtable(files)->max_fds; fd++, ctx->pos++) { + struct file *file; + struct fd_data data; char name[10 + 1]; int len; - if (!fcheck_files(files, fd)) + file = fcheck_files(files, fd); + if (!file) continue; + data.mode = file->f_mode; rcu_read_unlock(); + data.fd = fd; len = snprintf(name, sizeof(name), "%u", fd); if (!proc_fill_cache(file, ctx, name, len, instantiate, p, - (void *)(unsigned long)fd)) + &data)) goto out_fd_loop; cond_resched(); rcu_read_lock(); @@ -313,27 +324,23 @@ static int proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry, struct task_struct *task, const void *ptr) { - unsigned fd = (unsigned long)ptr; + const struct fd_data *data = ptr; struct proc_inode *ei; struct inode *inode; inode = proc_pid_make_inode(dir->i_sb, task, S_IFREG | S_IRUSR); if (!inode) - goto out; + return -ENOENT; ei = PROC_I(inode); - ei->fd = fd; + ei->fd = data->fd; inode->i_fop = &proc_fdinfo_file_operations; + tid_fd_update_inode(task, inode, 0); d_set_d_op(dentry, &tid_fd_dentry_operations); d_add(dentry, inode); - - /* Close the race of the process dying before we return the dentry */ - if (tid_fd_revalidate(dentry, 0)) - return 0; - out: - return -ENOENT; + return 0; } static struct dentry * -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias() 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro 2018-05-26 0:04 ` [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open Al Viro 2018-05-26 0:04 ` [PATCH 3/5] don't bother with tid_fd_revalidate() in lookups Al Viro @ 2018-05-26 0:04 ` Al Viro 2018-05-26 0:04 ` [PATCH 5/5] switch the rest of procfs lookups " Al Viro 2018-05-31 8:28 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Alexey Dobriyan 4 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-26 0:04 UTC (permalink / raw) To: linux-fsdevel; +Cc: Alexey Dobriyan From: Al Viro <viro@zeniv.linux.org.uk> ... and get rid of pointless struct inode *dir argument of those, while we are at it. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/proc/base.c | 93 ++++++++++++++++++++++++---------------------------- fs/proc/fd.c | 30 ++++++++--------- fs/proc/internal.h | 2 +- fs/proc/namespaces.c | 19 +++++------ 4 files changed, 65 insertions(+), 79 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 6e0875505898..de22c2002b38 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1878,8 +1878,8 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx, struct dentry *child, *dir = file->f_path.dentry; struct qstr qname = QSTR_INIT(name, len); struct inode *inode; - unsigned type; - ino_t ino; + unsigned type = DT_UNKNOWN; + ino_t ino = 1; child = d_hash_and_lookup(dir, &qname); if (!child) { @@ -1888,22 +1888,23 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx, if (IS_ERR(child)) goto end_instantiate; if (d_in_lookup(child)) { - int err = instantiate(d_inode(dir), child, task, ptr); + struct dentry *res; + res = instantiate(child, task, ptr); d_lookup_done(child); - if (err < 0) { - dput(child); + if (IS_ERR(res)) goto end_instantiate; + if (unlikely(res)) { + dput(child); + child = res; } } } inode = d_inode(child); ino = inode->i_ino; type = inode->i_mode >> 12; +end_instantiate: dput(child); return dir_emit(ctx, name, len, ino, type); - -end_instantiate: - return dir_emit(ctx, name, len, 1, DT_UNKNOWN); } /* @@ -2065,19 +2066,19 @@ static const struct inode_operations proc_map_files_link_inode_operations = { .setattr = proc_setattr, }; -static int -proc_map_files_instantiate(struct inode *dir, struct dentry *dentry, +static struct dentry * +proc_map_files_instantiate(struct dentry *dentry, struct task_struct *task, const void *ptr) { fmode_t mode = (fmode_t)(unsigned long)ptr; struct proc_inode *ei; struct inode *inode; - inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | ((mode & FMODE_READ ) ? S_IRUSR : 0) | ((mode & FMODE_WRITE) ? S_IWUSR : 0)); if (!inode) - return -ENOENT; + return ERR_PTR(-ENOENT); ei = PROC_I(inode); ei->op.proc_get_link = map_files_get_link; @@ -2086,9 +2087,7 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry, inode->i_size = 64; d_set_d_op(dentry, &tid_map_files_dentry_operations); - d_add(dentry, inode); - - return 0; + return d_splice_alias(inode, dentry); } static struct dentry *proc_map_files_lookup(struct inode *dir, @@ -2097,19 +2096,19 @@ static struct dentry *proc_map_files_lookup(struct inode *dir, unsigned long vm_start, vm_end; struct vm_area_struct *vma; struct task_struct *task; - int result; + struct dentry *result; struct mm_struct *mm; - result = -ENOENT; + result = ERR_PTR(-ENOENT); task = get_proc_task(dir); if (!task) goto out; - result = -EACCES; + result = ERR_PTR(-EACCES); if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) goto out_put_task; - result = -ENOENT; + result = ERR_PTR(-ENOENT); if (dname_to_vma_addr(dentry, &vm_start, &vm_end)) goto out_put_task; @@ -2123,7 +2122,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir, goto out_no_vma; if (vma->vm_file) - result = proc_map_files_instantiate(dir, dentry, task, + result = proc_map_files_instantiate(dentry, task, (void *)(unsigned long)vma->vm_file->f_mode); out_no_vma: @@ -2132,7 +2131,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir, out_put_task: put_task_struct(task); out: - return ERR_PTR(result); + return result; } static const struct inode_operations proc_map_files_inode_operations = { @@ -2433,16 +2432,16 @@ static const struct file_operations proc_pid_set_timerslack_ns_operations = { .release = single_release, }; -static int proc_pident_instantiate(struct inode *dir, - struct dentry *dentry, struct task_struct *task, const void *ptr) +static struct dentry *proc_pident_instantiate(struct dentry *dentry, + struct task_struct *task, const void *ptr) { const struct pid_entry *p = ptr; struct inode *inode; struct proc_inode *ei; - inode = proc_pid_make_inode(dir->i_sb, task, p->mode); + inode = proc_pid_make_inode(dentry->d_sb, task, p->mode); if (!inode) - return -ENOENT; + return ERR_PTR(-ENOENT); ei = PROC_I(inode); if (S_ISDIR(inode->i_mode)) @@ -2454,8 +2453,7 @@ static int proc_pident_instantiate(struct inode *dir, ei->op = p->op; pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); - d_add(dentry, inode); - return 0; + return d_splice_alias(inode, dentry); } static struct dentry *proc_pident_lookup(struct inode *dir, @@ -2463,11 +2461,9 @@ static struct dentry *proc_pident_lookup(struct inode *dir, const struct pid_entry *ents, unsigned int nents) { - int error; struct task_struct *task = get_proc_task(dir); const struct pid_entry *p, *last; - - error = -ENOENT; + struct dentry *res = ERR_PTR(-ENOENT); if (!task) goto out_no_task; @@ -2486,11 +2482,11 @@ static struct dentry *proc_pident_lookup(struct inode *dir, if (p >= last) goto out; - error = proc_pident_instantiate(dir, dentry, task, p); + res = proc_pident_instantiate(dentry, task, p); out: put_task_struct(task); out_no_task: - return ERR_PTR(error); + return res; } static int proc_pident_readdir(struct file *file, struct dir_context *ctx, @@ -3133,15 +3129,14 @@ void proc_flush_task(struct task_struct *task) } } -static int proc_pid_instantiate(struct inode *dir, - struct dentry * dentry, +static struct dentry *proc_pid_instantiate(struct dentry * dentry, struct task_struct *task, const void *ptr) { struct inode *inode; - inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); if (!inode) - return -ENOENT; + return ERR_PTR(-ENOENT); inode->i_op = &proc_tgid_base_inode_operations; inode->i_fop = &proc_tgid_base_operations; @@ -3151,16 +3146,15 @@ static int proc_pid_instantiate(struct inode *dir, pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); - d_add(dentry, inode); - return 0; + return d_splice_alias(inode, dentry); } struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags) { - int result = -ENOENT; struct task_struct *task; unsigned tgid; struct pid_namespace *ns; + struct dentry *result = ERR_PTR(-ENOENT); tgid = name_to_int(&dentry->d_name); if (tgid == ~0U) @@ -3175,10 +3169,10 @@ struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, unsign if (!task) goto out; - result = proc_pid_instantiate(dir, dentry, task, NULL); + result = proc_pid_instantiate(dentry, task, NULL); put_task_struct(task); out: - return ERR_PTR(result); + return result; } /* @@ -3426,13 +3420,13 @@ static const struct inode_operations proc_tid_base_inode_operations = { .setattr = proc_setattr, }; -static int proc_task_instantiate(struct inode *dir, - struct dentry *dentry, struct task_struct *task, const void *ptr) +static struct dentry *proc_task_instantiate(struct dentry *dentry, + struct task_struct *task, const void *ptr) { struct inode *inode; - inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); if (!inode) - return -ENOENT; + return ERR_PTR(-ENOENT); inode->i_op = &proc_tid_base_inode_operations; inode->i_fop = &proc_tid_base_operations; @@ -3442,17 +3436,16 @@ static int proc_task_instantiate(struct inode *dir, pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); - d_add(dentry, inode); - return 0; + return d_splice_alias(inode, dentry); } static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags) { - int result = -ENOENT; struct task_struct *task; struct task_struct *leader = get_proc_task(dir); unsigned tid; struct pid_namespace *ns; + struct dentry *result = ERR_PTR(-ENOENT); if (!leader) goto out_no_task; @@ -3472,13 +3465,13 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry if (!same_thread_group(leader, task)) goto out_drop_task; - result = proc_task_instantiate(dir, dentry, task, NULL); + result = proc_task_instantiate(dentry, task, NULL); out_drop_task: put_task_struct(task); out: put_task_struct(leader); out_no_task: - return ERR_PTR(result); + return result; } /* diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 694faeacf42f..e6a69fd6a2bb 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -179,17 +179,16 @@ struct fd_data { unsigned fd; }; -static int -proc_fd_instantiate(struct inode *dir, struct dentry *dentry, - struct task_struct *task, const void *ptr) +static struct dentry *proc_fd_instantiate(struct dentry *dentry, + struct task_struct *task, const void *ptr) { const struct fd_data *data = ptr; struct proc_inode *ei; struct inode *inode; - inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK); if (!inode) - return -ENOENT; + return ERR_PTR(-ENOENT); ei = PROC_I(inode); ei->fd = data->fd; @@ -201,8 +200,7 @@ proc_fd_instantiate(struct inode *dir, struct dentry *dentry, tid_fd_update_inode(task, inode, data->mode); d_set_d_op(dentry, &tid_fd_dentry_operations); - d_add(dentry, inode); - return 0; + return d_splice_alias(inode, dentry); } static struct dentry *proc_lookupfd_common(struct inode *dir, @@ -210,8 +208,8 @@ static struct dentry *proc_lookupfd_common(struct inode *dir, instantiate_t instantiate) { struct task_struct *task = get_proc_task(dir); - int result = -ENOENT; struct fd_data data = {.fd = name_to_int(&dentry->d_name)}; + struct dentry *result = ERR_PTR(-ENOENT); if (!task) goto out_no_task; @@ -220,11 +218,11 @@ static struct dentry *proc_lookupfd_common(struct inode *dir, if (!tid_fd_mode(task, data.fd, &data.mode)) goto out; - result = instantiate(dir, dentry, task, &data); + result = instantiate(dentry, task, &data); out: put_task_struct(task); out_no_task: - return ERR_PTR(result); + return result; } static int proc_readfd_common(struct file *file, struct dir_context *ctx, @@ -320,17 +318,16 @@ const struct inode_operations proc_fd_inode_operations = { .setattr = proc_setattr, }; -static int -proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry, - struct task_struct *task, const void *ptr) +static struct dentry *proc_fdinfo_instantiate(struct dentry *dentry, + struct task_struct *task, const void *ptr) { const struct fd_data *data = ptr; struct proc_inode *ei; struct inode *inode; - inode = proc_pid_make_inode(dir->i_sb, task, S_IFREG | S_IRUSR); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR); if (!inode) - return -ENOENT; + return ERR_PTR(-ENOENT); ei = PROC_I(inode); ei->fd = data->fd; @@ -339,8 +336,7 @@ proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry, tid_fd_update_inode(task, inode, 0); d_set_d_op(dentry, &tid_fd_dentry_operations); - d_add(dentry, inode); - return 0; + return d_splice_alias(inode, dentry); } static struct dentry * diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 04a455b9ae69..275b062e58af 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -154,7 +154,7 @@ extern struct dentry *proc_pid_lookup(struct inode *, struct dentry *, unsigned extern loff_t mem_lseek(struct file *, loff_t, int); /* Lookups */ -typedef int instantiate_t(struct inode *, struct dentry *, +typedef struct dentry *instantiate_t(struct dentry *, struct task_struct *, const void *); extern bool proc_fill_cache(struct file *, struct dir_context *, const char *, int, instantiate_t, struct task_struct *, const void *); diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c index ad1adce6541d..dd2b35f78b09 100644 --- a/fs/proc/namespaces.c +++ b/fs/proc/namespaces.c @@ -87,16 +87,16 @@ static const struct inode_operations proc_ns_link_inode_operations = { .setattr = proc_setattr, }; -static int proc_ns_instantiate(struct inode *dir, - struct dentry *dentry, struct task_struct *task, const void *ptr) +static struct dentry *proc_ns_instantiate(struct dentry *dentry, + struct task_struct *task, const void *ptr) { const struct proc_ns_operations *ns_ops = ptr; struct inode *inode; struct proc_inode *ei; - inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | S_IRWXUGO); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | S_IRWXUGO); if (!inode) - return -ENOENT; + return ERR_PTR(-ENOENT); ei = PROC_I(inode); inode->i_op = &proc_ns_link_inode_operations; @@ -104,8 +104,7 @@ static int proc_ns_instantiate(struct inode *dir, pid_update_inode(task, inode); d_set_d_op(dentry, &pid_dentry_operations); - d_add(dentry, inode); - return 0; + return d_splice_alias(inode, dentry); } static int proc_ns_dir_readdir(struct file *file, struct dir_context *ctx) @@ -144,12 +143,10 @@ const struct file_operations proc_ns_dir_operations = { static struct dentry *proc_ns_dir_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) { - int error; struct task_struct *task = get_proc_task(dir); const struct proc_ns_operations **entry, **last; unsigned int len = dentry->d_name.len; - - error = -ENOENT; + struct dentry *res = ERR_PTR(-ENOENT); if (!task) goto out_no_task; @@ -164,11 +161,11 @@ static struct dentry *proc_ns_dir_lookup(struct inode *dir, if (entry == last) goto out; - error = proc_ns_instantiate(dir, dentry, task, *entry); + res = proc_ns_instantiate(dentry, task, *entry); out: put_task_struct(task); out_no_task: - return ERR_PTR(error); + return res; } const struct inode_operations proc_ns_dir_inode_operations = { -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 5/5] switch the rest of procfs lookups to d_splice_alias() 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro ` (2 preceding siblings ...) 2018-05-26 0:04 ` [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias() Al Viro @ 2018-05-26 0:04 ` Al Viro 2018-05-31 8:28 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Alexey Dobriyan 4 siblings, 0 replies; 46+ messages in thread From: Al Viro @ 2018-05-26 0:04 UTC (permalink / raw) To: linux-fsdevel; +Cc: Alexey Dobriyan From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/proc/generic.c | 3 +-- fs/proc/proc_sysctl.c | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 2078e70e1595..b77034a694ef 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -256,8 +256,7 @@ struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry, if (!inode) return ERR_PTR(-ENOMEM); d_set_d_op(dentry, &proc_misc_dentry_ops); - d_add(dentry, inode); - return NULL; + return d_splice_alias(inode, dentry); } read_unlock(&proc_subdir_lock); return ERR_PTR(-ENOENT); diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 8989936f2995..4d765e5e91ed 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -554,9 +554,8 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry, if (!inode) goto out; - err = NULL; d_set_d_op(dentry, &proc_sys_dentry_operations); - d_add(dentry, inode); + err = d_splice_alias(inode, dentry); out: if (h) @@ -684,6 +683,7 @@ static bool proc_sys_fill_cache(struct file *file, if (IS_ERR(child)) return false; if (d_in_lookup(child)) { + struct dentry *res; inode = proc_sys_make_inode(dir->d_sb, head, table); if (!inode) { d_lookup_done(child); @@ -691,7 +691,16 @@ static bool proc_sys_fill_cache(struct file *file, return false; } d_set_d_op(child, &proc_sys_dentry_operations); - d_add(child, inode); + res = d_splice_alias(inode, child); + d_lookup_done(child); + if (unlikely(res)) { + if (IS_ERR(res)) { + dput(child); + return false; + } + dput(child); + child = res; + } } } inode = d_inode(child); -- 2.11.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro ` (3 preceding siblings ...) 2018-05-26 0:04 ` [PATCH 5/5] switch the rest of procfs lookups " Al Viro @ 2018-05-31 8:28 ` Alexey Dobriyan 4 siblings, 0 replies; 46+ messages in thread From: Alexey Dobriyan @ 2018-05-31 8:28 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel On Sat, May 26, 2018 at 01:04:06AM +0100, Al Viro wrote: > + pid_update_inode(task, inode); > d_set_d_op(dentry, &pid_dentry_operations); > d_add(dentry, inode); > - /* Close the race of the process dying before we return the dentry */ > - if (pid_revalidate(dentry, 0)) > - return 0; > -out: > - return -ENOENT; > + return 0; > } ACK this. I never understood what race is prevented if process can exit after pid_revalidate(). ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs) 2018-05-26 0:03 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro @ 2018-05-26 13:07 ` Alexey Dobriyan 2018-05-26 13:56 ` Alexey Dobriyan 1 sibling, 1 reply; 46+ messages in thread From: Alexey Dobriyan @ 2018-05-26 13:07 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel On Sat, May 26, 2018 at 01:03:02AM +0100, Al Viro wrote: > On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote: > ... and a lot more convoluted ones - procfs side. > I would really like an ACK/NAK from Alexey on that part... Is /proc part self contained? This happens with 5 patches against mainline: Welcome to Debian GNU/Linux 8 (jessie)! [ 0.764706] systemd[1]: Failed to insert module 'autofs4' [ 0.765357] systemd[1]: Failed to insert module 'ipv6' [ 0.766047] systemd[1]: Set hostname to <debian88>. [ 0.778266] random: systemd-sysv-ge: uninitialized urandom read (16 bytes read) [ 0.785611] random: systemd: uninitialized urandom read (16 bytes read) [ 0.790655] ------------[ cut here ]------------ [nothing here] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs) 2018-05-26 13:07 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Alexey Dobriyan @ 2018-05-26 13:56 ` Alexey Dobriyan 2018-05-26 18:20 ` Al Viro 0 siblings, 1 reply; 46+ messages in thread From: Alexey Dobriyan @ 2018-05-26 13:56 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel On Sat, May 26, 2018 at 04:07:26PM +0300, Alexey Dobriyan wrote: > On Sat, May 26, 2018 at 01:03:02AM +0100, Al Viro wrote: > > On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote: > > > ... and a lot more convoluted ones - procfs side. > > I would really like an ACK/NAK from Alexey on that part... > > Is /proc part self contained? This happens with 5 patches against mainline: > > Welcome to Debian GNU/Linux 8 (jessie)! > > [ 0.764706] systemd[1]: Failed to insert module 'autofs4' > [ 0.765357] systemd[1]: Failed to insert module 'ipv6' > [ 0.766047] systemd[1]: Set hostname to <debian88>. > [ 0.778266] random: systemd-sysv-ge: uninitialized urandom read (16 bytes read) > [ 0.785611] random: systemd: uninitialized urandom read (16 bytes read) > [ 0.790655] ------------[ cut here ]------------ Regardless, the broken patch is "[PATCH 3/5] don't bother with tid_fd_revalidate() in lookups". ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs) 2018-05-26 13:56 ` Alexey Dobriyan @ 2018-05-26 18:20 ` Al Viro 2018-05-26 18:38 ` Matthew Wilcox 0 siblings, 1 reply; 46+ messages in thread From: Al Viro @ 2018-05-26 18:20 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: linux-fsdevel On Sat, May 26, 2018 at 04:56:40PM +0300, Alexey Dobriyan wrote: > On Sat, May 26, 2018 at 04:07:26PM +0300, Alexey Dobriyan wrote: > > On Sat, May 26, 2018 at 01:03:02AM +0100, Al Viro wrote: > > > On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote: > > > > > ... and a lot more convoluted ones - procfs side. > > > I would really like an ACK/NAK from Alexey on that part... > > > > Is /proc part self contained? This happens with 5 patches against mainline: > > > > Welcome to Debian GNU/Linux 8 (jessie)! > > > > [ 0.764706] systemd[1]: Failed to insert module 'autofs4' > > [ 0.765357] systemd[1]: Failed to insert module 'ipv6' > > [ 0.766047] systemd[1]: Set hostname to <debian88>. > > [ 0.778266] random: systemd-sysv-ge: uninitialized urandom read (16 bytes read) > > [ 0.785611] random: systemd: uninitialized urandom read (16 bytes read) > > [ 0.790655] ------------[ cut here ]------------ > > Regardless, the broken patch is > "[PATCH 3/5] don't bother with tid_fd_revalidate() in lookups". D'oh... Incremental would be diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 694faeacf42f..f5de22a9e9e0 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -247,15 +247,15 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx, for (fd = ctx->pos - 2; fd < files_fdtable(files)->max_fds; fd++, ctx->pos++) { - struct file *file; + struct file *f; struct fd_data data; char name[10 + 1]; int len; - file = fcheck_files(files, fd); - if (!file) + f = fcheck_files(files, fd); + if (!f) continue; - data.mode = file->f_mode; + data.mode = f->f_mode; rcu_read_unlock(); data.fd = fd; ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs) 2018-05-26 18:20 ` Al Viro @ 2018-05-26 18:38 ` Matthew Wilcox 0 siblings, 0 replies; 46+ messages in thread From: Matthew Wilcox @ 2018-05-26 18:38 UTC (permalink / raw) To: Al Viro; +Cc: Alexey Dobriyan, linux-fsdevel On Sat, May 26, 2018 at 07:20:21PM +0100, Al Viro wrote: > D'oh... Incremental would be > > diff --git a/fs/proc/fd.c b/fs/proc/fd.c > index 694faeacf42f..f5de22a9e9e0 100644 > --- a/fs/proc/fd.c > +++ b/fs/proc/fd.c > @@ -247,15 +247,15 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx, > for (fd = ctx->pos - 2; > fd < files_fdtable(files)->max_fds; > fd++, ctx->pos++) { > - struct file *file; > + struct file *f; Ugh. -Wshadow doesn't get turned on until W=2 which is really damn noisy. It's going to take a _lot_ of work to move that over into W=1 or even set by default. ^ permalink raw reply [flat|nested] 46+ messages in thread
end of thread, other threads:[~2018-05-31 18:23 UTC | newest] Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-13 21:26 [RFC][PATCHES] reducing d_add() use, part 1 Al Viro 2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro 2018-05-13 21:30 ` [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer Al Viro 2018-05-13 21:30 ` [PATCH 03/15] bfs_add_entry: " Al Viro 2018-05-13 21:30 ` [PATCH 04/15] cramfs_lookup(): use d_splice_alias() Al Viro 2018-05-13 21:30 ` [PATCH 05/15] freevxfs_lookup(): " Al Viro 2018-05-14 8:41 ` Christoph Hellwig 2018-05-14 15:26 ` Al Viro 2018-05-13 21:30 ` [PATCH 06/15] minix_lookup: " Al Viro 2018-05-13 21:30 ` [PATCH 07/15] qnx4_lookup: " Al Viro 2018-05-14 10:48 ` Anders Larsen 2018-05-13 21:30 ` [PATCH 08/15] sysv_lookup: " Al Viro 2018-05-14 8:41 ` Christoph Hellwig 2018-05-13 21:30 ` [PATCH 09/15] ubifs_lookup: " Al Viro 2018-05-14 6:48 ` Richard Weinberger 2018-05-13 21:30 ` [PATCH 10/15] qnx6_lookup: switch to d_splice_alias() Al Viro 2018-05-13 21:30 ` [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias() Al Viro 2018-05-13 21:30 ` [PATCH 12/15] adfs_lookup_byname: .. *is* taken care of in fs/namei.c Al Viro 2018-05-13 21:30 ` [PATCH 13/15] adfs_lookup: do not fail with ENOENT on negatives, use d_splice_alias() Al Viro 2018-05-13 21:30 ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro 2018-05-14 8:41 ` Christoph Hellwig 2018-05-13 21:30 ` [PATCH 15/15] openpromfs: switch to d_splice_alias() Al Viro 2018-05-16 9:45 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Tigran Aivazian 2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro 2018-05-25 23:54 ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro 2018-05-25 23:54 ` [PATCH 02/10] orangefs_lookup: simplify Al Viro 2018-05-31 18:23 ` Mike Marshall 2018-05-25 23:54 ` [PATCH 03/10] omfs_lookup(): report IO errors, use d_splice_alias() Al Viro 2018-05-25 23:54 ` [PATCH 04/10] hfs: " Al Viro 2018-05-25 23:54 ` [PATCH 05/10] hfs: don't allow mounting over .../rsrc Al Viro 2018-05-25 23:54 ` [PATCH 06/10] hfsplus: switch to d_splice_alias() Al Viro 2018-05-25 23:54 ` [PATCH 07/10] ncp_lookup(): use d_splice_alias() Al Viro 2018-05-25 23:54 ` [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup() Al Viro 2018-05-25 23:54 ` [PATCH 09/10] cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL Al Viro 2018-05-25 23:54 ` [PATCH 10/10] cifs_lookup(): switch to d_splice_alias() Al Viro 2018-05-26 0:03 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro 2018-05-26 0:04 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro 2018-05-26 0:04 ` [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open Al Viro 2018-05-26 0:04 ` [PATCH 3/5] don't bother with tid_fd_revalidate() in lookups Al Viro 2018-05-26 0:04 ` [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias() Al Viro 2018-05-26 0:04 ` [PATCH 5/5] switch the rest of procfs lookups " Al Viro 2018-05-31 8:28 ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Alexey Dobriyan 2018-05-26 13:07 ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Alexey Dobriyan 2018-05-26 13:56 ` Alexey Dobriyan 2018-05-26 18:20 ` Al Viro 2018-05-26 18:38 ` Matthew Wilcox
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).