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@xxxxxxxxxxxx>
> Cc: Eric Sandeen <sandeen@xxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@xxxxxxxxxxxxx>
> ---
I have just some mostly minor comments:
...
> 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?
...
> @@ -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?
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
