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: Mon, 8 Oct 2012 16:17:03 +0200 Message-ID: <20121008141703.GD9243@quack.suse.cz> References: <1349414653.7347.2.camel@nexus.lab.ntt.co.jp> <1349415760.7347.14.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, Konishi Ryusuke , Steven Whitehouse To: Fernando Luis =?iso-8859-1?Q?V=E1zquez?= Cao Return-path: Received: from cantor2.suse.de ([195.135.220.15]:58010 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750779Ab2JHORG (ORCPT ); Mon, 8 Oct 2012 10:17:06 -0400 Content-Disposition: inline In-Reply-To: <1349415760.7347.14.camel@nexus.lab.ntt.co.jp> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri 05-10-12 14:42:40, 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 > Cc: Josef Bacik > Cc: Eric Sandeen > Cc: Christoph Hellwig > Cc: Jan Kara > Cc: Dave Chinner > Cc: Konishi Ryusuke > Cc: Steven Whitehouse > Signed-off-by: Fernando Luis Vazquez Cao Just some style nits below. So after fixing them you can add: Reviewed-by: Jan Kara Honza > --- >=20 > diff -urNp linux-3.6-orig/fs/block_dev.c linux-3.6/fs/block_dev.c > --- linux-3.6-orig/fs/block_dev.c 2012-10-04 15:05:42.168084928 +0900 > +++ linux-3.6/fs/block_dev.c 2012-10-04 15:08:40.228086607 +0900 =2E.. > int thaw_bdev(struct block_device *bdev, struct super_block *sb) > { > int error =3D -EINVAL; > =20 > mutex_lock(&bdev->bd_fsfreeze_mutex); > + > if (!bdev->bd_fsfreeze_count) > goto out; > =20 > - if (--bdev->bd_fsfreeze_count > 0 || !sb) { > + bdev->bd_fsfreeze_count--; > + > + if (!sb) { > error =3D 0; > goto out; > } > =20 > error =3D thaw_super(sb); > - if (error) > + /* If the superblock is already unfrozen, i.e. thaw_super() returne= d > + * -EINVAL, we consider the block device level thaw successful. Thi= s > + * behavior is important in a scenario where a filesystem frozen us= ing > + * freeze_bdev() is thawed through the superblock level API; if we > + * caused the subsequent thaw_bdev() to fail bdev->bd_fsfreeze_coun= t > + * would not go back to 0 which means that future calls to freeze_b= dev() > + * would not freeze the superblock, just increase the counter. */ ^^ The correct formatting of long comments is: /* * Text * Text */ > + if (error && error !=3D -EINVAL) > bdev->bd_fsfreeze_count++; > + else > + error =3D 0; > out: > mutex_unlock(&bdev->bd_fsfreeze_mutex); > return error; =2E.. > diff -urNp linux-3.6-orig/fs/super.c linux-3.6/fs/super.c > --- linux-3.6-orig/fs/super.c 2012-10-04 15:06:00.264085275 +0900 > +++ linux-3.6/fs/super.c 2012-10-04 15:09:21.164086840 +0900 =2E.. > @@ -1356,29 +1363,29 @@ 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" */ > - } > + if (++sb->s_freeze_count > 1) > + goto out_deactivate; > + > + /* If MS_BORN is not set it means we have a failing mount (this is > + * possible if we got here from freeze_bdev()). Keep an active > + * reference so that the superblock is not killed until it is thawe= d > + * via thaw_bdev(). */ Again, comment formatting is wrong. --=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