From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fernando Luis Vazquez Cao Subject: Re: [PATCH 4/9] fsfreeze: emergency thaw will deadlock on s_umount Date: Tue, 25 Sep 2012 19:31:49 +0900 Message-ID: <50618815.70402@lab.ntt.co.jp> References: <1347605006.6868.2.camel@nexus.lab.ntt.co.jp> <1347605336.6868.6.camel@nexus.lab.ntt.co.jp> <20120925092423.GC8049@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Josef Bacik , Eric Sandeen , Dave Chinner , Christoph Hellwig , linux-fsdevel@vger.kernel.org To: Jan Kara Return-path: Received: from tama500.ecl.ntt.co.jp ([129.60.39.148]:40791 "EHLO tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754791Ab2IYKcO (ORCPT ); Tue, 25 Sep 2012 06:32:14 -0400 In-Reply-To: <20120925092423.GC8049@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 2012/09/25 18:24, Jan Kara wrote: > On Fri 14-09-12 15:48:56, Fernando Luis V=E1zquez Cao wrote: >> The emergency thaw process uses iterate_super() which holds the >> sb->s_umount lock in read mode. The current thaw_super() code takes >> the sb->s_umount lock in write mode, hence leading to an instant >> deadlock. >> >> Use the unlocked version of thaw_super() to do the thawing and repla= ce >> iterate_supers() with __iterate_supers() so that the unfreeze operat= ion can >> be performed with s_umount held as the locking rules for fsfreeze in= dicate. >> >> As a bonus, by using thaw_super(), which does not nest, instead of t= haw_bdev() >> when can get rid of the ugly while loop. > Hum, but that will leave block devices frozen? Isn't that a bug? I= t > definitely creates an inconsistent state. I think emergency thaw shou= ld > also iterate over all block devices (via iterate_bdevs()) and thaw al= l of > them (and no, this does not allow us to avoid the iterate_super() cha= nges > because there are filesystems without block devices...). Yes, we may end up with an inconsistent state when bdev->bd_fsfreeze_count =3D=3D 1 entering thaw_bdev (other cases will work as expected) because we have this code: error =3D thaw_super(sb); if (error) { bdev->bd_fsfreeze_count++; mutex_unlock(&bdev->bd_fsfreeze_mutex); return error; } After the emergency unfreeze thaw_super() will return -EINVAL and bdev->bd_fsfreeze_count will be restored to one which can confuse the caller. I think that the best solution to this problem is considering the thaw successful when thaw_super() returns -EINVAL, because that means the filesystem is already unfrozen. I prefer this approach to thawing bdevs too during emergency thaw, because dm or external users may call thaw_bdev after that, which would fail returning -EINVAL. If you agree with this I will implement it. Thanks, =46ernando -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html