From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Sandeen Subject: Re: [PATCH 7/9] fsfreeze: freeze_super and thaw_bdev don't play well together Date: Fri, 14 Sep 2012 20:15:03 -0500 Message-ID: <5053D697.5090006@redhat.com> References: <1347605006.6868.2.camel@nexus.lab.ntt.co.jp> <1347605614.6868.9.camel@nexus.lab.ntt.co.jp> <5053838E.7010502@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Josef Bacik , Dave Chinner , Christoph Hellwig , Jan Kara , linux-fsdevel@vger.kernel.org, fernando@intellilink.co.jp To: =?UTF-8?B?RmVybmFuZG8gTHVpcyBWw6F6cXVleiBDYW8=?= Return-path: Received: from mx1.redhat.com ([209.132.183.28]:56873 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750859Ab2IOBPT (ORCPT ); Fri, 14 Sep 2012 21:15:19 -0400 In-Reply-To: <5053838E.7010502@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 9/14/12 2:20 PM, Eric Sandeen wrote: > On 9/14/12 1:53 AM, Fernando Luis V=C3=A1zquez Cao wrote: >> thaw_bdev() has re-entrancy guards to allow freezes to nest >> together. That is, it ensures that the filesystem is not thawed >> until the last thaw command is issued. This is needed to prevent the >> filesystem from being unfrozen while an existing freezer is still >> operating on the filesystem in a frozen state (e.g. dm-snapshot). >> >> Currently, freeze_super() and thaw_super() bypasses these guards, >> and as a result manual freezing and unfreezing via the ioctl methods >> do not nest correctly. hence mixing userspace directed freezes with >> block device level freezes result in inconsistency due to premature >> thawing of the filesystem. >> >> Move the re-enterency guards to the superblock and into freeze_super >> and thaw_super() so that userspace directed freezes nest correctly >> again. Caveat: Things work as expected as long as direct calls to >> thaw_super are always in response to a previous sb level freeze. In >> other words an unpaired call to thaw_super can still thaw a >> filesystem frozen using freeze_bdev (this issue could be addressed >> in a follow-up patch if deemed necessary). >> >> This patch retains the bdev level mutex and counter to keep the >> "feature" that we can freeze a block device that does not have a >> filesystem mounted yet. >> >> IMPORTANT CAVEAT: This patch restores the nesting feature of the fsf= reeze ioctl >> which got removed by commit 18e9e5104fcd9a973ffe3eed3816c87f2a1b6cd2 >> ("Introduce freeze_super and thaw_super for the fsfreeze ioctl"). It= could be >> argued that it is too late to fix the userland ABI breakage caused b= y that >> patch and that the current ABI is the one that should be kept. If th= is is the >> respective maintainer(s) opinion this could be modified to not allow= fsfreeze >> ioctl nesting. >=20 > I think what's more important is that a given process or person can e= xpect > their freeze to last until they issue an unfreeze, but maybe there ar= e > counter-arguments. >=20 >> Changes since version 2: >> - Fix reference count leak in freeze_super when MS_BORN flag is no= t set in >> the superblock. >> - Protect freeze counter using s_umount and get rid of superblock = level >> fsfreeze mutex. >> >> Cc: Josef Bacik >> Cc: Eric Sandeen >> Cc: Christoph Hellwig >> Cc: Jan Kara >> Cc: Dave Chinner >> Signed-off-by: Fernando Luis Vazquez Cao >> --- >=20 > >=20 >> @@ -1506,6 +1514,7 @@ static void do_thaw_one(struct super_blo >> =20 >> /* We were called from __iterate_supers with superblock lock taken >> * so we do not need to do it here. */ >> + sb->s_freeze_count =3D 1; /* avoid multiple calls to __thaw_super = */ >> res =3D __thaw_super(sb); >=20 > Hm, freeze_super() did: >=20 > atomic_inc(&sb->s_active); >=20 > as well as >=20 > if (++sb->s_freeze_count > 1) >=20 > for each successful nested freeze. =20 >=20 > so won't this leave a bunch of active refs on the superblock? Sorry, Fernando pointed out that I missed the fact that we only keep 1 = active ref on s_active even for nested freezers. So this is ok. -Eric >=20 >> if (!res) >> deactivate_locked_super(sb); >> diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/inclu= de/linux/fs.h >> --- linux-3.6-rc5-orig/include/linux/fs.h 2012-09-14 13:46:38.325179= 510 +0900 >> +++ linux-3.6-rc5/include/linux/fs.h 2012-09-14 13:51:36.485205815 += 0900 >> @@ -1578,6 +1578,8 @@ struct super_block { >> =20 >> /* Being remounted read-only */ >> int s_readonly_remount; >> + >> + int s_freeze_count; /* nr of nested freezes */ >> }; >> =20 >> /* superblock cache pruning functions */ >> >> >=20 -- 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