Re: [PATCH RFC 6/8] xfs: fix imbalanced locking

[Shan Hai <shan.hai@oracle.com>]

On 2018年07月08日 23:53, Christoph Hellwig wrote:
> On Fri, Jul 06, 2018 at 11:12:27AM +0800, Shan Hai wrote:
>> The XFS_ILOCK_SHARED type lock is held in the xfs_ioc_fsgetxattr
>> but does not released when the inode is in local format, fix it by
>> eleasing the lock on the local format inode.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/xfs_ioctl.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 0ef5ece5634c..246562615ccd 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -907,7 +907,10 @@ xfs_ioc_fsgetxattr(
>>   	} else {
>>   		if (ip->i_df.if_flags & XFS_IFEXTENTS)
>>   			fa.fsx_nextents = xfs_iext_count(&ip->i_df);
>> -		else
>> +		else if (ip->i_df.if_flags & XFS_IFINLINE) {
>> +			xfs_iunlock(ip, XFS_ILOCK_SHARED);
>> +			return 0;
>> +		} else
>>   			fa.fsx_nextents = ip->i_d.di_nextents;
>>   	}
>>   	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> This looks odd, and the changelog confuses even more.
>
> Please avoid the early return, which means we miss the copy_to_user
> of the fa struct.  It seems to me like this should be something like:
>
> 		if (ip->i_df.if_flags & XFS_IFEXTENTS)
>                          fa.fsx_nextents = xfs_iext_count(&ip->i_df);
> 		esle if (ip->i_df.if_flags & XFS_IFINLINE)
> 			fa.fsx_nextents = 0;
>                  else
> 			fa.fsx_nextents = ip->i_d.di_nextents;


Agreed, I will double check this, thanks for the review.

Thanks
Shan Hai