[PATCH] ext4_sb_breadahead_unmovable: Change to be no-blocking

[PATCH] ext4_sb_breadahead_unmovable: Change to be no-blocking

[Hui Zhu]

From: Hui Zhu <teawater@antgroup.com>

Encountered an issue where a large number of filesystem reads and writes
occurred suddenly within a container.  At the same time, other tasks on
the same host that were performing filesystem read and write operations
became blocked.  It was observed that many of the blocked tasks were
blocked on the ext4 journal lock. For example:
PID: 171453 TASK: ffff926566c9440 CPU: 54 COMMAND: "Thread"

Meanwhile, it was observed that the task holding the ext4 journal lock
was blocked for an extended period of time on "shrink_page_list" due to
"ext4_sb_breadahead_unmovable".

The function "grow_dev_page" increased the gfp mask with "__GFP_NOFAIL",
causing longer blocking times.
	/*
	 * XXX: __getblk_slow() can not really deal with failure and
	 * will endlessly loop on improvised global reclaim.  Prefer
	 * looping in the allocator rather than here, at least that
	 * code knows what it's doing.
	 */
	gfp_mask |= __GFP_NOFAIL;
However, "ext4_sb_breadahead_unmovable" is a prefetch function and
failures are acceptable.

Therefore, this commit changes "ext4_sb_breadahead_unmovable" to be
non-blocking, removing "__GFP_DIRECT_RECLAIM" from the gfp mask in the
"grow_dev_page" function if caller is ext4_sb_breadahead_unmovable to
alleviate memory-related blocking issues.

