From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fernando Luis Vazquez Cao Subject: Re: [PATCH 7/9] fsfreeze: freeze_super and thaw_bdev don't play well together Date: Tue, 25 Sep 2012 19:51:33 +0900 Message-ID: <50618CB5.2010602@lab.ntt.co.jp> References: <1347605006.6868.2.camel@nexus.lab.ntt.co.jp> <1347605614.6868.9.camel@nexus.lab.ntt.co.jp> <20120925094828.GD8049@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]:40967 "EHLO tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754545Ab2IYKvt (ORCPT ); Tue, 25 Sep 2012 06:51:49 -0400 In-Reply-To: <20120925094828.GD8049@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 2012/09/25 18:48, Jan Kara wrote: > On Fri 14-09-12 15:53:34, Fernando Luis V=E1zquez 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. >> >> Changes since version 2: >> - Fix reference count leak in freeze_super when MS_BORN flag is n= ot 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 >> --- > I have just some mostly minor comments: Thank you for the detailed review. >> diff -urNp linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c linux-3.6-rc5/fs/= gfs2/ops_fstype.c >> --- linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c 2012-09-14 12:46:15.1528= 71285 +0900 >> +++ linux-3.6-rc5/fs/gfs2/ops_fstype.c 2012-09-14 13:51:36.457205813= +0900 >> @@ -1288,11 +1288,6 @@ static struct dentry *gfs2_mount(struct >> if (IS_ERR(bdev)) >> return ERR_CAST(bdev); >> =20 >> - /* >> - * once the super is inserted into the list by sget, s_umount >> - * will protect the lockfs code from trying to start a snapshot >> - * while we are mounting >> - */ > Shouldn't this comment be replaced with something more accurate in= stead > of just deleting it? Good point. >> @@ -1365,29 +1363,27 @@ static void sb_wait_write(struct super_b >> * freezing. Then we transition to SB_FREEZE_COMPLETE state. This = state is >> * mostly auxiliary for filesystems to verify they do not modify f= rozen fs. >> * >> - * sb->s_writers.frozen is protected by sb->s_umount. >> + * sb->s_writers.frozen and sb->s_freeze_count are protected by sb-= >s_umount. >> */ >> int freeze_super(struct super_block *sb) >> { >> - int ret; >> + int ret =3D 0; >> =20 >> atomic_inc(&sb->s_active); >> down_write(&sb->s_umount); >> - if (sb->s_writers.frozen !=3D SB_UNFROZEN) { >> - deactivate_locked_super(sb); >> - return -EBUSY; >> - } >> =20 >> if (!(sb->s_flags & MS_BORN)) { >> - up_write(&sb->s_umount); >> - return 0; /* sic - it's "nothing to do" */ >> + atomic_dec(&sb->s_active); >> + goto out_active; /* sic - it's "nothing to do" */ > Why not 'goto out_deactivate' here instead of manually decrementin= g > s_active? I was afraid that calling deactivate_locked_super() when the MS_BORN flag is set could release the last active reference to the superblock and end up freeing it. By the way, I probably should explain in the patch that that piece of code fixes a reference leak bug in the current code. 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