Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Yafang Shao <laoar.shao@gmail.com>
Cc: hch@infradead.org, darrick.wong@oracle.com, mhocko@kernel.org,
	willy@infradead.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	Yafang Shao <shaoyafang@didiglobal.com>
Subject: Re: [PATCH v4 1/2] xfs: avoid double restore PF_MEMALLOC_NOFS if transaction reservation fails
Date: Wed, 5 Aug 2020 09:20:05 +1000
Message-ID: <20200804232005.GD2114@dread.disaster.area> (raw)
In-Reply-To: <20200801154632.866356-2-laoar.shao@gmail.com>

On Sat, Aug 01, 2020 at 11:46:31AM -0400, Yafang Shao wrote:
> From: Yafang Shao <shaoyafang@didiglobal.com>
> 
> In xfs_trans_alloc(), if xfs_trans_reserve() fails, it will call
> xfs_trans_cancel(), in which it will restore the flag PF_MEMALLOC_NOFS.
> However this flags has been restored in xfs_trans_reserve(). Although
> this behavior doesn't introduce any obvious issue, we'd better improve it.
.....

> 
> Signed-off-by: Yafang Shao <shaoyafang@didiglobal.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> ---
>  fs/xfs/xfs_trans.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 3c94e5ff4316..9ff41970d0c7 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -162,10 +162,9 @@ xfs_trans_reserve(
>  	 */
>  	if (blocks > 0) {
>  		error = xfs_mod_fdblocks(mp, -((int64_t)blocks), rsvd);
> -		if (error != 0) {
> -			current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> +		if (error != 0)
>  			return -ENOSPC;
> -		}
> +
>  		tp->t_blk_res += blocks;
>  	}
>  
> @@ -240,8 +239,6 @@ xfs_trans_reserve(
>  		tp->t_blk_res = 0;
>  	}
>  
> -	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> -
>  	return error;
>  }

So you fix it here....

>  
> @@ -972,6 +969,7 @@ xfs_trans_roll(
>  	struct xfs_trans	**tpp)
>  {
>  	struct xfs_trans	*trans = *tpp;
> +	struct xfs_trans        *tp;
>  	struct xfs_trans_res	tres;
>  	int			error;
>  
> @@ -1005,5 +1003,10 @@ xfs_trans_roll(
>  	 * the prior and the next transactions.
>  	 */
>  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> -	return xfs_trans_reserve(*tpp, &tres, 0, 0);
> +	tp = *tpp;
> +	error = xfs_trans_reserve(tp, &tres, 0, 0);
> +	if (error)
> +		current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> +
> +	return error;
>  }

.... but then introduce the exact same issue here. i.e. when
xfs_trans_roll() fails, the caller will call xfs_trans_cancel() on
the returned transaction....

Realistically, I think this is kinda missing the overall intent of
rolling transactions. The issue here is that when we commit a
transaction, we unconditionally clear the PF_MEMALLOC_NOFS flag via
__xfs_trans_commit() just before freeing the transaction despite the
fact the long running rolling transaction has not yet completed.

This means that when we roll a transactions, we are bouncing the
NOFS state unnecessarily like so:

t0		t1		t2
alloc
 reserve
  NOFS
roll
		alloc
commit
clear NOFS
		reserve
		  NOFS
		roll
				alloc
		commit
		clear NOFS
				reserve
				  NOFS
				.....

right until the last commit of the sequence. IOWs, we are repeatedly
setting and clearing NOFS even though we actually only need to set
it at the t0 and clear it on the final commit or cancel.

Hence I think that __xfs_trans_commit() should probably only clear
NOFS on successful commit if @regrant is false (i.e. not called from
xfs_trans_roll()) and the setting of NOFS should be removed from
xfs_trans_reserve() and moved up into the initial xfs_trans_alloc()
before xfs_trans_reserve() is called.

This way the calls to either xfs_trans_commit() or
xfs_trans_cancel() will clear the NOFS state as they indicate that
we are exiting transaction context completely....

Also, please convert these to memalloc_nofs_save()/restore() calls
as that is the way we are supposed to mark these regions now.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-01 15:46 [PATCH v4 0/2] void xfs transaction reservation recursion Yafang Shao
2020-08-01 15:46 ` [PATCH v4 1/2] xfs: avoid double restore PF_MEMALLOC_NOFS if transaction reservation fails Yafang Shao
2020-08-04 23:20   ` Dave Chinner [this message]
2020-08-04 23:50     ` Matthew Wilcox
2020-08-05  1:28       ` Dave Chinner
2020-08-07  4:05         ` Yafang Shao
2020-08-01 15:46 ` [PATCH v4 2/2] xfs: avoid transaction reservation recursion Yafang Shao
2020-08-04 23:35   ` Dave Chinner
2020-08-07  4:11     ` Yafang Shao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200804232005.GD2114@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=darrick.wong@oracle.com \
    --cc=hch@infradead.org \
    --cc=laoar.shao@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=mhocko@kernel.org \
    --cc=shaoyafang@didiglobal.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git