Signed-off-by: Hui Zhu <teawater@antgroup.com>
---
 fs/buffer.c                 | 41 +++++++++++++++++++++++--------------
 fs/ext4/super.c             |  2 +-
 include/linux/buffer_head.h | 17 ++++++++++-----
 3 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index bd091329026c..1086da366392 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1041,7 +1041,7 @@ static sector_t folio_init_buffers(struct folio *folio,
  */
 static int
 grow_dev_page(struct block_device *bdev, sector_t block,
-	      pgoff_t index, int size, int sizebits, gfp_t gfp)
+	      pgoff_t index, int size, int sizebits, gfp_t gfp, bool noblocking)
 {
 	struct inode *inode = bdev->bd_inode;
 	struct folio *folio;
@@ -1052,16 +1052,24 @@ grow_dev_page(struct block_device *bdev, sector_t block,
 
 	gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS) | gfp;
 
-	/*
-	 * XXX: __getblk_slow() can not really deal with failure and
-	 * will endlessly loop on improvised global reclaim.  Prefer
-	 * looping in the allocator rather than here, at least that
-	 * code knows what it's doing.
-	 */
-	gfp_mask |= __GFP_NOFAIL;
+	if (noblocking)
+		gfp_mask &= ~__GFP_DIRECT_RECLAIM;
+	else {
+		/*
+		 * XXX: __getblk_slow() can not really deal with failure and
+		 * will endlessly loop on improvised global reclaim.  Prefer
+		 * looping in the allocator rather than here, at least that
+		 * code knows what it's doing.
+		 */
+		gfp_mask |= __GFP_NOFAIL;
+	}
 
 	folio = __filemap_get_folio(inode->i_mapping, index,
 			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp_mask);
+	if (IS_ERR(folio)) {
+		ret = PTR_ERR(folio);
+		goto out;
+	}
 
 	bh = folio_buffers(folio);
 	if (bh) {
@@ -1091,6 +1099,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
 failed:
 	folio_unlock(folio);
 	folio_put(folio);
+out:
 	return ret;
 }
 
@@ -1099,7 +1108,8 @@ grow_dev_page(struct block_device *bdev, sector_t block,
  * that page was dirty, the buffers are set dirty also.
  */
 static int
-grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
+grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp,
+	     bool noblocking)
 {
 	pgoff_t index;
 	int sizebits;
@@ -1120,12 +1130,13 @@ grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
 	}
 
 	/* Create a page with the proper size buffers.. */
-	return grow_dev_page(bdev, block, index, size, sizebits, gfp);
+	return grow_dev_page(bdev, block, index, size, sizebits, gfp,
+			     noblocking);
 }
 
 static struct buffer_head *
 __getblk_slow(struct block_device *bdev, sector_t block,
-	     unsigned size, gfp_t gfp)
+	      unsigned size, gfp_t gfp, bool noblocking)
 {
 	/* Size must be multiple of hard sectorsize */
 	if (unlikely(size & (bdev_logical_block_size(bdev)-1) ||
@@ -1147,7 +1158,7 @@ __getblk_slow(struct block_device *bdev, sector_t block,
 		if (bh)
 			return bh;
 
-		ret = grow_buffers(bdev, block, size, gfp);
+		ret = grow_buffers(bdev, block, size, gfp, noblocking);
 		if (ret < 0)
 			return NULL;
 	}
@@ -1436,13 +1447,13 @@ EXPORT_SYMBOL(__find_get_block);
  */
 struct buffer_head *
 __getblk_gfp(struct block_device *bdev, sector_t block,
-	     unsigned size, gfp_t gfp)
+	     unsigned size, gfp_t gfp, bool noblocking)
 {
 	struct buffer_head *bh = __find_get_block(bdev, block, size);
 
 	might_sleep();
 	if (bh == NULL)
-		bh = __getblk_slow(bdev, block, size, gfp);
+		bh = __getblk_slow(bdev, block, size, gfp, noblocking);
 	return bh;
 }
 EXPORT_SYMBOL(__getblk_gfp);
@@ -1476,7 +1487,7 @@ struct buffer_head *
 __bread_gfp(struct block_device *bdev, sector_t block,
 		   unsigned size, gfp_t gfp)
 {
-	struct buffer_head *bh = __getblk_gfp(bdev, block, size, gfp);
+	struct buffer_head *bh = __getblk_gfp(bdev, block, size, gfp, false);
 
 	if (likely(bh) && !buffer_uptodate(bh))
 		bh = __bread_slow(bh);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c94ebf704616..d13b6f5c21eb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -254,7 +254,7 @@ struct buffer_head *ext4_sb_bread_unmovable(struct super_block *sb,
 
 void ext4_sb_breadahead_unmovable(struct super_block *sb, sector_t block)
 {
-	struct buffer_head *bh = sb_getblk_gfp(sb, block, 0);
+	struct buffer_head *bh = sb_getblk_noblocking(sb, block);
 
 	if (likely(bh)) {
 		if (trylock_buffer(bh))
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 6cb3e9af78c9..50cb0cc81962 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -234,7 +234,7 @@ wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
 struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
 			unsigned size);
 struct buffer_head *__getblk_gfp(struct block_device *bdev, sector_t block,
-				  unsigned size, gfp_t gfp);
+				  unsigned size, gfp_t gfp, bool noblocking);
 void __brelse(struct buffer_head *);
 void __bforget(struct buffer_head *);
 void __breadahead(struct block_device *, sector_t block, unsigned int size);
@@ -363,17 +363,24 @@ sb_breadahead(struct super_block *sb, sector_t block)
 	__breadahead(sb->s_bdev, block, sb->s_blocksize);
 }
 
+static inline struct buffer_head *
+sb_getblk_noblocking(struct super_block *sb, sector_t block)
+{
+	return __getblk_gfp(sb->s_bdev, block, sb->s_blocksize, 0, true);
+}
+
 static inline struct buffer_head *
 sb_getblk(struct super_block *sb, sector_t block)
 {
-	return __getblk_gfp(sb->s_bdev, block, sb->s_blocksize, __GFP_MOVABLE);
+	return __getblk_gfp(sb->s_bdev, block, sb->s_blocksize, __GFP_MOVABLE,
+			    false);
 }
 
 
 static inline struct buffer_head *
 sb_getblk_gfp(struct super_block *sb, sector_t block, gfp_t gfp)
 {
-	return __getblk_gfp(sb->s_bdev, block, sb->s_blocksize, gfp);
+	return __getblk_gfp(sb->s_bdev, block, sb->s_blocksize, gfp, false);
 }
 
 static inline struct buffer_head *
@@ -414,14 +421,14 @@ static inline struct buffer_head *getblk_unmovable(struct block_device *bdev,
 						   sector_t block,
 						   unsigned size)
 {
-	return __getblk_gfp(bdev, block, size, 0);
+	return __getblk_gfp(bdev, block, size, 0, false);
 }
 
 static inline struct buffer_head *__getblk(struct block_device *bdev,
 					   sector_t block,
 					   unsigned size)
 {
-	return __getblk_gfp(bdev, block, size, __GFP_MOVABLE);
+	return __getblk_gfp(bdev, block, size, __GFP_MOVABLE, false);
 }
 
 static inline void bh_readahead(struct buffer_head *bh, blk_opf_t op_flags)
-- 
2.19.1.6.gb485710b

Re: [PATCH] ext4_sb_breadahead_unmovable: Change to be no-blocking

[Matthew Wilcox]

On Fri, Aug 11, 2023 at 03:57:05AM +0000, Hui Zhu wrote:
> Encountered an issue where a large number of filesystem reads and writes
> occurred suddenly within a container.  At the same time, other tasks on
> the same host that were performing filesystem read and write operations
> became blocked.  It was observed that many of the blocked tasks were
> blocked on the ext4 journal lock. For example:
> PID: 171453 TASK: ffff926566c9440 CPU: 54 COMMAND: "Thread"
> 
> Meanwhile, it was observed that the task holding the ext4 journal lock
> was blocked for an extended period of time on "shrink_page_list" due to
> "ext4_sb_breadahead_unmovable".
> 
> The function "grow_dev_page" increased the gfp mask with "__GFP_NOFAIL",
> causing longer blocking times.
> 	/*
> 	 * XXX: __getblk_slow() can not really deal with failure and
> 	 * will endlessly loop on improvised global reclaim.  Prefer
> 	 * looping in the allocator rather than here, at least that
> 	 * code knows what it's doing.
> 	 */
> 	gfp_mask |= __GFP_NOFAIL;
> However, "ext4_sb_breadahead_unmovable" is a prefetch function and
> failures are acceptable.

That's a really good point.

> Therefore, this commit changes "ext4_sb_breadahead_unmovable" to be
> non-blocking, removing "__GFP_DIRECT_RECLAIM" from the gfp mask in the
> "grow_dev_page" function if caller is ext4_sb_breadahead_unmovable to
> alleviate memory-related blocking issues.

Uh, not like this though.  Fix the gfp flags in the callers instead of
working this new "bool" flag through the buffer head layers.

[PATCH] ext4_sb_breadahead_unmovable: Change to be no-blocking

[Hui Zhu]

From: Hui Zhu <teawater@antgroup.com>

This version fix the gfp flags in the callers instead of working this
new "bool" flag through the buffer head layers according to the comments
from Matthew Wilcox.

Encountered an issue where a large number of filesystem reads and writes
occurred suddenly within a container.  At the same time, other tasks on
the same host that were performing filesystem read and write operations
became blocked.  It was observed that many of the blocked tasks were
blocked on the ext4 journal lock. For example:
PID: 171453 TASK: ffff926566c9440 CPU: 54 COMMAND: "Thread"
0 [] __schedule at xxxxxxxxxxxxxxx
1 [] schedule at xxxxxxxxxxxxxxx
2 [] wait_transaction_locked at xxxxxxxxxxxxxxx
3 [] add_transaction_credits at xxxxxxxxxxxxxxx
4 [] start_this_handle at xxxxxxxxxxxxxxx
5 [] jbd2__journal_start at xxxxxxxxxxxxxxx
6 [] ext4_journal_start_sb at xxxxxxxxxxxxxxx
7 [] ext4_dirty_inode at xxxxxxxxxxxxxxx
8 [] mark_inode_dirty at xxxxxxxxxxxxxxx
9 [] generic_update_time at xxxxxxxxxxxxxxx

Meanwhile, it was observed that the task holding the ext4 journal lock
was blocked for an extended period of time on "shrink_page_list" due to
"ext4_sb_breadahead_unmovable".
0 [] __schedule at xxxxxxxxxxxxxxx
1 [] _cond_resched at xxxxxxxxxxxxxxx
2 [] shrink_page_list at xxxxxxxxxxxxxxx
3 [] shrink_inactive_list at xxxxxxxxxxxxxxx
4 [] shrink_lruvec at xxxxxxxxxxxxxxx
5 [] shrink_node_memcgs at xxxxxxxxxxxxxxx
6 [] shrink_node at xxxxxxxxxxxxxxx
7 [] shrink_zones at xxxxxxxxxxxxxxx
8 [] do_try_to_free_pages at xxxxxxxxxxxxxxx
9 [] try_to_free_mem_cgroup_pages at xxxxxxxxxxxxxxx
10 [] try_charge at xxxxxxxxxxxxxxx
11 [] mem_cgroup_charge at xxxxxxxxxxxxxxx
12 [] __add_to_page_cache_locked at xxxxxxxxxxxxxxx
13 [] add_to_page_cache_lru at xxxxxxxxxxxxxxx
14 [] pagecache_get_page at xxxxxxxxxxxxxxx
15 [] grow_dev_page at xxxxxxxxxxxxxxx
16 [] __getblk_slow at xxxxxxxxxxxxxxx
17 [] ext4_sb_breadahead_unmovable at xxxxxxxxxxxxxxx
18 [] __ext4_get_inode_loc at xxxxxxxxxxxxxxx
19 [] ext4_get_inode_loc at xxxxxxxxxxxxxxx
20 [] ext4_reserve_inode_write at xxxxxxxxxxxxxxx
21 [] __ext4_mark_inode_dirty at xxxxxxxxxxxxxxx
22 [] add_dirent_to_buf at xxxxxxxxxxxxxxx
23 [] ext4_add_entry at xxxxxxxxxxxxxxx
24 [] ext4_add_nondir at xxxxxxxxxxxxxxx
25 [] ext4_create at xxxxxxxxxxxxxxx
26 [] vfs_create at xxxxxxxxxxxxxxx

The function "grow_dev_page" increased the gfp mask with "__GFP_NOFAIL",
causing longer blocking times.
	/*
	 * XXX: __getblk_slow() can not really deal with failure and
	 * will endlessly loop on improvised global reclaim.  Prefer
	 * looping in the allocator rather than here, at least that
	 * code knows what it's doing.
	 */
	gfp_mask |= __GFP_NOFAIL;
However, "ext4_sb_breadahead_unmovable" is a prefetch function and
failures are acceptable.

Therefore, this commit changes "ext4_sb_breadahead_unmovable" to be
non-blocking.
Change gfp to ~__GFP_DIRECT_RECLAIM when ext4_sb_breadahead_unmovable
calls sb_getblk_gfp.
Modify grow_dev_page to will not be blocked by the allocation of folio
if gfp is ~__GFP_DIRECT_RECLAIM.

Signed-off-by: Hui Zhu <teawater@antgroup.com>
---
 fs/buffer.c     | 27 +++++++++++++++++++--------
 fs/ext4/super.c |  3 ++-
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index bd091329026c..330cf19c77b1 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1038,6 +1038,7 @@ static sector_t folio_init_buffers(struct folio *folio,
  * Create the page-cache page that contains the requested block.
  *
  * This is used purely for blockdev mappings.
+ * Will not blocking by allocate folio if gfp is ~__GFP_DIRECT_RECLAIM.
  */
 static int
 grow_dev_page(struct block_device *bdev, sector_t block,
@@ -1050,18 +1051,27 @@ grow_dev_page(struct block_device *bdev, sector_t block,
 	int ret = 0;
 	gfp_t gfp_mask;
 
-	gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS) | gfp;
+	gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS);
+	if (gfp == ~__GFP_DIRECT_RECLAIM)
+		gfp_mask &= ~__GFP_DIRECT_RECLAIM;
+	else {
+		gfp_mask |= gfp;
 
-	/*
-	 * XXX: __getblk_slow() can not really deal with failure and
-	 * will endlessly loop on improvised global reclaim.  Prefer
-	 * looping in the allocator rather than here, at least that
-	 * code knows what it's doing.
-	 */
-	gfp_mask |= __GFP_NOFAIL;
+		/*
+		 * XXX: __getblk_slow() can not really deal with failure and
+		 * will endlessly loop on improvised global reclaim.  Prefer
+		 * looping in the allocator rather than here, at least that
+		 * code knows what it's doing.
+		 */
+		gfp_mask |= __GFP_NOFAIL;
+	}
 
 	folio = __filemap_get_folio(inode->i_mapping, index,
 			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp_mask);
+	if (IS_ERR(folio)) {
+		ret = PTR_ERR(folio);
+		goto out;
+	}
 
 	bh = folio_buffers(folio);
 	if (bh) {
@@ -1091,6 +1101,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
 failed:
 	folio_unlock(folio);
 	folio_put(folio);
+out:
 	return ret;
 }
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c94ebf704616..6a529509b83b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -254,7 +254,8 @@ struct buffer_head *ext4_sb_bread_unmovable(struct super_block *sb,
 
 void ext4_sb_breadahead_unmovable(struct super_block *sb, sector_t block)
 {
-	struct buffer_head *bh = sb_getblk_gfp(sb, block, 0);
+	struct buffer_head *bh = sb_getblk_gfp(sb, block,
+					       ~__GFP_DIRECT_RECLAIM);
 
 	if (likely(bh)) {
 		if (trylock_buffer(bh))
-- 
2.19.1.6.gb485710b

Re: [PATCH] ext4_sb_breadahead_unmovable: Change to be no-blocking

[Hui Zhu]

Oops, this is the version 2 of the patch.

Best,
Hui

Hui Zhu <teawaterz@linux.alibaba.com> 于2023年8月11日周五 15:15写道：
>
> From: Hui Zhu <teawater@antgroup.com>
>
> This version fix the gfp flags in the callers instead of working this
> new "bool" flag through the buffer head layers according to the comments
> from Matthew Wilcox.
>
> Encountered an issue where a large number of filesystem reads and writes
> occurred suddenly within a container.  At the same time, other tasks on
> the same host that were performing filesystem read and write operations
> became blocked.  It was observed that many of the blocked tasks were
> blocked on the ext4 journal lock. For example:
> PID: 171453 TASK: ffff926566c9440 CPU: 54 COMMAND: "Thread"
> 0 [] __schedule at xxxxxxxxxxxxxxx
> 1 [] schedule at xxxxxxxxxxxxxxx
> 2 [] wait_transaction_locked at xxxxxxxxxxxxxxx
> 3 [] add_transaction_credits at xxxxxxxxxxxxxxx
> 4 [] start_this_handle at xxxxxxxxxxxxxxx
> 5 [] jbd2__journal_start at xxxxxxxxxxxxxxx
> 6 [] ext4_journal_start_sb at xxxxxxxxxxxxxxx
> 7 [] ext4_dirty_inode at xxxxxxxxxxxxxxx
> 8 [] mark_inode_dirty at xxxxxxxxxxxxxxx
> 9 [] generic_update_time at xxxxxxxxxxxxxxx
>
> Meanwhile, it was observed that the task holding the ext4 journal lock
> was blocked for an extended period of time on "shrink_page_list" due to
> "ext4_sb_breadahead_unmovable".
> 0 [] __schedule at xxxxxxxxxxxxxxx
> 1 [] _cond_resched at xxxxxxxxxxxxxxx
> 2 [] shrink_page_list at xxxxxxxxxxxxxxx
> 3 [] shrink_inactive_list at xxxxxxxxxxxxxxx
> 4 [] shrink_lruvec at xxxxxxxxxxxxxxx
> 5 [] shrink_node_memcgs at xxxxxxxxxxxxxxx
> 6 [] shrink_node at xxxxxxxxxxxxxxx
> 7 [] shrink_zones at xxxxxxxxxxxxxxx
> 8 [] do_try_to_free_pages at xxxxxxxxxxxxxxx
> 9 [] try_to_free_mem_cgroup_pages at xxxxxxxxxxxxxxx
> 10 [] try_charge at xxxxxxxxxxxxxxx
> 11 [] mem_cgroup_charge at xxxxxxxxxxxxxxx
> 12 [] __add_to_page_cache_locked at xxxxxxxxxxxxxxx
> 13 [] add_to_page_cache_lru at xxxxxxxxxxxxxxx
> 14 [] pagecache_get_page at xxxxxxxxxxxxxxx
> 15 [] grow_dev_page at xxxxxxxxxxxxxxx
> 16 [] __getblk_slow at xxxxxxxxxxxxxxx
> 17 [] ext4_sb_breadahead_unmovable at xxxxxxxxxxxxxxx
> 18 [] __ext4_get_inode_loc at xxxxxxxxxxxxxxx
> 19 [] ext4_get_inode_loc at xxxxxxxxxxxxxxx
> 20 [] ext4_reserve_inode_write at xxxxxxxxxxxxxxx
> 21 [] __ext4_mark_inode_dirty at xxxxxxxxxxxxxxx
> 22 [] add_dirent_to_buf at xxxxxxxxxxxxxxx
> 23 [] ext4_add_entry at xxxxxxxxxxxxxxx
> 24 [] ext4_add_nondir at xxxxxxxxxxxxxxx
> 25 [] ext4_create at xxxxxxxxxxxxxxx
> 26 [] vfs_create at xxxxxxxxxxxxxxx
>
> The function "grow_dev_page" increased the gfp mask with "__GFP_NOFAIL",
> causing longer blocking times.
>         /*
>          * XXX: __getblk_slow() can not really deal with failure and
>          * will endlessly loop on improvised global reclaim.  Prefer
>          * looping in the allocator rather than here, at least that
>          * code knows what it's doing.
>          */
>         gfp_mask |= __GFP_NOFAIL;
> However, "ext4_sb_breadahead_unmovable" is a prefetch function and
> failures are acceptable.
>
> Therefore, this commit changes "ext4_sb_breadahead_unmovable" to be
> non-blocking.
> Change gfp to ~__GFP_DIRECT_RECLAIM when ext4_sb_breadahead_unmovable
> calls sb_getblk_gfp.
> Modify grow_dev_page to will not be blocked by the allocation of folio
> if gfp is ~__GFP_DIRECT_RECLAIM.
>
> Signed-off-by: Hui Zhu <teawater@antgroup.com>
> ---
>  fs/buffer.c     | 27 +++++++++++++++++++--------
>  fs/ext4/super.c |  3 ++-
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index bd091329026c..330cf19c77b1 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1038,6 +1038,7 @@ static sector_t folio_init_buffers(struct folio *folio,
>   * Create the page-cache page that contains the requested block.
>   *
>   * This is used purely for blockdev mappings.
> + * Will not blocking by allocate folio if gfp is ~__GFP_DIRECT_RECLAIM.
>   */
>  static int
>  grow_dev_page(struct block_device *bdev, sector_t block,
> @@ -1050,18 +1051,27 @@ grow_dev_page(struct block_device *bdev, sector_t block,
>         int ret = 0;
>         gfp_t gfp_mask;
>
> -       gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS) | gfp;
> +       gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS);
> +       if (gfp == ~__GFP_DIRECT_RECLAIM)
> +               gfp_mask &= ~__GFP_DIRECT_RECLAIM;
> +       else {
> +               gfp_mask |= gfp;
>
> -       /*
> -        * XXX: __getblk_slow() can not really deal with failure and
> -        * will endlessly loop on improvised global reclaim.  Prefer
> -        * looping in the allocator rather than here, at least that
> -        * code knows what it's doing.
> -        */
> -       gfp_mask |= __GFP_NOFAIL;
> +               /*
> +                * XXX: __getblk_slow() can not really deal with failure and
> +                * will endlessly loop on improvised global reclaim.  Prefer
> +                * looping in the allocator rather than here, at least that
> +                * code knows what it's doing.
> +                */
> +               gfp_mask |= __GFP_NOFAIL;
> +       }
>
>         folio = __filemap_get_folio(inode->i_mapping, index,
>                         FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp_mask);
> +       if (IS_ERR(folio)) {
> +               ret = PTR_ERR(folio);
> +               goto out;
> +       }
>
>         bh = folio_buffers(folio);
>         if (bh) {
> @@ -1091,6 +1101,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
>  failed:
>         folio_unlock(folio);
>         folio_put(folio);
> +out:
>         return ret;
>  }
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c94ebf704616..6a529509b83b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -254,7 +254,8 @@ struct buffer_head *ext4_sb_bread_unmovable(struct super_block *sb,
>
>  void ext4_sb_breadahead_unmovable(struct super_block *sb, sector_t block)
>  {
> -       struct buffer_head *bh = sb_getblk_gfp(sb, block, 0);
> +       struct buffer_head *bh = sb_getblk_gfp(sb, block,
> +                                              ~__GFP_DIRECT_RECLAIM);
>
>         if (likely(bh)) {
>                 if (trylock_buffer(bh))
> --
> 2.19.1.6.gb485710b
>

Re: [PATCH] ext4_sb_breadahead_unmovable: Change to be no-blocking

[Matthew Wilcox]

On Fri, Aug 11, 2023 at 07:15:19AM +0000, Hui Zhu wrote:
> From: Hui Zhu <teawater@antgroup.com>
> 
> This version fix the gfp flags in the callers instead of working this
> new "bool" flag through the buffer head layers according to the comments
> from Matthew Wilcox.

FYI, this paragraph should have been below the --- so it gets excluded
from the commit log.

> Meanwhile, it was observed that the task holding the ext4 journal lock
> was blocked for an extended period of time on "shrink_page_list" due to
> "ext4_sb_breadahead_unmovable".
> 0 [] __schedule at xxxxxxxxxxxxxxx
> 1 [] _cond_resched at xxxxxxxxxxxxxxx
> 2 [] shrink_page_list at xxxxxxxxxxxxxxx
> 3 [] shrink_inactive_list at xxxxxxxxxxxxxxx
> 4 [] shrink_lruvec at xxxxxxxxxxxxxxx
> 5 [] shrink_node_memcgs at xxxxxxxxxxxxxxx
> 6 [] shrink_node at xxxxxxxxxxxxxxx
> 7 [] shrink_zones at xxxxxxxxxxxxxxx
> 8 [] do_try_to_free_pages at xxxxxxxxxxxxxxx
> 9 [] try_to_free_mem_cgroup_pages at xxxxxxxxxxxxxxx
> 10 [] try_charge at xxxxxxxxxxxxxxx
> 11 [] mem_cgroup_charge at xxxxxxxxxxxxxxx
> 12 [] __add_to_page_cache_locked at xxxxxxxxxxxxxxx
> 13 [] add_to_page_cache_lru at xxxxxxxxxxxxxxx
> 14 [] pagecache_get_page at xxxxxxxxxxxxxxx
> 15 [] grow_dev_page at xxxxxxxxxxxxxxx

After applying your patch, we'd still get into trouble with
folio_alloc_buffers() also specifying __GFP_NOWAIT.  So I decided
to pass the GFP flags into folio_alloc_buffers() -- see the patch
series I just sent out.

> @@ -1050,18 +1051,27 @@ grow_dev_page(struct block_device *bdev, sector_t block,
>  	int ret = 0;
>  	gfp_t gfp_mask;
>  
> -	gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS) | gfp;
> +	gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS);
> +	if (gfp == ~__GFP_DIRECT_RECLAIM)
> +		gfp_mask &= ~__GFP_DIRECT_RECLAIM;

This isn't how we normally use gfp_mask.  OTOH, how buffer.c uses GFP
masks is also a bit weird.  The bdev_getblk() I just added is more
normal.

Please try the patchset I cc'd you on (with the __GFP_ACCOUNT added);
I'm currently running it through xfstests and it's holding up fine.
I suppose I should play around with memcgs to try to make it happen a
bit more often.