[PATCH v4 0/3] protect relocation with sb_start_write

[PATCH v4 0/3] protect relocation with sb_start_write

[Naohiro Aota]

This series is a follow-up to the series below [1]. The old series added
an assertion to btrfs_relocate_chunk() to check if it is protected
with sb_start_write(). However, it revealed another location we need
to add sb_start_write() [2].

Also, this series moved the ASSERT from btrfs_relocate_chunk() to
btrfs_relocate_block_group() because we do not need to call
sb_start_write() on a non-data block group [3].

[1] https://lore.kernel.org/linux-btrfs/cover.1645157220.git.naohiro.aota@wdc.com/T/

[2] https://lore.kernel.org/linux-btrfs/cover.1645157220.git.naohiro.aota@wdc.com/T/#e06eecc07ce1cd1e45bfd30a374bd2d15b4fd76d8

[3] https://lore.kernel.org/linux-btrfs/YjMSaLIhKNcKUuHM@debian9.Home/

Patch 1 adds sb_{start,end}_write() to the resumed async balancing.

Patches 2 and 3 add an ASSERT to catch a future error.

--
v4:
  - Fix subject of patch 2 (Filipe)
  - Revise the comment for the ASSERT (Filipe)
v3:
  - Only add sb_write_started(), which we really use. (Dave Chinner)
  - Drop patch "btrfs: mark device addition as mnt_want_write_file" (Filipe Manana)
  - Narrow asserting region to btrfs_relocate_block_group() and check only
    when the BG is data BG. (Filipe Manana)
v2:
  - Use mnt_want_write_file() instead of sb_start_write() for
    btrfs_ioctl_add_dev()
  - Drop bogus fixes tag
  - Change the stable target to meaningful 4.9+

Naohiro Aota (3):
  btrfs: mark resumed async balance as writing
  fs: add a check function for sb_start_write()
  btrfs: assert that relocation is protected with sb_start_write()

 fs/btrfs/relocation.c | 10 ++++++++++
 fs/btrfs/volumes.c    |  2 ++
 include/linux/fs.h    |  5 +++++
 3 files changed, 17 insertions(+)

-- 
2.35.1

[PATCH v4 1/3] btrfs: mark resumed async balance as writing

[Naohiro Aota]

When btrfs balance is interrupted with umount, the background balance
resumes on the next mount. There is a potential deadlock with FS freezing
here like as described in commit 26559780b953 ("btrfs: zoned: mark
relocation as writing"). Mark the process as sb_writing to avoid it.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Cc: stable@vger.kernel.org # 4.9+
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3fd17e87815a..3471698fd831 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4430,10 +4430,12 @@ static int balance_kthread(void *data)
 	struct btrfs_fs_info *fs_info = data;
 	int ret = 0;
 
+	sb_start_write(fs_info->sb);
 	mutex_lock(&fs_info->balance_mutex);
 	if (fs_info->balance_ctl)
 		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL);
 	mutex_unlock(&fs_info->balance_mutex);
+	sb_end_write(fs_info->sb);
 
 	return ret;
 }
-- 
2.35.1

[PATCH v4 2/3] fs: add a check function for sb_start_write()

[Naohiro Aota]

Add a function sb_write_started() to return if sb_start_write() is
properly called. It is used in the next commit.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 include/linux/fs.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 27746a3da8fd..1f3df6e8a74d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1732,6 +1732,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)
 #define __sb_writers_release(sb, lev)	\
 	percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_)
 
+static inline bool sb_write_started(const struct super_block *sb)
+{
+	return lockdep_is_held_type(sb->s_writers.rw_sem + SB_FREEZE_WRITE - 1, 1);
+}
+
 /**
  * sb_end_write - drop write access to a superblock
  * @sb: the super we wrote to
-- 
2.35.1

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

[Naohiro Aota]

Relocation of a data block group creates ordered extents. 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index fdc2c4b411f0..5e52cd8d5f23 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3977,6 +3977,16 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 	if (!bg)
 		return -ENOENT;
 
+	/*
+	 * Relocation of a data block group creates ordered extents.
+	 * Without sb_start_write(), we can freeze the FS while unfinished
+	 * ordered extents are left. Such ordered extents can cause a
+	 * deadlock e.g, when syncfs() is trying to finish them because
+	 * they never finish as the FS is already frozen.
+	 */
+	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

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

[Filipe Manana]

On Tue, Mar 29, 2022 at 03:56:00PM +0900, Naohiro Aota wrote:
> Relocation of a data block group creates ordered extents. 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 | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index fdc2c4b411f0..5e52cd8d5f23 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -3977,6 +3977,16 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
>  	if (!bg)
>  		return -ENOENT;
>  
> +	/*
> +	 * Relocation of a data block group creates ordered extents.
> +	 * Without sb_start_write(), we can freeze the FS while unfinished
> +	 * ordered extents are left. Such ordered extents can cause a
> +	 * deadlock e.g, when syncfs() is trying to finish them because

syncfs() is not trying to finish them, it's waiting for them to complete.

> +	 * they never finish as the FS is already frozen.

More specifically they can't finish because they block when joining a transaction,
due to the fact that the freeze locks are being held in write mode.

Anyway, I won't make you send yet another version. Perhaps this is something
David can fixup when he picks the patch.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

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
> 

Re: [PATCH v4 0/3] protect relocation with sb_start_write

[David Sterba]

On Tue, Mar 29, 2022 at 03:55:57PM +0900, Naohiro Aota wrote:
> This series is a follow-up to the series below [1]. The old series added
> an assertion to btrfs_relocate_chunk() to check if it is protected
> with sb_start_write(). However, it revealed another location we need
> to add sb_start_write() [2].
> 
> Also, this series moved the ASSERT from btrfs_relocate_chunk() to
> btrfs_relocate_block_group() because we do not need to call
> sb_start_write() on a non-data block group [3].
> 
> [1] https://lore.kernel.org/linux-btrfs/cover.1645157220.git.naohiro.aota@wdc.com/T/
> 
> [2] https://lore.kernel.org/linux-btrfs/cover.1645157220.git.naohiro.aota@wdc.com/T/#e06eecc07ce1cd1e45bfd30a374bd2d15b4fd76d8
> 
> [3] https://lore.kernel.org/linux-btrfs/YjMSaLIhKNcKUuHM@debian9.Home/
> 
> Patch 1 adds sb_{start,end}_write() to the resumed async balancing.
> 
> Patches 2 and 3 add an ASSERT to catch a future error.
> 
> --
> v4:
>   - Fix subject of patch 2 (Filipe)
>   - Revise the comment for the ASSERT (Filipe)
> v3:
>   - Only add sb_write_started(), which we really use. (Dave Chinner)
>   - Drop patch "btrfs: mark device addition as mnt_want_write_file" (Filipe Manana)
>   - Narrow asserting region to btrfs_relocate_block_group() and check only
>     when the BG is data BG. (Filipe Manana)
> v2:
>   - Use mnt_want_write_file() instead of sb_start_write() for
>     btrfs_ioctl_add_dev()
>   - Drop bogus fixes tag
>   - Change the stable target to meaningful 4.9+
> 
> Naohiro Aota (3):
>   btrfs: mark resumed async balance as writing
>   fs: add a check function for sb_start_write()
>   btrfs: assert that relocation is protected with sb_start_write()

Added to misc-next, with the comment of 3/3 updated according to
Filipe's suggestion. Thanks.