vfs: don't decrement i_nlink in d_tmpfile
Subsystems (11)
PATCH 1/1 vfs: don't decrement i_nlink in d_tmpfile
From: Darrick J. Wong <[email protected]>
d_tmpfile was introduced to instantiate an inode in the dentry cache as a temporary file. This helper decrements the inode's nlink count and dirties the inode, presumably so that filesystems could call new_inode to create a new inode with nlink == 1 and then call d_tmpfile which will decrement nlink.
However, this doesn't play well with XFS, which needs to allocate, initialize, and insert a tempfile inode on its unlinked list in a single transaction. In order to maintain referential integrity of the XFS metadata, we cannot have an inode on the unlinked list with nlink >= 1.
XFS and btrfs hack around d_tmpfile's behavior by creating the inode with nlink == 0 and then incrementing it just prior to calling d_tmpfile, anticipating that it will be reset to 0.
Everywhere else outside of d_tmpfile, it appears that nlink updates and persistence is the responsibility of individual filesystems. Therefore, move the nlink decrement out of d_tmpfile into the callers, and require that callers only pass in inodes with nlink already set to 0.
--- fs/btrfs/inode.c | 8 -------- fs/dcache.c | 8 ++++++-- fs/ext2/namei.c | 2 +- fs/ext4/namei.c | 1 + fs/f2fs/namei.c | 1 + fs/minix/namei.c | 2 +- fs/ubifs/dir.c | 1 + fs/udf/namei.c | 2 +- fs/xfs/xfs_iops.c | 13 ++----------- mm/shmem.c | 1 + 10 files changed, 15 insertions(+), 24 deletions(-)
fs/btrfs/inode.c +0 -8
@@ -10382,14 +10382,6 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)if (ret) goto out; -/* -* We set number of links to 0 in btrfs_new_inode(), and here we set -* it to 1 because d_tmpfile() will issue a warning if the count is 0, -* through: -* -* d_tmpfile() -> inode_dec_link_count() -> drop_nlink() -*/ -set_nlink (inode,1 );d_tmpfile (dentry, inode);unlock_new_inode (inode);mark_inode_dirty (inode);
fs/dcache.c +6 -2
@@ -3042,12 +3042,16 @@ void d_genocide(struct dentry *parent)EXPORT_SYMBOL (d_genocide); +/* +* Instantiate an inode in the dentry cache as a temporary file. Callers must +* ensure that @inode has a zero link count. +*/ void d_tmpfile (struct dentry *dentry,struct inode *inode) { -inode_dec_link_count (inode);BUG_ON (dentry->d_name.name != dentry->d_iname || !hlist_unhashed (&dentry->d_u.d_alias) || - !d_unlinked (dentry)); + !d_unlinked (dentry) || + inode->i_nlink !=0 );spin_lock (&dentry->d_parent->d_lock);spin_lock_nested (&dentry->d_lock,DENTRY_D_LOCK_NESTED ); dentry->d_name.len = sprintf(dentry->d_iname, "#%llu",
fs/ext2/namei.c +1 -1
@@ -117,7 +117,7 @@ static int ext2_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)return PTR_ERR (inode);ext2_set_file_ops (inode); -mark_inode_dirty (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode);unlock_new_inode (inode);return 0 ;
fs/ext4/namei.c +1 -0
@@ -2517,6 +2517,7 @@ static int ext4_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) inode->i_op = &ext4_file_inode_operations; inode->i_fop = &ext4_file_operations;ext4_set_aops (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode); err =ext4_orphan_add (handle, inode);if (err)
fs/f2fs/namei.c +1 -0
@@ -780,6 +780,7 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,f2fs_i_links_write (inode, false); *whiteout = inode; }else { +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }/* link_count was changed by d_tmpfile as well. */
fs/minix/namei.c +1 -1
@@ -57,7 +57,7 @@ static int minix_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)struct inode *inode =minix_new_inode (dir, mode, &error);if (inode) {minix_set_inode (inode,0 ); -mark_inode_dirty (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }return error;
fs/ubifs/dir.c +1 -0
@@ -419,6 +419,7 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,drop_nlink (inode); *whiteout = inode; }else { +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }ubifs_assert (c, ui->dirty);
fs/udf/namei.c +1 -1
@@ -652,7 +652,7 @@ static int udf_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) inode->i_data.a_ops = &udf_aops; inode->i_op = &udf_file_inode_operations; inode->i_fop = &udf_file_operations; -mark_inode_dirty (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode);unlock_new_inode (inode);return 0 ;
fs/xfs/xfs_iops.c +2 -11
@@ -191,18 +191,9 @@ xfs_generic_create(xfs_setup_iops (ip); -if (tmpfile) { -/* -* The VFS requires that any inode fed to d_tmpfile must have -* nlink == 1 so that it can decrement the nlink in d_tmpfile. -* However, we created the temp file with nlink == 0 because -* we're not allowed to put an inode with nlink > 0 on the -* unlinked list. Therefore we have to set nlink to 1 so that -* d_tmpfile can immediately set it back to zero. -*/ -set_nlink (inode,1 ); +if (tmpfile)d_tmpfile (dentry, inode); - }else +else d_instantiate (dentry, inode);xfs_finish_inode_setup (ip);
mm/shmem.c +1 -0
@@ -2818,6 +2818,7 @@ shmem_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) error =simple_acl_create (dir, inode);if (error) goto out_iput; +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }return error;
Quoted 66 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
d_tmpfilewas introduced to instantiate an inode in the dentry cache as a temporary file. This helper decrements the inode's nlink count and dirties the inode, presumably so that filesystems could callnew_inodeto create a new inode with nlink == 1 and then calld_tmpfilewhich will decrement nlink.However, this doesn't play well with XFS, which needs to allocate, initialize, and insert a tempfile inode on its unlinked list in a single transaction. In order to maintain referential integrity of the XFS metadata, we cannot have an inode on the unlinked list with nlink >= 1.
XFS and btrfs hack around d_tmpfile's behavior by creating the inode with nlink == 0 and then incrementing it just prior to calling
d_tmpfile, anticipating that it will be reset to 0.Everywhere else outside of
d_tmpfile, it appears that nlink updates and persistence is the responsibility of individual filesystems. Therefore, move the nlink decrement out ofd_tmpfileinto the callers, and require that callers only pass in inodes with nlink already set to 0.Signed-off-by Darrick J. Wong <[email protected]>--- fs/btrfs/inode.c | 8 -------- fs/dcache.c | 8 ++++++-- fs/ext2/namei.c | 2 +- fs/ext4/namei.c | 1 + fs/f2fs/namei.c | 1 + fs/minix/namei.c | 2 +- fs/ubifs/dir.c | 1 + fs/udf/namei.c | 2 +- fs/xfs/xfs_iops.c | 13 ++----------- mm/shmem.c | 1 + 10 files changed, 15 insertions(+), 24 deletions(-)fs/btrfs/inode.c +0
-8@@ -10382,14 +10382,6 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)if (ret) goto out; -/* -* We set number of links to 0 in btrfs_new_inode(), and here we set -* it to 1 because d_tmpfile() will issue a warning if the count is 0, -* through: -* -* d_tmpfile() -> inode_dec_link_count() -> drop_nlink() -*/ -set_nlink (inode,1 );d_tmpfile (dentry, inode);unlock_new_inode (inode);mark_inode_dirty (inode);fs/dcache.c +2
-0@@ -3042,12 +3042,16 @@ void d_genocide(struct dentry *parent)EXPORT_SYMBOL (d_genocide); +/* +* Instantiate an inode in the dentry cache as a temporary file. Callers must
fs/dcache.c void d_genocide(struct dentry *parent) · line 3054
Quoted 10 lines
fs/dcache.c +4
-2+* ensure that @inode has a zero link count. +*/ void d_tmpfile (struct dentry *dentry,struct inode *inode) { -inode_dec_link_count (inode);BUG_ON (dentry->d_name.name != dentry->d_iname || !hlist_unhashed (&dentry->d_u.d_alias) || - !d_unlinked (dentry)); + !d_unlinked (dentry) || + inode->i_nlink !=0 );
You've just promoted i_nlink filesystem accounting error (which are not that rare) from WARN_ON() to BUG_ON(), not to mention Linus' objection to any use of BUG_ON() at all.
!hlist_unhashed is anyway checked again in d_instantiate(). !d_unlinked is not a reason to break the machine. The name check is really not a reason to break the machine. Can probably make tmp name code conditional to WARN_ON().
Thanks,
Amir.
Quoted 82 lines · Amir Goldstein
Quoted 66 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
d_tmpfilewas introduced to instantiate an inode in the dentry cache as a temporary file. This helper decrements the inode's nlink count and dirties the inode, presumably so that filesystems could callnew_inodeto create a new inode with nlink == 1 and then calld_tmpfilewhich will decrement nlink.However, this doesn't play well with XFS, which needs to allocate, initialize, and insert a tempfile inode on its unlinked list in a single transaction. In order to maintain referential integrity of the XFS metadata, we cannot have an inode on the unlinked list with nlink >= 1.
XFS and btrfs hack around d_tmpfile's behavior by creating the inode with nlink == 0 and then incrementing it just prior to calling
d_tmpfile, anticipating that it will be reset to 0.Everywhere else outside of
d_tmpfile, it appears that nlink updates and persistence is the responsibility of individual filesystems. Therefore, move the nlink decrement out ofd_tmpfileinto the callers, and require that callers only pass in inodes with nlink already set to 0.Signed-off-by Darrick J. Wong <[email protected]>--- fs/btrfs/inode.c | 8 -------- fs/dcache.c | 8 ++++++-- fs/ext2/namei.c | 2 +- fs/ext4/namei.c | 1 + fs/f2fs/namei.c | 1 + fs/minix/namei.c | 2 +- fs/ubifs/dir.c | 1 + fs/udf/namei.c | 2 +- fs/xfs/xfs_iops.c | 13 ++----------- mm/shmem.c | 1 + 10 files changed, 15 insertions(+), 24 deletions(-)fs/btrfs/inode.c +0
-8@@ -10382,14 +10382,6 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)if (ret) goto out; -/* -* We set number of links to 0 in btrfs_new_inode(), and here we set -* it to 1 because d_tmpfile() will issue a warning if the count is 0, -* through: -* -* d_tmpfile() -> inode_dec_link_count() -> drop_nlink() -*/ -set_nlink (inode,1 );d_tmpfile (dentry, inode);unlock_new_inode (inode);mark_inode_dirty (inode);fs/dcache.c +2
-0@@ -3042,12 +3042,16 @@ void d_genocide(struct dentry *parent)EXPORT_SYMBOL (d_genocide); +/* +* Instantiate an inode in the dentry cache as a temporary file. Callers must fs/dcache.c void d_genocide(struct dentry *parent) · line 3054
Quoted 10 lines
fs/dcache.c +4
-2+* ensure that @inode has a zero link count. +*/ void d_tmpfile (struct dentry *dentry,struct inode *inode) { -inode_dec_link_count (inode);BUG_ON (dentry->d_name.name != dentry->d_iname || !hlist_unhashed (&dentry->d_u.d_alias) || - !d_unlinked (dentry)); + !d_unlinked (dentry) || + inode->i_nlink !=0 );You've just promoted
i_nlinkfilesystem accounting error (which are not that rare) fromWARN_ON()toBUG_ON(), not to mention Linus' objection to any use ofBUG_ON()at all.
Quoted 4 lines
!hlist_unhashed is anyway checked again in
d_instantiate(). !d_unlinked is not a reason to break the machine. The name check is really not a reason to break the machine. Can probably make tmp name code conditional toWARN_ON().
Fair enough, I'll remove the redundant checks and downgrade that to a WARN_ON, if nobody else objects....
--D
Quoted 2 lines
Thanks,
Amir.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
PATCH v2 1/1 vfs: don't decrement i_nlink in d_tmpfile
In reply to Darrick J. WongFrom: Darrick J. Wong <[email protected]>
d_tmpfile was introduced to instantiate an inode in the dentry cache as a temporary file. This helper decrements the inode's nlink count and dirties the inode, presumably so that filesystems could call new_inode to create a new inode with nlink == 1 and then call d_tmpfile which will decrement nlink.
However, this doesn't play well with XFS, which needs to allocate, initialize, and insert a tempfile inode on its unlinked list in a single transaction. In order to maintain referential integrity of the XFS metadata, we cannot have an inode on the unlinked list with nlink >= 1.
XFS and btrfs hack around d_tmpfile's behavior by creating the inode with nlink == 0 and then incrementing it just prior to calling d_tmpfile, anticipating that it will be reset to 0.
Everywhere else, it appears that nlink updates and persistence is the responsibility of individual filesystems. Therefore, move the nlink decrement out of d_tmpfile into the callers, and require that callers only pass in inodes with nlink already set to 0.
---
v2: convert BUG_ON to WARN_ON per review comment
--- fs/btrfs/inode.c | 8 -------- fs/dcache.c | 10 ++++++---- fs/ext2/namei.c | 2 +- fs/ext4/namei.c | 1 + fs/f2fs/namei.c | 1 + fs/minix/namei.c | 2 +- fs/ubifs/dir.c | 1 + fs/udf/namei.c | 2 +- fs/xfs/xfs_iops.c | 13 ++----------- mm/shmem.c | 1 + 10 files changed, 15 insertions(+), 26 deletions(-)
fs/btrfs/inode.c +0 -8
@@ -10382,14 +10382,6 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)if (ret) goto out; -/* -* We set number of links to 0 in btrfs_new_inode(), and here we set -* it to 1 because d_tmpfile() will issue a warning if the count is 0, -* through: -* -* d_tmpfile() -> inode_dec_link_count() -> drop_nlink() -*/ -set_nlink (inode,1 );d_tmpfile (dentry, inode);unlock_new_inode (inode);mark_inode_dirty (inode);
fs/dcache.c +6 -4
@@ -3042,12 +3042,14 @@ void d_genocide(struct dentry *parent)EXPORT_SYMBOL (d_genocide); +/* +* Instantiate an inode in the dentry cache as a temporary file. Callers must +* ensure that @inode has a zero link count. +*/ void d_tmpfile (struct dentry *dentry,struct inode *inode) { -inode_dec_link_count (inode); -BUG_ON (dentry->d_name.name != dentry->d_iname || - !hlist_unhashed (&dentry->d_u.d_alias) || - !d_unlinked (dentry)); +WARN_ON (dentry->d_name.name != dentry->d_iname || + !d_unlinked (dentry) || inode->i_nlink !=0 );spin_lock (&dentry->d_parent->d_lock);spin_lock_nested (&dentry->d_lock,DENTRY_D_LOCK_NESTED ); dentry->d_name.len = sprintf(dentry->d_iname, "#%llu",
fs/ext2/namei.c +1 -1
@@ -117,7 +117,7 @@ static int ext2_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)return PTR_ERR (inode);ext2_set_file_ops (inode); -mark_inode_dirty (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode);unlock_new_inode (inode);return 0 ;
fs/ext4/namei.c +1 -0
@@ -2517,6 +2517,7 @@ static int ext4_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) inode->i_op = &ext4_file_inode_operations; inode->i_fop = &ext4_file_operations;ext4_set_aops (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode); err =ext4_orphan_add (handle, inode);if (err)
fs/f2fs/namei.c +1 -0
@@ -780,6 +780,7 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,f2fs_i_links_write (inode, false); *whiteout = inode; }else { +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }/* link_count was changed by d_tmpfile as well. */
fs/minix/namei.c +1 -1
@@ -57,7 +57,7 @@ static int minix_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)struct inode *inode =minix_new_inode (dir, mode, &error);if (inode) {minix_set_inode (inode,0 ); -mark_inode_dirty (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }return error;
fs/ubifs/dir.c +1 -0
@@ -419,6 +419,7 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,drop_nlink (inode); *whiteout = inode; }else { +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }ubifs_assert (c, ui->dirty);
fs/udf/namei.c +1 -1
@@ -652,7 +652,7 @@ static int udf_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) inode->i_data.a_ops = &udf_aops; inode->i_op = &udf_file_inode_operations; inode->i_fop = &udf_file_operations; -mark_inode_dirty (inode); +inode_dec_link_count (inode);d_tmpfile (dentry, inode);unlock_new_inode (inode);return 0 ;
fs/xfs/xfs_iops.c +2 -11
@@ -191,18 +191,9 @@ xfs_generic_create(xfs_setup_iops (ip); -if (tmpfile) { -/* -* The VFS requires that any inode fed to d_tmpfile must have -* nlink == 1 so that it can decrement the nlink in d_tmpfile. -* However, we created the temp file with nlink == 0 because -* we're not allowed to put an inode with nlink > 0 on the -* unlinked list. Therefore we have to set nlink to 1 so that -* d_tmpfile can immediately set it back to zero. -*/ -set_nlink (inode,1 ); +if (tmpfile)d_tmpfile (dentry, inode); - }else +else d_instantiate (dentry, inode);xfs_finish_inode_setup (ip);
mm/shmem.c +1 -0
@@ -2818,6 +2818,7 @@ shmem_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) error =simple_acl_create (dir, inode);if (error) goto out_iput; +inode_dec_link_count (inode);d_tmpfile (dentry, inode); }return error;
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Al Viro
maintainerQuoted 13 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
d_tmpfilewas introduced to instantiate an inode in the dentry cache as a temporary file. This helper decrements the inode's nlink count and dirties the inode, presumably so that filesystems could callnew_inodeto create a new inode with nlink == 1 and then calld_tmpfilewhich will decrement nlink.However, this doesn't play well with XFS, which needs to allocate, initialize, and insert a tempfile inode on its unlinked list in a single transaction. In order to maintain referential integrity of the XFS metadata, we cannot have an inode on the unlinked list with nlink >= 1.
Quoted 8 lines
XFS and btrfs hack around d_tmpfile's behavior by creating the inode with nlink == 0 and then incrementing it just prior to calling
d_tmpfile, anticipating that it will be reset to 0.Everywhere else, it appears that nlink updates and persistence is the responsibility of individual filesystems. Therefore, move the nlink decrement out of
d_tmpfileinto the callers, and require that callers only pass in inodes with nlink already set to 0.
NAK. You are changing semantics of existing helper, requiring to add boilerplate to existing users. With zero indication that such need has appeared - no warnings, etc.
If you need a variant that wouldn't do nlink decrement, just add it and turn the existing one into a wrapper. Yield smaller patch, at that...