Re: [PATCH v1 12/17] xfs: remove parent pointers in unlink

From: "Darrick J. Wong" <djwong@kernel.org>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v1 12/17] xfs: remove parent pointers in unlink
Date: Wed, 29 Jun 2022 10:35:45 -0700	[thread overview]
Message-ID: <YryNcV4uiLNHPzyp@magnolia> (raw)
In-Reply-To: <20220611094200.129502-13-allison.henderson@oracle.com>

On Sat, Jun 11, 2022 at 02:41:55AM -0700, Allison Henderson wrote:
> This patch removes the parent pointer attribute during unlink
> 
> [bfoster: rebase, use VFS inode generation]
> [achender: rebased, changed __unint32_t to xfs_dir2_dataptr_t
>            implemented xfs_attr_remove_parent]
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c |  2 +-
>  fs/xfs/libxfs/xfs_attr.h |  1 +
>  fs/xfs/xfs_inode.c       | 63 +++++++++++++++++++++++++++++++---------
>  3 files changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index f814a9177237..b86188b63897 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -966,7 +966,7 @@ xfs_attr_defer_replace(
>  }
>  
>  /* Removes an attribute for an inode as a deferred operation */
> -static int
> +int
>  xfs_attr_defer_remove(
>  	struct xfs_da_args	*args)
>  {
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 576062e37d11..386dfc8d6053 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -560,6 +560,7 @@ bool xfs_attr_is_leaf(struct xfs_inode *ip);
>  int xfs_attr_get_ilocked(struct xfs_da_args *args);
>  int xfs_attr_get(struct xfs_da_args *args);
>  int xfs_attr_defer_add(struct xfs_da_args *args);
> +int xfs_attr_defer_remove(struct xfs_da_args *args);
>  int xfs_attr_set(struct xfs_da_args *args);
>  int xfs_attr_set_iter(struct xfs_attr_intent *attr);
>  int xfs_attr_remove_iter(struct xfs_attr_intent *attr);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 41c58df8e568..160f57df6d58 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2828,16 +2828,27 @@ xfs_iunpin_wait(
>   */
>  int
>  xfs_remove(
> -	xfs_inode_t             *dp,
> -	struct xfs_name		*name,
> -	xfs_inode_t		*ip)
> -{
> -	xfs_mount_t		*mp = dp->i_mount;
> -	xfs_trans_t             *tp = NULL;
> -	int			is_dir = S_ISDIR(VFS_I(ip)->i_mode);
> -	int			dontcare;
> -	int                     error = 0;
> -	uint			resblks;
> +	xfs_inode_t             	*dp,

Please convert these to 'struct xfs_inode	*dp' and fix the
whitespace between the type name and the variable.

> +	struct xfs_name			*name,
> +	xfs_inode_t			*ip)
> +{
> +	xfs_mount_t			*mp = dp->i_mount;
> +	xfs_trans_t             	*tp = NULL;
> +	int				is_dir = S_ISDIR(VFS_I(ip)->i_mode);
> +	int				dontcare;
> +	int                     	error = 0;
> +	uint				resblks;
> +	xfs_dir2_dataptr_t		dir_offset;
> +	struct xfs_parent_name_rec	rec;
> +	struct xfs_da_args		args = {
> +		.dp		= ip,
> +		.geo		= mp->m_attr_geo,
> +		.whichfork	= XFS_ATTR_FORK,
> +		.attr_filter	= XFS_ATTR_PARENT,
> +		.op_flags	= XFS_DA_OP_OKNOENT,
> +		.name		= (const uint8_t *)&rec,
> +		.namelen	= sizeof(rec),
> +	};
>  
>  	trace_xfs_remove(dp, name);
>  
> @@ -2852,6 +2863,12 @@ xfs_remove(
>  	if (error)
>  		goto std_return;
>  
> +	if (xfs_has_larp(mp)) {
> +		error = xfs_attr_grab_log_assist(mp);
> +		if (error)
> +			goto std_return;
> +	}
> +
>  	/*
>  	 * We try to get the real space reservation first, allowing for
>  	 * directory btree deletion(s) implying possible bmap insert(s).  If we
> @@ -2865,10 +2882,10 @@ xfs_remove(
>  	 */
>  	resblks = XFS_REMOVE_SPACE_RES(mp);
>  	error = xfs_trans_alloc_dir(dp, &M_RES(mp)->tr_remove, ip, &resblks,
> -			&tp, &dontcare, XFS_ILOCK_EXCL);
> +			&tp, &dontcare, 0);
>  	if (error) {
>  		ASSERT(error != -ENOSPC);
> -		goto std_return;
> +		goto drop_incompat;
>  	}
>  
>  	/*
> @@ -2922,12 +2939,22 @@ xfs_remove(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	error = xfs_dir_removename(tp, dp, name, ip->i_ino, resblks, NULL);
> +	error = xfs_dir_removename(tp, dp, name, ip->i_ino, resblks, &dir_offset);
>  	if (error) {
>  		ASSERT(error != -ENOENT);
>  		goto out_trans_cancel;
>  	}
>  
> +	if (xfs_sb_version_hasparent(&mp->m_sb)) {
> +		xfs_init_parent_name_rec(&rec, dp, dir_offset);
> +		args.hashval = xfs_da_hashname(args.name, args.namelen);
> +		args.trans = tp;
> +
> +		error = xfs_attr_defer_remove(&args);
> +		if (error)
> +			goto out_trans_cancel;

Why not queue the pptr removal inside xfs_dir_removename?  Is there ever
going to be a case where we want to remove a dirent but not update the
parent pointers?

OH.  Right.  Online repair makes this messy, since it builds all the new
directory structures in an O_TMPFILE directory, without bumping the link
counts of any of the children or changing dotdot entries of child dirs.
Then the contents are swapext'd.

So yes, it's perhaps best to keep those separate....?

--D

> +	}
> +
>  	/*
>  	 * If this is a synchronous mount, make sure that the
>  	 * remove transaction goes to disk before returning to
> @@ -2938,15 +2965,23 @@ xfs_remove(
>  
>  	error = xfs_trans_commit(tp);
>  	if (error)
> -		goto std_return;
> +		goto out_unlock;
>  
>  	if (is_dir && xfs_inode_is_filestream(ip))
>  		xfs_filestream_deassociate(ip);
>  
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	xfs_iunlock(dp, XFS_ILOCK_EXCL);
>  	return 0;
>  
>   out_trans_cancel:
>  	xfs_trans_cancel(tp);
> + out_unlock:
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> + drop_incompat:
> +	if (xfs_has_larp(mp))
> +		xlog_drop_incompat_feat(mp->m_log);
>   std_return:
>  	return error;
>  }
> -- 
> 2.25.1
> 