[2/2] fs: xfs: use BUG_ON if writepage call comes from direct reclaim

Message ID 1530591079-33813-2-git-send-email-yang.shi@linux.alibaba.com
State New
Headers show
Series
  • [1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim
Related show

Commit Message

Yang Shi July 3, 2018, 4:11 a.m.
direct reclaim doesn't write out filesystem page, only kswapd could do
this. So, if it is called from direct relaim, it is definitely a bug.

And, Mel Gorman mentioned "Ultimately, this will be a BUG_ON." in commit
94054fa3fca1fd78db02cb3d68d5627120f0a1d4 ("xfs: warn if direct reclaim
tries to writeback pages"),

It has been many years since that commit, so it should be safe to
elevate WARN_ON to BUG_ON now.

Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 fs/xfs/xfs_aops.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Dave Chinner July 3, 2018, 4:37 a.m. | #1
On Tue, Jul 03, 2018 at 12:11:19PM +0800, Yang Shi wrote:
> direct reclaim doesn't write out filesystem page, only kswapd could do
> this. So, if it is called from direct relaim, it is definitely a bug.
> 
> And, Mel Gorman mentioned "Ultimately, this will be a BUG_ON." in commit
> 94054fa3fca1fd78db02cb3d68d5627120f0a1d4 ("xfs: warn if direct reclaim
> tries to writeback pages"),
> 
> It has been many years since that commit, so it should be safe to
> elevate WARN_ON to BUG_ON now.

NACK.

The existing code warns and then handles the situation gracefully -
this is the appropriate way to handle incorrect calling contexts.
There is absolutely no good reason to panic production kernels
in situations like this.

Cheers,

Dave.
Darrick J. Wong July 3, 2018, 5:19 p.m. | #2
On Tue, Jul 03, 2018 at 12:11:19PM +0800, Yang Shi wrote:
> direct reclaim doesn't write out filesystem page, only kswapd could do
> this. So, if it is called from direct relaim, it is definitely a bug.
> 
> And, Mel Gorman mentioned "Ultimately, this will be a BUG_ON." in commit
> 94054fa3fca1fd78db02cb3d68d5627120f0a1d4 ("xfs: warn if direct reclaim
> tries to writeback pages"),
> 
> It has been many years since that commit, so it should be safe to
> elevate WARN_ON to BUG_ON now.
> 
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>  fs/xfs/xfs_aops.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 8eb3ba3..7efc2d2 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -1080,11 +1080,9 @@ static inline int xfs_bio_add_buffer(struct bio *bio, struct buffer_head *bh)
>  	 * allow reclaim from kswapd as the stack usage there is relatively low.
>  	 *
>  	 * This should never happen except in the case of a VM regression so
> -	 * warn about it.
> +	 * BUG about it.
>  	 */
> -	if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD)) ==
> -			PF_MEMALLOC))
> -		goto redirty;
> +	BUG_ON((current->flags & (PF_MEMALLOC|PF_KSWAPD)) == PF_MEMALLOC);

Ugh, please do not increase the BUG() factor.  Even if this happens due
to a regression it's /much/ easier to debug if we don't halt the system.

(IOWs, I decline to take this patch.)

--D

>  
>  	/*
>  	 * Given that we do not allow direct reclaim to call us, we should
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 8eb3ba3..7efc2d2 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -1080,11 +1080,9 @@  static inline int xfs_bio_add_buffer(struct bio *bio, struct buffer_head *bh)
 	 * allow reclaim from kswapd as the stack usage there is relatively low.
 	 *
 	 * This should never happen except in the case of a VM regression so
-	 * warn about it.
+	 * BUG about it.
 	 */
-	if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD)) ==
-			PF_MEMALLOC))
-		goto redirty;
+	BUG_ON((current->flags & (PF_MEMALLOC|PF_KSWAPD)) == PF_MEMALLOC);
 
 	/*
 	 * Given that we do not allow direct reclaim to call us, we should