[PATCH] xfs: fix double unlock in xfs_swap_extents()

[PATCH] xfs: fix double unlock in xfs_swap_extents()

[Felix Blyakher]

Regreesion from commit ef8f7fc, which rearranged the code in
xfs_swap_extents() leading to double unlock of xfs inode iolock.
That resulted in xfs_fsr deadlocking itself on platforms, which
don't handle double unlock of rw_semaphore nicely. It caused the
count go negative, which represents the write holder, without
really having one. ia64 is one of the platforms where deadlock
was easily reproduced and the fix was tested.

Signed-off-by: Felix Blyakher <felixb-sJ/iWh9BUns@public.gmane.org>
---
 fs/xfs/xfs_dfrag.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
index e6d839b..7465f9e 100644
--- a/fs/xfs/xfs_dfrag.c
+++ b/fs/xfs/xfs_dfrag.c
@@ -347,13 +347,15 @@ xfs_swap_extents(
 
 	error = xfs_trans_commit(tp, XFS_TRANS_SWAPEXT);
 
-out_unlock:
-	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
-	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
 out:
 	kmem_free(tempifp);
 	return error;
 
+out_unlock:
+	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
+	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
+	goto out;
+
 out_trans_cancel:
 	xfs_trans_cancel(tp, 0);
 	goto out_unlock;
-- 
1.5.4.rc3

Re: [PATCH] xfs: fix double unlock in xfs_swap_extents()

[Eric Sandeen]

Felix Blyakher wrote:
> Regreesion from commit ef8f7fc, which rearranged the code in
> xfs_swap_extents() leading to double unlock of xfs inode iolock.
> That resulted in xfs_fsr deadlocking itself on platforms, which
> don't handle double unlock of rw_semaphore nicely. It caused the
> count go negative, which represents the write holder, without
> really having one. ia64 is one of the platforms where deadlock
> was easily reproduced and the fix was tested.
> 
> Signed-off-by: Felix Blyakher <felixb-sJ/iWh9BUns@public.gmane.org>

Also-written-by: Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org>
Independently-arrived-at-by: Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org>

;)

But seriously ...

Reviewed-by: Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org>

> ---
>  fs/xfs/xfs_dfrag.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
> index e6d839b..7465f9e 100644
> --- a/fs/xfs/xfs_dfrag.c
> +++ b/fs/xfs/xfs_dfrag.c
> @@ -347,13 +347,15 @@ xfs_swap_extents(
>  
>  	error = xfs_trans_commit(tp, XFS_TRANS_SWAPEXT);
>  
> -out_unlock:
> -	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> -	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
>  out:
>  	kmem_free(tempifp);
>  	return error;
>  
> +out_unlock:
> +	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> +	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> +	goto out;
> +
>  out_trans_cancel:
>  	xfs_trans_cancel(tp, 0);
>  	goto out_unlock;

Re: [PATCH] xfs: fix double unlock in xfs_swap_extents()

[Felix Blyakher]

On May 7, 2009, at 9:57 PM, Eric Sandeen wrote:

> Felix Blyakher wrote:
>> Regreesion from commit ef8f7fc, which rearranged the code in
>> xfs_swap_extents() leading to double unlock of xfs inode iolock.
>> That resulted in xfs_fsr deadlocking itself on platforms, which
>> don't handle double unlock of rw_semaphore nicely. It caused the
>> count go negative, which represents the write holder, without
>> really having one. ia64 is one of the platforms where deadlock
>> was easily reproduced and the fix was tested.
>>
>> Signed-off-by: Felix Blyakher <felixb-sJ/iWh9BUns@public.gmane.org>
>
> Also-written-by: Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org>
> Independently-arrived-at-by: Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org>

That would make it: Signed-off-by: Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org> :)

> ;)
>
> But seriously ...

Seriously.

Felix

>
>
> Reviewed-by: Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org>
>
>> ---
>> fs/xfs/xfs_dfrag.c |    8 +++++---
>> 1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
>> index e6d839b..7465f9e 100644
>> --- a/fs/xfs/xfs_dfrag.c
>> +++ b/fs/xfs/xfs_dfrag.c
>> @@ -347,13 +347,15 @@ xfs_swap_extents(
>>
>> 	error = xfs_trans_commit(tp, XFS_TRANS_SWAPEXT);
>>
>> -out_unlock:
>> -	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
>> -	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
>> out:
>> 	kmem_free(tempifp);
>> 	return error;
>>
>> +out_unlock:
>> +	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
>> +	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
>> +	goto out;
>> +
>> out_trans_cancel:
>> 	xfs_trans_cancel(tp, 0);
>> 	goto out_unlock;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux- 
> kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] xfs: fix double unlock in xfs_swap_extents()

[Christoph Hellwig]

The patch looks good, but a little nitpick:

On Thu, May 07, 2009 at 08:13:22PM -0500, Felix Blyakher wrote:
>  out:
>  	kmem_free(tempifp);
>  	return error;
>  
> +out_unlock:
> +	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> +	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> +	goto out;
> +
>  out_trans_cancel:
>  	xfs_trans_cancel(tp, 0);
>  	goto out_unlock;

this would be more readable as:

 out_trans_cancel:
	xfs_trans_cancel(tp, 0);
 out_unlock:
	xfs_iunlock(ip,  XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
	xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
	goto out;