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 14:20:46 -0500 Message-ID: <5053838E.7010502@redhat.com> 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=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]:24913 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759908Ab2INTVB (ORCPT ); Fri, 14 Sep 2012 15:21:01 -0400 In-Reply-To: <1347605614.6868.9.camel@nexus.lab.ntt.co.jp> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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). >=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. I think what's more important is that a given process or person can exp= ect their freeze to last until they issue an unfreeze, but maybe there are counter-arguments. > 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 > --- > @@ -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); Hm, freeze_super() did: atomic_inc(&sb->s_active); as well as if (++sb->s_freeze_count > 1) for each successful nested freeze. =20 so won't this leave a bunch of active refs on the superblock? > if (!res) > deactivate_locked_super(sb); > diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/includ= e/linux/fs.h > --- linux-3.6-rc5-orig/include/linux/fs.h 2012-09-14 13:46:38.3251795= 10 +0900 > +++ linux-3.6-rc5/include/linux/fs.h 2012-09-14 13:51:36.485205815 +0= 900 > @@ -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 >=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