[PATCH] fs: don't flush in-flight wb switches for superblocks without cgroup writeback

[PATCH] fs: don't flush in-flight wb switches for superblocks without cgroup writeback

[Haifeng Xu]

When deactivating any type of superblock, it had to wait for the in-flight
wb switches to be completed. wb switches are executed in inode_switch_wbs_work_fn()
which needs to acquire the wb_switch_rwsem and races against sync_inodes_sb().
If there are too much dirty data in the superblock, the waiting time may increase
significantly.

For superblocks without cgroup writeback such as tmpfs, they have nothing to
do with the wb swithes, so the flushing can be avoided.

Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
---
 fs/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/super.c b/fs/super.c
index 095ba793e10c..f846f853e957 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -621,7 +621,8 @@ void generic_shutdown_super(struct super_block *sb)
 		sync_filesystem(sb);
 		sb->s_flags &= ~SB_ACTIVE;
 
-		cgroup_writeback_umount();
+		if (sb->s_bdi != &noop_backing_dev_info)
+			cgroup_writeback_umount();
 
 		/* Evict all inodes with zero refcount. */
 		evict_inodes(sb);
-- 
2.25.1

Re: [PATCH] fs: don't flush in-flight wb switches for superblocks without cgroup writeback

[Jan Kara]

On Thu 25-07-24 10:39:58, Haifeng Xu wrote:
> When deactivating any type of superblock, it had to wait for the in-flight
> wb switches to be completed. wb switches are executed in inode_switch_wbs_work_fn()
> which needs to acquire the wb_switch_rwsem and races against sync_inodes_sb().
> If there are too much dirty data in the superblock, the waiting time may increase
> significantly.
> 
> For superblocks without cgroup writeback such as tmpfs, they have nothing to
> do with the wb swithes, so the flushing can be avoided.
> 
> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
> ---
>  fs/super.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 095ba793e10c..f846f853e957 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -621,7 +621,8 @@ void generic_shutdown_super(struct super_block *sb)
>  		sync_filesystem(sb);
>  		sb->s_flags &= ~SB_ACTIVE;
>  
> -		cgroup_writeback_umount();
> +		if (sb->s_bdi != &noop_backing_dev_info)
> +			cgroup_writeback_umount();

So a more obvious check would be:

		if (sb->s_bdi->capabilities & BDI_CAP_WRITEBACK)

even better would be if we'd pass 'sb' into cgroup_writeback_umount() and
that function would do this check inside so that callers don't have to
bother... I know there is only one caller so this is not a huge deal but
still I'd find it cleaner that way.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't flush in-flight wb switches for superblocks without cgroup writeback

[Haifeng Xu]

             

On 2024/7/25 16:42, Jan Kara wrote:
> On Thu 25-07-24 10:39:58, Haifeng Xu wrote:
>> When deactivating any type of superblock, it had to wait for the in-flight
>> wb switches to be completed. wb switches are executed in inode_switch_wbs_work_fn()
>> which needs to acquire the wb_switch_rwsem and races against sync_inodes_sb().
>> If there are too much dirty data in the superblock, the waiting time may increase
>> significantly.
>>
>> For superblocks without cgroup writeback such as tmpfs, they have nothing to
>> do with the wb swithes, so the flushing can be avoided.
>>
>> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
>> ---
>>  fs/super.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/super.c b/fs/super.c
>> index 095ba793e10c..f846f853e957 100644
>> --- a/fs/super.c
>> +++ b/fs/super.c
>> @@ -621,7 +621,8 @@ void generic_shutdown_super(struct super_block *sb)
>>  		sync_filesystem(sb);
>>  		sb->s_flags &= ~SB_ACTIVE;
>>  
>> -		cgroup_writeback_umount();
>> +		if (sb->s_bdi != &noop_backing_dev_info)
>> +			cgroup_writeback_umount();
> 
> So a more obvious check would be:
> 
> 		if (sb->s_bdi->capabilities & BDI_CAP_WRITEBACK)
> 
> even better would be if we'd pass 'sb' into cgroup_writeback_umount() and
> that function would do this check inside so that callers don't have to
> bother... I know there is only one caller so this is not a huge deal but
> still I'd find it cleaner that way.
> 
> 								Honza
> 

Yes, Thanks for you suggestions!

[PATCH v2] fs: don't flush in-flight wb switches for superblocks without cgroup writeback

[Haifeng Xu]

When deactivating any type of superblock, it had to wait for the in-flight
wb switches to be completed. wb switches are executed in inode_switch_wbs_work_fn()
which needs to acquire the wb_switch_rwsem and races against sync_inodes_sb().
If there are too much dirty data in the superblock, the waiting time may increase
significantly.

For superblocks without cgroup writeback such as tmpfs, they have nothing to
do with the wb swithes, so the flushing can be avoided.

Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
Suggested-by: Jan Kara <jack@suse.cz>
---
Changes since v1:
- do the check in cgroup_writeback_umount().
- check the capabilities of bdi.
---
 fs/fs-writeback.c         | 6 +++++-
 fs/super.c                | 2 +-
 include/linux/writeback.h | 4 ++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 92a5b8283528..09facd4356d9 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1140,8 +1140,12 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
  * rare occurrences and synchronize_rcu() can take a while, perform
  * flushing iff wb switches are in flight.
  */
