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

[Jan Kara <jack@suse.cz>]

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:

...
> 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@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