Re: [PATCH] debugfs: remove inc_nlink in debugfs_create_automount
Greg KH wrote on 2018/12/22 19:25:
On Sat, Dec 22, 2018 at 04:45:36PM +0800, yangerkun wrote:
Remove inc_nlink in debugfs_create_automount, or this inode will never
be free.
Signed-off-by: yangerkun
---
fs/debugfs/inode.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 13b01351dd1c..9294238e364f 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -551,12 +551,11 @@ struct dentry *debugfs_create_automount(const char *name,
if (unlikely(!inode))
return failed_creating(dentry);
+ /* directory inodes start off with i_nlink == 2 (for "." entry) */
make_empty_dir_inode(inode);
inode->i_flags |= S_AUTOMOUNT;
inode->i_private = data;
dentry->d_fsdata = (void *)f;
- /* directory inodes start off with i_nlink == 2 (for "." entry) */
- inc_nlink(inode);
Again, have you tested this and how? How did you find this issue?
thanks,
greg k-h
.
Thanks a lot for Al Viro's explain.
I was confusing about i_nlink and i_count before, so after insert and
remove module show as below, i think the inode can still exist with
nlink will be 1.
So the only problem is that the nlink should be 2 for a empty dir, and
for debugfs,the influence is that the stat for the dir. Right?
#include
#include
#include
static char *name = "test-dir";
module_param(name, charp, 0644);
MODULE_PARM_DESC(name, "Get an string(char *) value from user...\n");
struct inode *inode;
int init_module(void)
{
struct dentry *dentry;
dentry = debugfs_create_automount(name, NULL, NULL, NULL);
inode = dentry->d_inode;
debugfs_remove(dentry);
return 0;
}
void cleanup_module(void)
{
printk("%d\n", inode->i_nlink);
}
MODULE_LICENSE("GPL");
Thanks,
Kun.
Re: [PATCH] debugfs: remove inc_nlink in debugfs_create_automount
On Sat, Dec 22, 2018 at 04:45:36PM +0800, yangerkun wrote: > Remove inc_nlink in debugfs_create_automount, or this inode will never > be free. Explain, please. What exactly would care about i_nlink in debugfs? It does *NOT* free any kind of backing store on inode eviction, for a good and simple reason - there is no backing store at all. And as for the icache retention, debugfs inodes are * never looked up in icache * never hashed * ... and thus never retained in icache past the final iput() i_nlink serves as a refcount - for on-disk inodes on filesystems that allow hardlinks and need to decide if the on-disk inode needs to follow an in-core one into oblivion. The lifetime of in-core inode is *NOT* controlled by i_nlink. They can very well outlive i_nlink dropping to 0, for starters. Consider e.g. the following: cat >/tmp/a.sh <<'EOF' echo still not freed >/tmp/a (sleep 5 && date && stat - && cat) st_nlink and that came straight from ->i_nlink of the (very much alive) in-core inode. And of course, in-core inodes do get freed just fine without i_nlink reaching zero. It's used for 4 things: 1) deciding whether it makes sense to evict in-core inode as soon as we have no more (in-core) references pinning them (i.e. when ->i_count reaches zero). If there's a chance that somebody will do an icache lookup finding that one, we might want to keep it around until memory pressure kicks it out. And since for something like normal Unix filesystem such icache lookup can happen as long as there are links to the (on-disk) inode left in some directories, default policy is "try to keep it around if i_nlink is positive *AND* it is reachable from icache in the first place". Filesystems might override that, but it's all moot if the in-core inode is *not* reachable from icache in the first place. Which is the case for debugfs and similar beasts. 2) deciding whether we want to free the on-disk inode when an in-core one gets evicted. Note that such freeing can not happen as long as in-core inode is around - unlinking an open file does *not* free it; it's still open and IO on such descriptor works just fine. There the normal rules are "if we are evicting an in-core inode and we know that it has no links left, it's time to free the on-disk counterpart". Up to individual filesystems, not applicable to debugfs for obvious reasons. 3) for the same filesystems, if the link count is maintained in on-disk inode we'll need to update it on unlink et.al. ->i_nlink of in-core inode is handy for keeping track of that. Again, not applicable in debugfs 4) reporting st_nlink to userland on stat/fstat/etc. That *is* applicable in debugfs and, in fact, it is the only use of ->i_nlink there.
[PATCH] debugfs: remove inc_nlink in debugfs_create_automount
Remove inc_nlink in debugfs_create_automount, or this inode will never be free. Signed-off-by: yangerkun --- fs/debugfs/inode.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index 13b01351dd1c..9294238e364f 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -551,12 +551,11 @@ struct dentry *debugfs_create_automount(const char *name, if (unlikely(!inode)) return failed_creating(dentry); + /* directory inodes start off with i_nlink == 2 (for "." entry) */ make_empty_dir_inode(inode); inode->i_flags |= S_AUTOMOUNT; inode->i_private = data; dentry->d_fsdata = (void *)f; - /* directory inodes start off with i_nlink == 2 (for "." entry) */ - inc_nlink(inode); d_instantiate(dentry, inode); inc_nlink(d_inode(dentry->d_parent)); fsnotify_mkdir(d_inode(dentry->d_parent), dentry); -- 2.14.4
Re: [PATCH] debugfs: remove inc_nlink in debugfs_create_automount
On Sat, Dec 22, 2018 at 04:45:36PM +0800, yangerkun wrote: > Remove inc_nlink in debugfs_create_automount, or this inode will never > be free. > > Signed-off-by: yangerkun > --- > fs/debugfs/inode.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > index 13b01351dd1c..9294238e364f 100644 > --- a/fs/debugfs/inode.c > +++ b/fs/debugfs/inode.c > @@ -551,12 +551,11 @@ struct dentry *debugfs_create_automount(const char > *name, > if (unlikely(!inode)) > return failed_creating(dentry); > > + /* directory inodes start off with i_nlink == 2 (for "." entry) */ > make_empty_dir_inode(inode); > inode->i_flags |= S_AUTOMOUNT; > inode->i_private = data; > dentry->d_fsdata = (void *)f; > - /* directory inodes start off with i_nlink == 2 (for "." entry) */ > - inc_nlink(inode); Again, have you tested this and how? How did you find this issue? thanks, greg k-h
