Re: [PATCH 04/45] fs: simplify freeze_bdev/thaw_bdev

[Jan Kara <jack@xxxxxxx>]

On Tue 24-11-20 14:27:10, Christoph Hellwig wrote:
> Store the frozen superblock in struct block_device to avoid the awkward
> interface that can return a sb only used a cookie, an ERR_PTR or NULL.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Some comments below...

> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index d8664f5c1ff669..60492620d51866 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -548,55 +548,47 @@ EXPORT_SYMBOL(fsync_bdev);
>   * count down in thaw_bdev(). When it becomes 0, thaw_bdev() will unfreeze
>   * actually.
>   */
> -struct super_block *freeze_bdev(struct block_device *bdev)
> +int freeze_bdev(struct block_device *bdev)
>  {
>       struct super_block *sb;
>       int error = 0;
>  
>       mutex_lock(&bdev->bd_fsfreeze_mutex);
> -     if (++bdev->bd_fsfreeze_count > 1) {
> -             /*
> -              * We don't even need to grab a reference - the first call
> -              * to freeze_bdev grab an active reference and only the last
> -              * thaw_bdev drops it.
> -              */
> -             sb = get_super(bdev);
> -             if (sb)
> -                     drop_super(sb);
> -             mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -             return sb;
> -     }
> +     if (++bdev->bd_fsfreeze_count > 1)
> +             goto done;
>  
>       sb = get_active_super(bdev);
>       if (!sb)
> -             goto out;
> +             goto sync;
>       if (sb->s_op->freeze_super)
>               error = sb->s_op->freeze_super(sb);
>       else
>               error = freeze_super(sb);
> +     deactivate_super(sb);
> +
>       if (error) {
> -             deactivate_super(sb);
>               bdev->bd_fsfreeze_count--;
> -             mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -             return ERR_PTR(error);
> +             goto done;
>       }
> -     deactivate_super(sb);
> - out:
> +     bdev->bd_fsfreeze_sb = sb;
> +
> +sync:
>       sync_blockdev(bdev);
> +done:
>       mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -     return sb;      /* thaw_bdev releases s->s_umount */
> +     return error;   /* thaw_bdev releases s->s_umount */

The comment about thaw_bdev() seems to be stale? At least I don't see what
it's speaking about...

>  }
>  EXPORT_SYMBOL(freeze_bdev);
>  
>  /**
>   * thaw_bdev  -- unlock filesystem
>   * @bdev:    blockdevice to unlock
> - * @sb:              associated superblock
>   *
>   * Unlocks the filesystem and marks it writeable again after freeze_bdev().
>   */
> -int thaw_bdev(struct block_device *bdev, struct super_block *sb)
> +int thaw_bdev(struct block_device *bdev)
>  {
> +     struct super_block *sb;
>       int error = -EINVAL;
>  
>       mutex_lock(&bdev->bd_fsfreeze_mutex);
> @@ -607,6 +599,7 @@ int thaw_bdev(struct block_device *bdev, struct 
> super_block *sb)
>       if (--bdev->bd_fsfreeze_count > 0)
>               goto out;
>  
> +     sb = bdev->bd_fsfreeze_sb;
>       if (!sb)
>               goto out;
>  
> @@ -618,7 +611,7 @@ int thaw_bdev(struct block_device *bdev, struct 
> super_block *sb)
>               bdev->bd_fsfreeze_count++;
>  out:
>       mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -     return error;
> +     return 0;

But we now won't return -EINVAL if this gets called e.g. with
bd_fsfreeze_count == 0, right?

                                                                        Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR