Re: [PATCH 4/3] xfs: attach dquots to inode before reading data/cow fork mappings

[Dave Chinner <david@fromorbit.com>]

On Sun, Nov 27, 2022 at 10:36:29AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> I've been running near-continuous integration testing of online fsck,
> and I've noticed that once a day, one of the ARM VMs will fail the test
> with out of order records in the data fork.
> 
> xfs/804 races fsstress with online scrub (aka scan but do not change
> anything), so I think this might be a bug in the core xfs code.  This
> also only seems to trigger if one runs the test for more than ~6 minutes
> via TIME_FACTOR=13 or something.
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/tree/tests/xfs/804?h=djwong-wtf
.....
> So.  Fix this by moving the dqattach_locked call up, and add a comment
> about how we must attach the dquots *before* sampling the data/cow fork
> contents.
> 
> Fixes: a526c85c2236 ("xfs: move xfs_file_iomap_begin_delay around") # goes further back than this
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_iomap.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 1bdd7afc1010..d903f0586490 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -984,6 +984,14 @@ xfs_buffered_write_iomap_begin(
>  	if (error)
>  		goto out_unlock;
>  
> +	/*
> +	 * Attach dquots before we access the data/cow fork mappings, because
> +	 * this function can cycle the ILOCK.
> +	 */
> +	error = xfs_qm_dqattach_locked(ip, false);
> +	if (error)
> +		goto out_unlock;
> +
>  	/*
>  	 * Search the data fork first to look up our source mapping.  We
>  	 * always need the data fork map, as we have to return it to the
> @@ -1071,10 +1079,6 @@ xfs_buffered_write_iomap_begin(
>  			allocfork = XFS_COW_FORK;
>  	}
>  
> -	error = xfs_qm_dqattach_locked(ip, false);
> -	if (error)
> -		goto out_unlock;
> -
>  	if (eof && offset + count > XFS_ISIZE(ip)) {
>  		/*
>  		 * Determine the initial size of the preallocation.
> 

Why not attached the dquots before we call xfs_ilock_for_iomap()?
That way we can just call xfs_qm_dqattach(ip, false) and just return
on failure immediately. That's exactly what we do in the
xfs_iomap_write_direct() path, and it avoids the need to mention
anything about lock cycling because we just don't care
about cycling the ILOCK to read in or allocate dquots before we
start the real work that needs to be done...

Hmmmmm - this means there's a potential problem with IOCB_NOWAIT
here - if the dquots are not in memory, we're going to drop and then
retake the ILOCK_EXCL without trylocks, potentially blocking a task
that should not get blocked. That's a separate problem, though, and
we probably need to plumb NOWAIT through to the dquot lookup cache
miss case to solve that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com