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@xxxxxxxxxxxx>
> Cc: Eric Sandeen <sandeen@xxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> Cc: Konishi Ryusuke <konishi.ryusuke@xxxxxxxxxxxxx>
> Cc: Steven Whitehouse <swhiteho@xxxxxxxxxx>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@xxxxxxxxxxxxx>
Just some style nits below. So after fixing them you can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
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@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
