[PATCH v3 0/3] protect relocation with sb_start_write

[PATCH v3 0/3] protect relocation with sb_start_write

[Naohiro Aota]

This series is a follow-up to the series below. 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() [1].

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

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

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

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

--
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 check functions for sb_start_{write,pagefault,intwrite}
  btrfs: assert that relocation is protected with sb_start_write()

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

-- 
2.35.1

[PATCH v3 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 v3 2/3] fs: add check functions for sb_start_{write,pagefault,intwrite}

[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..57fedc4af4a1 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(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

Re: [PATCH v3 2/3] fs: add check functions for sb_start_{write,pagefault,intwrite}

[Filipe Manana]

On Mon, Mar 28, 2022 at 03:29:21PM +0900, Naohiro Aota wrote:
> 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..57fedc4af4a1 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(struct super_block *sb)

The argument can be made const.

Also, the subject "fs: add check functions for sb_start_{write,pagefault,intwrite}" is
now oudated, since only sb_write_started() is added.

Thanks.

> +{
> +	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 v3 3/3] btrfs: assert that relocation is protected with sb_start_write()

[Naohiro Aota]

btrfs_relocate_chunk() initiates new 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 | 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 */
+	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 v3 3/3] btrfs: assert that relocation is protected with sb_start_write()

[Filipe Manana]

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
>