-void cgroup_writeback_umount(void)
+void cgroup_writeback_umount(struct super_block *sb)
 {
+
+	if (!(sb->s_bdi->capabilities & BDI_CAP_WRITEBACK))
+		return;
+
 	/*
 	 * SB_ACTIVE should be reliably cleared before checking
 	 * isw_nr_in_flight, see generic_shutdown_super().
diff --git a/fs/super.c b/fs/super.c
index 095ba793e10c..acc16450da0e 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -621,7 +621,7 @@ void generic_shutdown_super(struct super_block *sb)
 		sync_filesystem(sb);
 		sb->s_flags &= ~SB_ACTIVE;
 
-		cgroup_writeback_umount();
+		cgroup_writeback_umount(sb);
 
 		/* Evict all inodes with zero refcount. */
 		evict_inodes(sb);
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 112d806ddbe4..d78d3dce4ede 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -217,7 +217,7 @@ void wbc_account_cgroup_owner(struct writeback_control *wbc, struct page *page,
 			      size_t bytes);
 int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
 			   enum wb_reason reason, struct wb_completion *done);
-void cgroup_writeback_umount(void);
+void cgroup_writeback_umount(struct super_block *sb);
 bool cleanup_offline_cgwb(struct bdi_writeback *wb);
 
 /**
@@ -324,7 +324,7 @@ static inline void wbc_account_cgroup_owner(struct writeback_control *wbc,
 {
 }
 
-static inline void cgroup_writeback_umount(void)
+static inline void cgroup_writeback_umount(struct super_block *sb)
 {
 }
 
-- 
2.25.1

Re: [PATCH v2] fs: don't flush in-flight wb switches for superblocks without cgroup writeback

[Christian Brauner]

On Fri, 26 Jul 2024 11:05:25 +0800, Haifeng Xu wrote:
> When deactivating any type of superblock, it had to wait for the in-flight
> wb switches to be completed. wb switches are executed in inode_switch_wbs_work_fn()
> which needs to acquire the wb_switch_rwsem and races against sync_inodes_sb().
> If there are too much dirty data in the superblock, the waiting time may increase
> significantly.
> 
> For superblocks without cgroup writeback such as tmpfs, they have nothing to
> do with the wb swithes, so the flushing can be avoided.
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] fs: don't flush in-flight wb switches for superblocks without cgroup writeback
      https://git.kernel.org/vfs/vfs/c/5f307d17dc72

Re: [PATCH v2] fs: don't flush in-flight wb switches for superblocks without cgroup writeback

[Jan Kara]

On Fri 26-07-24 11:05:25, Haifeng Xu wrote:
> When deactivating any type of superblock, it had to wait for the in-flight
> wb switches to be completed. wb switches are executed in inode_switch_wbs_work_fn()
> which needs to acquire the wb_switch_rwsem and races against sync_inodes_sb().
> If there are too much dirty data in the superblock, the waiting time may increase
> significantly.
> 
> For superblocks without cgroup writeback such as tmpfs, they have nothing to
> do with the wb swithes, so the flushing can be avoided.
> 
> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Looks good! Thanks! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> Changes since v1:
> - do the check in cgroup_writeback_umount().
> - check the capabilities of bdi.
> ---
>  fs/fs-writeback.c         | 6 +++++-
>  fs/super.c                | 2 +-
>  include/linux/writeback.h | 4 ++--
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 92a5b8283528..09facd4356d9 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1140,8 +1140,12 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>   * rare occurrences and synchronize_rcu() can take a while, perform
>   * flushing iff wb switches are in flight.
>   */
> -void cgroup_writeback_umount(void)
> +void cgroup_writeback_umount(struct super_block *sb)
>  {
> +
> +	if (!(sb->s_bdi->capabilities & BDI_CAP_WRITEBACK))
> +		return;
> +
>  	/*
>  	 * SB_ACTIVE should be reliably cleared before checking
>  	 * isw_nr_in_flight, see generic_shutdown_super().
> diff --git a/fs/super.c b/fs/super.c
> index 095ba793e10c..acc16450da0e 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -621,7 +621,7 @@ void generic_shutdown_super(struct super_block *sb)
>  		sync_filesystem(sb);
>  		sb->s_flags &= ~SB_ACTIVE;
>  
> -		cgroup_writeback_umount();
> +		cgroup_writeback_umount(sb);
>  
>  		/* Evict all inodes with zero refcount. */
>  		evict_inodes(sb);
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index 112d806ddbe4..d78d3dce4ede 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -217,7 +217,7 @@ void wbc_account_cgroup_owner(struct writeback_control *wbc, struct page *page,
>  			      size_t bytes);
>  int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>  			   enum wb_reason reason, struct wb_completion *done);
> -void cgroup_writeback_umount(void);
> +void cgroup_writeback_umount(struct super_block *sb);
>  bool cleanup_offline_cgwb(struct bdi_writeback *wb);
>  
>  /**
> @@ -324,7 +324,7 @@ static inline void wbc_account_cgroup_owner(struct writeback_control *wbc,
>  {
>  }
>  
> -static inline void cgroup_writeback_umount(void)
> +static inline void cgroup_writeback_umount(struct super_block *sb)
>  {
>  }
>  
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR