Re: [PATCH v3] xfs: Fix deadlock between AGI and AGF when target_ip exists in xfs_rename()

[kaixuxia <xiakaixu1987@gmail.com>]

On 2019/11/11 21:22, Brian Foster wrote:
> On Thu, Nov 07, 2019 at 08:08:11PM +0800, kaixuxia wrote:
>> When target_ip exists in xfs_rename(), the xfs_dir_replace() call may
>> need to hold the AGF lock to allocate more blocks, and then invoking
>> the xfs_droplink() call to hold AGI lock to drop target_ip onto the
>> unlinked list, so we get the lock order AGF->AGI. This would break the
>> ordering constraint on AGI and AGF locking - inode allocation locks
>> the AGI, then can allocate a new extent for new inodes, locking the
>> AGF after the AGI.
>>
>> In this patch we check whether the replace operation need more
>> blocks firstly. If so, acquire the agi lock firstly to preserve
>> locking order(AGI/AGF). Actually, the locking order problem only
>> occurs when we are locking the AGI/AGF of the same AG. For multiple
>> AGs the AGI lock will be released after the transaction committed.
>>
>> Signed-off-by: kaixuxia <kaixuxia@tencent.com>
>> ---
>> Changes in v3:
>>  -Invoke xfs_dir2_sf_replace_needblock() call in xfs_inode.c
>>   directly.
>>  -Fix the typo.
>>
>>  fs/xfs/libxfs/xfs_dir2.h    |  2 ++
>>  fs/xfs/libxfs/xfs_dir2_sf.c | 21 +++++++++++++++++++++
>>  fs/xfs/xfs_inode.c          | 15 +++++++++++++++
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
>> index f542447..d4a2b09 100644
>> --- a/fs/xfs/libxfs/xfs_dir2.h
>> +++ b/fs/xfs/libxfs/xfs_dir2.h
>> @@ -124,6 +124,8 @@ extern int xfs_dir_lookup(struct xfs_trans *tp, struct xfs_inode *dp,
>>  extern int xfs_dir_removename(struct xfs_trans *tp, struct xfs_inode *dp,
>>  				struct xfs_name *name, xfs_ino_t ino,
>>  				xfs_extlen_t tot);
>> +extern bool xfs_dir2_sf_replace_needblock(struct xfs_inode *dp,
>> +				struct xfs_inode *src_ip);
>>  extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp,
>>  				struct xfs_name *name, xfs_ino_t inum,
>>  				xfs_extlen_t tot);
>> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
>> index 85f14fc..7098cdd 100644
>> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
>> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
>> @@ -945,6 +945,27 @@ static int xfs_dir2_sf_addname_pick(xfs_da_args_t *args, int objchange,
>>  }
>>  
>>  /*
>> + * Check whether the sf dir replace operation need more blocks.
>> + */
>> +bool
>> +xfs_dir2_sf_replace_needblock(
>> +	struct xfs_inode	*dp,
>> +	struct xfs_inode	*src_ip)
>> +{
>> +	int			newsize;
>> +	xfs_dir2_sf_hdr_t	*sfp;
> 
> We shouldn't introduce new typedef usages. Please use struct
> xfs_dir2_sf_hdr here and throughout the patch.

OKay, will fix it in the next version.
> 
>> +
>> +	if (dp->i_d.di_format != XFS_DINODE_FMT_LOCAL)
>> +		return false;
>> +
>> +	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
>> +	newsize = dp->i_df.if_bytes + (sfp->count + 1) * XFS_INO64_DIFF;
>> +
>> +	return src_ip->i_ino > XFS_DIR2_MAX_SHORT_INUM &&
>> +	       sfp->i8count == 0 && newsize > XFS_IFORK_DSIZE(dp);
>> +}
>> +
> 
> The whole point of this function is to reduce code duplication. There
> should be two callers, one down in the dir code that does the format
> conversion and the new caller in the higher level code to grab the AGI.

Yeah, will add the logic.
Thanks for your comments!

Kaixu
> 
> Brian
> 
>> +/*
>>   * Replace the inode number of an entry in a shortform directory.
>>   */
>>  int						/* error */
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 18f4b26..cb0b93b 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -3196,6 +3196,7 @@ struct xfs_iunlink {
>>  	struct xfs_trans	*tp;
>>  	struct xfs_inode	*wip = NULL;		/* whiteout inode */
>>  	struct xfs_inode	*inodes[__XFS_SORT_INODES];
>> +	struct xfs_buf		*agibp;
>>  	int			num_inodes = __XFS_SORT_INODES;
>>  	bool			new_parent = (src_dp != target_dp);
>>  	bool			src_is_directory = S_ISDIR(VFS_I(src_ip)->i_mode);
>> @@ -3361,6 +3362,20 @@ struct xfs_iunlink {
>>  		 * In case there is already an entry with the same
>>  		 * name at the destination directory, remove it first.
>>  		 */
>> +
>> +		/*
>> +		 * Check whether the replace operation need more blocks.
>> +		 * If so, acquire the agi lock firstly to preserve locking
>> +		 * order (AGI/AGF). Only convert the shortform directory to
>> +		 * block form maybe need more blocks.
>> +		 */
>> +		if (xfs_dir2_sf_replace_needblock(target_dp, src_ip)) {
>> +			error = xfs_read_agi(mp, tp,
>> +				XFS_INO_TO_AGNO(mp, target_ip->i_ino), &agibp);
>> +			if (error)
>> +				goto out_trans_cancel;
>> +		}
>> +
>>  		error = xfs_dir_replace(tp, target_dp, target_name,
>>  					src_ip->i_ino, spaceres);
>>  		if (error)
>> -- 
>> 1.8.3.1
>>
> 

-- 
kaixuxia