Re: [PATCH 7/9] fsfreeze: freeze_super and thaw_bdev don't play well together

[Jan Kara <jack@suse.cz>]

On Tue 25-09-12 19:51:33, Fernando Luis Vazquez Cao wrote:
> On 2012/09/25 18:48, Jan Kara wrote:
> >On Fri 14-09-12 15:53:34, Fernando Luis Vázquez 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 fsfreeze 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 this 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 not set in
> >>     the superblock.
> >>   - Protect freeze counter using s_umount and get rid of superblock level
> >>     fsfreeze mutex.
> >>
> >>Cc: Josef Bacik <jbacik@fusionio.com>
> >>Cc: Eric Sandeen <sandeen@redhat.com>
> >>Cc: Christoph Hellwig <hch@infradead.org>
> >>Cc: Jan Kara <jack@suse.cz>
> >>Cc: Dave Chinner <dchinner@redhat.com>
> >>Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> >>---
> >   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.152871285 +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);
> >>-	/*
> >>-	 * 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 instead
> >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 frozen 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 = 0;
> >>  	atomic_inc(&sb->s_active);
> >>  	down_write(&sb->s_umount);
> >>-	if (sb->s_writers.frozen != SB_UNFROZEN) {
> >>-		deactivate_locked_super(sb);
> >>-		return -EBUSY;
> >>-	}
> >>  	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?
> 
> 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.
  Well, the caller has to have the sb pinned somehow so that it can safely
pass it into freeze_super(). So deactivate_locked_super() cannot really
end up freeing the superblock...

> By the way, I probably should explain in the
> patch that that piece of code fixes a reference
> leak bug in the current code.
  Yep, that would be nice.

								Honza
-- 
Jan Kara <jack@suse.cz>
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