Re: [PATCH 4/9] fsfreeze: emergency thaw will deadlock on s_umount

[Fernando Luis Vazquez Cao <fernando_b1@lab.ntt.co.jp>]

On 2012/10/09 17:20, Jan Kara wrote:
> On Tue 09-10-12 14:07:52, Fernando Luis Vazquez Cao wrote:
>>>> diff -urNp linux-3.6.0-rc7-orig/fs/buffer.c linux-3.6.0-rc7/fs/buffer.c
>>>> --- linux-3.6.0-rc7-orig/fs/buffer.c	2012-09-26 13:20:14.842365056 +0900
>>>> +++ linux-3.6.0-rc7/fs/buffer.c	2012-09-26 15:02:22.630595704 +0900
> ...
>>> What I would put here is:
>>> 	if (!res) {
>>> 		deactivate_locked_super(sb);
>>> 		/*
>>> 		 * We have to re-acquire s_umount because
>>> 		 * iterate_supers_write() will unlock it. It still holds
>>> 		 * passive reference so sb cannot be freed under us.
>>> 		 */
>>> 		down_write(&sb->s_umount);
>>> 	}
>>> 	
>>> Is there any problem with this I miss?
>> The reason  I wrote the code as I did is that I did not want to re-acquire
>> s_umount in the normal case (s_active >= 2 entering the if statement).
>>
>> What about combining our approaches and doing something like this?:
>>
>> if (!res && !atomic_add_unless(&sb->s_active, -1, 1)) {
>>       deactivate_locked_super(sb);
>>       down_write(&sb->s_umount);
>> }
>    Well, we are speaking about emergency thaw code. That's no performance
> critical path by any means so I'd trade code simplicity for speed as much
> as possible. atomic_add_unless() makes you think twice what the hell we are
> doing here so I'd avoid it...

I left (and documented) atomic_add_unless() in the new iteration I 
cooked locally,
but I can get rid of it if you feel strongly about it.

I really appreciate you in-depth comments!

Thanks,
Fernando