On 9/14/12 1:53 AM, 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.
I think what's more important is that a given process or person can expect
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 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>
> ---
<big snip for now>
> @@ -1506,6 +1514,7 @@ static void do_thaw_one(struct super_blo
>
> /* We were called from __iterate_supers with superblock lock taken
> * so we do not need to do it here. */
> + sb->s_freeze_count = 1; /* avoid multiple calls to __thaw_super */
> res = __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.
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/include/linux/fs.h
> --- linux-3.6-rc5-orig/include/linux/fs.h 2012-09-14 13:46:38.325179510 +0900
> +++ linux-3.6-rc5/include/linux/fs.h 2012-09-14 13:51:36.485205815 +0900
> @@ -1578,6 +1578,8 @@ struct super_block {
>
> /* Being remounted read-only */
> int s_readonly_remount;
> +
> + int s_freeze_count; /* nr of nested freezes */
> };
>
> /* superblock cache pruning functions */
>
>
--
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
