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

[Jan Kara <jack@suse.cz>]

On Fri 05-10-12 14:42:40, 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.
> 
> 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>
> Cc: Konishi Ryusuke <konishi.ryusuke@lab.ntt.co.jp>
> Cc: Steven Whitehouse <swhiteho@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
  Just some style nits below. So after fixing them you can add:
Reviewed-by: Jan Kara <jack@suse.cz>

									Honza
> ---
> 
> diff -urNp linux-3.6-orig/fs/block_dev.c linux-3.6/fs/block_dev.c
> --- linux-3.6-orig/fs/block_dev.c	2012-10-04 15:05:42.168084928 +0900
> +++ linux-3.6/fs/block_dev.c	2012-10-04 15:08:40.228086607 +0900
...
>  int thaw_bdev(struct block_device *bdev, struct super_block *sb)
>  {
>  	int error = -EINVAL;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> +
>  	if (!bdev->bd_fsfreeze_count)
>  		goto out;
>  
> -	if (--bdev->bd_fsfreeze_count > 0 || !sb) {
> +	bdev->bd_fsfreeze_count--;
> +
> +	if (!sb) {
>  		error = 0;
>  		goto out;
>  	}
>  
>  	error = thaw_super(sb);
> -	if (error)
> +	/* If the superblock is already unfrozen, i.e. thaw_super() returned
> +	 * -EINVAL, we consider the block device level thaw successful. This
> +	 * behavior is important in a scenario where a filesystem frozen using
> +	 * freeze_bdev() is thawed through the superblock level API; if we
> +	 * caused the subsequent thaw_bdev() to fail bdev->bd_fsfreeze_count
> +	 * would not go back to 0 which means that future calls to freeze_bdev()
> +	 * would not freeze the superblock, just increase the counter. */
  ^^ The correct formatting of long comments is:
  /*
   * Text
   * Text
   */

> +	if (error && error != -EINVAL)
>  		bdev->bd_fsfreeze_count++;
> +	else
> +		error = 0;
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
...
> diff -urNp linux-3.6-orig/fs/super.c linux-3.6/fs/super.c
> --- linux-3.6-orig/fs/super.c	2012-10-04 15:06:00.264085275 +0900
> +++ linux-3.6/fs/super.c	2012-10-04 15:09:21.164086840 +0900
...
> @@ -1356,29 +1363,29 @@ 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" */
> -	}
> +	if (++sb->s_freeze_count > 1)
> +		goto out_deactivate;
> +
> +	/* If MS_BORN is not set it means we have a failing mount (this is
> +	 * possible if we got here from freeze_bdev()). Keep an active
> +	 * reference so that the superblock is not killed until it is thawed
> +	 * via thaw_bdev(). */
  Again, comment formatting is wrong.

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