Re: [PATCH v3 3/3] btrfs: assert that relocation is protected with sb_start_write()

[Filipe Manana <fdmanana@kernel.org>]

On Mon, Mar 28, 2022 at 03:29:22PM +0900, Naohiro Aota wrote:
> btrfs_relocate_chunk() initiates new ordered extents.

Just say the relocation of data block groups creates ordered extents.
Saying btrfs_relocate_chunk() is a bit misleading, since the ordered
extents are created way deeper in the call chain.

> They can cause a
> hang when a process is trying to thaw the filesystem.
> 
> We should have called sb_start_write(), so the filesystem is not being
> frozen. Add an ASSERT to check it is protected.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/relocation.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index fdc2c4b411f0..705799b2b207 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -3977,6 +3977,10 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
>  	if (!bg)
>  		return -ENOENT;
>  
> +	/* Assert we called sb_start_write(), not to race with FS freezing */

Sentences should end with punctuation.
The comment is also a bit vague.

Just say that relocation of data block groups creates ordered extents, and
without sb_start_write() protection, we can deadlock if a fs freeze is started
before the relocation completes, because finishing an ordered extent requires
joining a transaction.

Otherwise it looks good, thanks.


> +	if (bg->flags & BTRFS_BLOCK_GROUP_DATA)
> +		ASSERT(sb_write_started(fs_info->sb));
> +
>  	if (btrfs_pinned_by_swapfile(fs_info, bg)) {
>  		btrfs_put_block_group(bg);
>  		return -ETXTBSY;
> -- 
> 2.35.1
>