On Tue 25-09-12 19:51:33, Fernando Luis Vazquez Cao wrote:

On 2012/09/25 18:48, Jan Kara wrote:

quoted

On Fri 14-09-12 15:53:34, Fernando Luis Vázquez Cao wrote:

quoted

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 <redacted>
Cc: Eric Sandeen <redacted>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <redacted>
Signed-off-by: Fernando Luis Vazquez Cao <redacted>
---

  I have just some mostly minor comments:

Thank you for the detailed review.

quoted

quoted

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.

quoted

quoted

@@ -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 [off-list ref]
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