[PATCH 1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim

[PATCH 1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim

[Yang Shi]

direct reclaim doesn't write out filesystem page, only kswapd could do
it. So, if the call comes from direct reclaim, it is definitely a bug.

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

Although it is for xfs, ext4 has the similar behavior, so elevate
WARN_ON to BUG_ON.

And, correct the comment accordingly.

Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 fs/ext4/inode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2ea07ef..089e388 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2071,7 +2071,7 @@ static int __ext4_journalled_writepage(struct page *page,
  * This function can get called via...
  *   - ext4_writepages after taking page lock (have journal handle)
  *   - journal_submit_inode_data_buffers (no journal handle)
- *   - shrink_page_list via the kswapd/direct reclaim (no journal handle)
+ *   - shrink_page_list via the kswapd (no journal handle)
  *   - grab_page_cache when doing write_begin (have journal handle)
  *
  * We don't do any block allocation in this function. If we have page with
@@ -2148,10 +2148,10 @@ static int ext4_writepage(struct page *page,
 		    (inode->i_sb->s_blocksize == PAGE_SIZE)) {
 			/*
 			 * For memory cleaning there's no point in writing only
-			 * some buffers. So just bail out. Warn if we came here
-			 * from direct reclaim.
+			 * some buffers. So just bail out. It is a bug if we
+			 * came here from direct reclaim.
 			 */
-			WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD))
+			BUG_ON((current->flags & (PF_MEMALLOC|PF_KSWAPD))
 							== PF_MEMALLOC);
 			unlock_page(page);
 			return 0;
-- 
1.8.3.1

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

[Yang Shi]

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);
 
 	/*
 	 * Given that we do not allow direct reclaim to call us, we should
-- 
1.8.3.1

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

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim

[Theodore Y. Ts'o]

On Tue, Jul 03, 2018 at 12:11:18PM +0800, Yang Shi wrote:
> direct reclaim doesn't write out filesystem page, only kswapd could do
> it. So, if the call comes from direct reclaim, it is definitely a bug.
> 
> And, Mel Gormane also mentioned "Ultimately, this will be a BUG_ON." In
> commit 94054fa3fca1fd78db02cb3d68d5627120f0a1d4 ("xfs: warn if direct
> reclaim tries to writeback pages").
> 
> Although it is for xfs, ext4 has the similar behavior, so elevate
> WARN_ON to BUG_ON.
> 
> And, correct the comment accordingly.
> 
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>

What's the upside of crashing the kernel if the file sytsem can handle it?

       	   	     	      	  	    - Ted

Re: [PATCH 1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim

[Yang Shi]

On 7/3/18 3:39 AM, Theodore Y. Ts'o wrote:
> On Tue, Jul 03, 2018 at 12:11:18PM +0800, Yang Shi wrote:
>> direct reclaim doesn't write out filesystem page, only kswapd could do
>> it. So, if the call comes from direct reclaim, it is definitely a bug.
>>
>> And, Mel Gormane also mentioned "Ultimately, this will be a BUG_ON." In
>> commit 94054fa3fca1fd78db02cb3d68d5627120f0a1d4 ("xfs: warn if direct
>> reclaim tries to writeback pages").
>>
>> Although it is for xfs, ext4 has the similar behavior, so elevate
>> WARN_ON to BUG_ON.
>>
>> And, correct the comment accordingly.
>>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: "Theodore Ts'o" <tytso@mit.edu>
>> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> What's the upside of crashing the kernel if the file sytsem can handle it?

I'm not sure if it is a good choice to let filesystem handle such vital 
VM regression. IMHO, writing out filesystem page from direct reclaim 
context is a vital VM bug. It means something is definitely wrong in VM. 
It should never happen.

It sounds ok to have filesystem throw out warning and handle it, but I'm 
not sure if someone will just ignore the warning, but it should *never* 
be ignored.

Yang

>
>         	   	     	      	  	    - Ted

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

[Darrick J. Wong]

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

Re: [PATCH 1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim

[Yang Shi]

On 7/3/18 10:05 AM, Yang Shi wrote:
>
>
> On 7/3/18 3:39 AM, Theodore Y. Ts'o wrote:
>> On Tue, Jul 03, 2018 at 12:11:18PM +0800, Yang Shi wrote:
>>> direct reclaim doesn't write out filesystem page, only kswapd could do
>>> it. So, if the call comes from direct reclaim, it is definitely a bug.
>>>
>>> And, Mel Gormane also mentioned "Ultimately, this will be a BUG_ON." In
>>> commit 94054fa3fca1fd78db02cb3d68d5627120f0a1d4 ("xfs: warn if direct
>>> reclaim tries to writeback pages").
>>>
>>> Although it is for xfs, ext4 has the similar behavior, so elevate
>>> WARN_ON to BUG_ON.
>>>
>>> And, correct the comment accordingly.
>>>
>>> Cc: Mel Gorman <mgorman@techsingularity.net>
>>> Cc: "Theodore Ts'o" <tytso@mit.edu>
>>> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> What's the upside of crashing the kernel if the file sytsem can 
>> handle it?

BTW, the comment does sound misleading. Direct reclaim is not a 
legitimate context to call writepage. I'd like to correct at least.

Thanks,
Yang

>
> I'm not sure if it is a good choice to let filesystem handle such 
> vital VM regression. IMHO, writing out filesystem page from direct 
> reclaim context is a vital VM bug. It means something is definitely 
> wrong in VM. It should never happen.
>
> It sounds ok to have filesystem throw out warning and handle it, but 
> I'm not sure if someone will just ignore the warning, but it should 
> *never* be ignored.
>
> Yang
>
>>
>>                                                 - Ted
>

Re: [PATCH 1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim

[Theodore Y. Ts'o]

On Tue, Jul 03, 2018 at 10:05:04AM -0700, Yang Shi wrote:
> I'm not sure if it is a good choice to let filesystem handle such vital VM
> regression. IMHO, writing out filesystem page from direct reclaim context is
> a vital VM bug. It means something is definitely wrong in VM. It should
> never happen.

If it does happen, it should happen reliably; this isn't the sort of
thing where some linked list had gotten corrupted.  This would be a
structural problem in the VM code.

So presumably, if the WARN_ON triggered, it should be be noticed by VM
developers, and they should fix it.

In general, though, BUG_ON's should be avoided unless there really is
no way to recover.

> It sounds ok to have filesystem throw out warning and handle it, but I'm not
> sure if someone will just ignore the warning, but it should *never* be
> ignored.

If a kernel develper (a VM developer in this case) ignores a warning,
that's just simply professional malpractice.  In general WARN_ON's
should only be used as a sign of a kernel bug.  So they should never
be ignored.

						- Ted

Re: [PATCH 1/2] fs: ext4: use BUG_ON if writepage call comes from direct reclaim

[Michal Hocko]

On Tue 03-07-18 10:05:04, Yang Shi wrote:
> 
> 
> On 7/3/18 3:39 AM, Theodore Y. Ts'o wrote:
> > On Tue, Jul 03, 2018 at 12:11:18PM +0800, Yang Shi wrote:
> > > direct reclaim doesn't write out filesystem page, only kswapd could do
> > > it. So, if the call comes from direct reclaim, it is definitely a bug.
> > > 
> > > And, Mel Gormane also mentioned "Ultimately, this will be a BUG_ON." In
> > > commit 94054fa3fca1fd78db02cb3d68d5627120f0a1d4 ("xfs: warn if direct
> > > reclaim tries to writeback pages").
> > > 
> > > Although it is for xfs, ext4 has the similar behavior, so elevate
> > > WARN_ON to BUG_ON.
> > > 
> > > And, correct the comment accordingly.
> > > 
> > > Cc: Mel Gorman <mgorman@techsingularity.net>
> > > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> > What's the upside of crashing the kernel if the file sytsem can handle it?
> 
> I'm not sure if it is a good choice to let filesystem handle such vital VM
> regression. IMHO, writing out filesystem page from direct reclaim context is
> a vital VM bug. It means something is definitely wrong in VM. It should
> never happen.

Could you be more specific about the vital part please? Issuing
writeback from the direct reclaim surely can be sub-optimal. But since
we have quite a large stacks it shouldn't overflow immediately even for
more complex storage setups. So what is the _vital_ bug here?
-- 
Michal Hocko
SUSE Labs