* [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
* [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
* [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
* [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
* [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
* [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 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
* 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 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
* 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
* 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
* 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
* 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
* [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: [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
* 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: [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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.