Sorry to go through such a lengthy discussion, but I think a clear understanding
of this is important. I promise to put the conclusions we get into
Documentation/filesystems/vfs.txt.
> By an "active hard link" I mean that the dentry for hard link path
> exists, and therefore has incremented i_count. The i_nlink value
> most certainly is "related" to i_count, as it defines the upper
> bound for i_count for a file in a well-behaved filesystem.
No it is not, or ext2 is not a well-behaved filesystem. The sequence
link("foo","bar");
open("foo");
open("bar");
unlink("foo");
will produce an inode with i_count>i_nlink.
i_nlink counts the on-disk references, whereas i_count counts the in-memory
references. When it comes to cleaning up in-memory resources, i_nlink should
not matter. If you tell me I have to do this in delete_inode, i_nlink does
matter.
> Of course the file isn't removed from the disk. The delete_inode call is used to
> remove the in-memory inode representation when it's no longer needed, but the
> name is somewhat of a misnomer.
No, the name very well describes of what the operation should do, and
indeed does for all local file systems I've been looking at (which
also support deleting file systems in the first place).
Please note that affs has the very same problem: it releases resources in
put_inode. I guess you can construct bad scenarios here as well. They are
typically exposed by doing 'cat' to a file repeatedly.
> A file that has been unlinked is just one case
> in which the in-memory inode isn't needed any more.
Wrong. In the above example, the in-memory inode will be needed even
after the unlink. Opening, then unlinking a file is a common Unix idiom
for temporary files. All local file systems handle it well, AFAICT.
Network file systems are somewhat different, though. NFS does silly renames.
How would smbfs behave if you unlink a file that is still open?
> A file system may choose to defer operations relating to removing the on-disk
> representation to the delete_inode operation; it depends on the fs.
This is not a case of defering the delete: ext2_delete_inode is the *earliest*
point where the on-disk inode can be deallocated.
> No, delete_inode will work fine. Try it.
Well, I decided to extend the VFS instead. All discussions aside, can you
please review this patch? At least, I could not reproduce the Oops with it
anymore.
Everybody testing this patch: you'll have to recompile the entire kernel, not
just the NTFS code.
Thanks,
Martin
diff -ur linux-2.1.84.0/fs/inode.c linux/fs/inode.c
--- linux-2.1.84.0/fs/inode.c Sun Feb 1 09:01:24 1998
+++ linux/fs/inode.c Sat Jan 31 23:53:19 1998
@@ -225,6 +225,8 @@
wait_on_inode(inode);
if (IS_WRITABLE(inode) && inode->i_sb && inode->i_sb->dq_op)
inode->i_sb->dq_op->drop(inode);
+ if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->clear_inode)
+ inode->i_sb->s_op->clear_inode(inode);
inode->i_state = 0;
}
diff -ur linux-2.1.84.0/fs/ntfs/fs.c linux/fs/ntfs/fs.c
--- linux-2.1.84.0/fs/ntfs/fs.c Sun Feb 1 09:00:57 1998
+++ linux/fs/ntfs/fs.c Sun Feb 1 08:48:17 1998
@@ -668,7 +668,11 @@
static void ntfs_put_inode(struct inode *ino)
{
- ntfs_debug(DEBUG_OTHER, "ntfs_put_inode %lx\n",ino->i_ino);
+}
+
+static int _ntfs_clear_inode(struct inode *ino)
+{
+ ntfs_debug(DEBUG_OTHER, "ntfs_clear_inode %lx\n",ino->i_ino);
#ifdef NTFS_IN_LINUX_KERNEL
if(ino->i_ino!=FILE_MFT)
ntfs_clear_inode(&ino->u.ntfs_i);
@@ -680,7 +684,7 @@
ino->u.generic_ip=0;
}
#endif
- clear_inode(ino);
+ return 0;
}
/* Called when umounting a filesystem by do_umount() in fs/super.c */
@@ -753,6 +757,7 @@
NULL, /* write_super */
ntfs_statfs,
ntfs_remount_fs, /* remount */
+ _ntfs_clear_inode, /* clear_inode */
};
/* Called to mount a filesystem by read_super() in fs/super.c
diff -ur linux-2.1.84.0/fs/ntfs/inode.c linux/fs/ntfs/inode.c
--- linux-2.1.84.0/fs/ntfs/inode.c Fri Jan 2 10:42:59 1998
+++ linux/fs/ntfs/inode.c Sat Jan 31 23:48:24 1998
@@ -289,8 +289,14 @@
void ntfs_clear_inode(ntfs_inode *ino)
{
int i;
+ if(!ino->attr){
+ ntfs_error("ntfs_clear_inode: double free\n");
+ return;
+ }
ntfs_free(ino->attr);
+ ino->attr=0;
ntfs_free(ino->records);
+ ino->records=0;
for(i=0;i<ino->attr_count;i++)
{
if(ino->attrs[i].name)
@@ -305,6 +311,7 @@
}
}
ntfs_free(ino->attrs);
+ ino->attrs=0;
}
/* Check and fixup a MFT record */
diff -ur linux-2.1.84.0/include/linux/fs.h linux/include/linux/fs.h
--- linux-2.1.84.0/include/linux/fs.h Sun Feb 1 09:01:24 1998
+++ linux/include/linux/fs.h Sat Jan 31 23:50:58 1998
@@ -612,6 +612,7 @@
void (*write_super) (struct super_block *);
int (*statfs) (struct super_block *, struct statfs *, int);
int (*remount_fs) (struct super_block *, int *, char *);
+ int (*clear_inode) (struct inode *);
};
struct dquot_operations {