Re: [PATCH] vfs: don't decrement i_nlink in d_tmpfile

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Feb 15, 2019 at 4:23 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> 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.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  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(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5c349667c761..bd189fc50f83 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -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);
> diff --git a/fs/dcache.c b/fs/dcache.c
> index aac41adf4743..5fb4ecce2589 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -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);

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.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/