From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 7/9] fsfreeze: freeze_super and thaw_bdev don't play well together Date: Tue, 25 Sep 2012 11:48:28 +0200 Message-ID: <20120925094828.GD8049@quack.suse.cz> References: <1347605006.6868.2.camel@nexus.lab.ntt.co.jp> <1347605614.6868.9.camel@nexus.lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Josef Bacik , Eric Sandeen , Dave Chinner , Christoph Hellwig , Jan Kara , linux-fsdevel@vger.kernel.org, fernando@intellilink.co.jp To: Fernando Luis =?iso-8859-1?Q?V=E1zquez?= Cao Return-path: Received: from cantor2.suse.de ([195.135.220.15]:46665 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753230Ab2IYJsd (ORCPT ); Tue, 25 Sep 2012 05:48:33 -0400 Content-Disposition: inline In-Reply-To: <1347605614.6868.9.camel@nexus.lab.ntt.co.jp> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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). >=20 > 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. >=20 > 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). >=20 > 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. >=20 > IMPORTANT CAVEAT: This patch restores the nesting feature of the fsfr= eeze 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 by= that > patch and that the current ABI is the one that should be kept. If thi= s is the > respective maintainer(s) opinion this could be modified to not allow = fsfreeze > ioctl nesting. >=20 > Changes since version 2: > - Fix reference count leak in freeze_super when MS_BORN flag is not= set in > the superblock. > - Protect freeze counter using s_umount and get rid of superblock l= evel > fsfreeze mutex. >=20 > 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: =2E.. > diff -urNp linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c linux-3.6-rc5/fs/g= fs2/ops_fstype.c > --- linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c 2012-09-14 12:46:15.15287= 1285 +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 inste= ad of just deleting it? =2E.. > @@ -1365,29 +1363,27 @@ static void sb_wait_write(struct super_b > * freezing. Then we transition to SB_FREEZE_COMPLETE state. This st= ate is > * mostly auxiliary for filesystems to verify they do not modify fro= zen 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 decrementing s_active? Honza --=20 Jan Kara SUSE Labs, CR -- 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