Re: [PATCH 3/5] fs: make the block device callbacks wait for superblocks being shut down

[Jan Kara]

On Wed 21-10-26 00:56:37, Josef Bacik wrote:
> Three of the callbacks the block layer has into the filesystem using a
> device rely on super_lock() sleeping on s_umount until an unmount is done,
> in the same unspoken way as sync(2) and freezing for suspend:
> 
>  - fs_bdev_sync() is there to get the filesystem onto the device.  While it
>    is being unmounted, the unmount is what does that.
> 
>  - fs_bdev_freeze() promises that nothing writes to the device once it has
>    returned.  An unmount writes to it until ->put_super() is done.
> 
>  - fs_bdev_mark_dead() without @surprise is an orderly removal, which syncs
>    the filesystem before the device goes away.  Going ahead while an
>    unmount is still writing would lose what it hasn't written yet.
> 
> Have them wait for SB_DYING when the superblock turns out to be going
> away, ahead of super_lock() not sleeping on such a superblock any more.
> With @surprise the device is gone already and there is nothing to wait
> for.
> 
> Only fs_bdev_sync() can be killed while it waits.  Its one caller is
> BLKFLSBUF, and all that does afterwards is invalidate_bdev(), which leaves
> dirty pages and pages under writeback alone.
> 
> The other two use super_wait_shutdown() and can't be killed, as they can't
> be today.  Killing the task doesn't call off the removal of the
> device, it would only make it overtake the unmount.  fs_bdev_freeze() could
> return an error, but by then it may have frozen other superblocks on the
> device, which its caller doesn't thaw on failure.
> 
> For now SB_DYING is always set by the time super_lock() fails, so nothing
> changes yet.
> 
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/super.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 76101307da01..9baaaa3256b2 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1572,8 +1572,12 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>  	for (sb_dev = super_dev_first(dev); sb_dev; sb_dev = super_dev_next(sb_dev)) {
>  		struct super_block *sb = sb_dev->sd_sb;
>  
> -		if (!super_lock_shared(sb))
> +		if (!super_lock_shared(sb)) {
> +			/* don't take the device away from under an unmount */
> +			if (!surprise)
> +				super_wait_shutdown(sb);
>  			continue;
> +		}
>  		if (sb->s_root && (sb->s_flags & SB_ACTIVE)) {
>  			if (!sb->s_op->remove_bdev ||
>  			    sb->s_op->remove_bdev(sb, bdev)) {
> @@ -1599,8 +1603,13 @@ static void fs_bdev_sync(struct block_device *bdev)
>  	for (sb_dev = super_dev_first(dev); sb_dev; sb_dev = super_dev_next(sb_dev)) {
>  		struct super_block *sb = sb_dev->sd_sb;
>  
> -		if (!super_lock_shared(sb))
> +		if (!super_lock_shared(sb)) {
> +			if (super_wait_shutdown_killable(sb)) {
> +				super_dev_put(sb_dev);
> +				return;
> +			}
>  			continue;
> +		}
>  		if (sb->s_root && (sb->s_flags & SB_ACTIVE))
>  			sync_filesystem(sb);
>  		super_unlock_shared(sb);
> @@ -1631,8 +1640,11 @@ static int fs_bdev_freeze(struct block_device *bdev)
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
>  	for (sb_dev = super_dev_first(dev); sb_dev; sb_dev = super_dev_next(sb_dev)) {
> -		if (!get_active_super(sb_dev->sd_sb))
> +		if (!get_active_super(sb_dev->sd_sb)) {
> +			/* an unmount writes to the device until it is done */
> +			super_wait_shutdown(sb_dev->sd_sb);
>  			continue;
> +		}
>  		err = fs_super_freeze(sb_dev->sd_sb);
>  		if (err && !error)
>  			error = err;
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR