From mboxrd@z Thu Jan  1 00:00:00 1970
From: Fernando Luis Vazquez Cao <fernando_b1@lab.ntt.co.jp>
Subject: Re: [PATCH 4/9] fsfreeze: emergency thaw will deadlock on s_umount
Date: Tue, 09 Oct 2012 18:52:27 +0900
Message-ID: <5073F3DB.8020408@lab.ntt.co.jp>
References: <1349414653.7347.2.camel@nexus.lab.ntt.co.jp> <1349415353.7347.8.camel@nexus.lab.ntt.co.jp> <20121008135733.GC9243@quack.suse.cz> <5073B128.6010107@lab.ntt.co.jp> <20121009082045.GB15790@quack.suse.cz>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	Josef Bacik <jbacik@fusionio.com>,
	Eric Sandeen <sandeen@redhat.com>,
	Dave Chinner <dchinner@redhat.com>,
	Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org,
	=?ISO-8859-1?Q?Fernando_Luis_V=E1zque?= =?ISO-8859-1?Q?z_Cao?=
	<fernando@intellilink.co.jp>
To: Jan Kara <jack@suse.cz>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from tama500.ecl.ntt.co.jp ([129.60.39.148]:52043 "EHLO
	tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754519Ab2JIJwp (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Tue, 9 Oct 2012 05:52:45 -0400
In-Reply-To: <20121009082045.GB15790@quack.suse.cz>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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