Re: [PATCH 7/9] xfs: check XFS_EOFBLOCKS_RELEASED earlier in xfs_release_eofblocks

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Thu, Aug 08, 2024 at 08:27:33AM -0700, Christoph Hellwig wrote:
> If the XFS_EOFBLOCKS_RELEASED flag is set, we are not going to free the
> eofblocks, so don't bother locking the inode or performing the checks in
> xfs_can_free_eofblocks.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 30b553ac8f56bb..f1593690ba88d2 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1234,9 +1234,9 @@ xfs_file_release(
>  	 */
>  	if (inode->i_nlink &&
>  	    (file->f_mode & FMODE_WRITE) &&
> +	    !xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED) &&
>  	    xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
> -		if (xfs_can_free_eofblocks(ip) &&
> -		    !xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED)) {
> +		if (xfs_can_free_eofblocks(ip)) {
>  			xfs_free_eofblocks(ip);
>  			xfs_iflags_set(ip, XFS_EOFBLOCKS_RELEASED);
>  		}

The test and set here is racy. A long time can pass between the test
and the setting of the flag, so maybe this should be optimised to
something like:

	if (inode->i_nlink &&
	    (file->f_mode & FMODE_WRITE) &&
	    (!(ip->i_flags & XFS_EOFBLOCKS_RELEASED)) &&
	    xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
		if (xfs_can_free_eofblocks(ip) &&
		    !xfs_iflags_test_and_set(ip, XFS_EOFBLOCKS_RELEASED))
			xfs_free_eofblocks(ip);
		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
	}

I do wonder, though - why do we need to hold the IOLOCK to call
xfs_can_free_eofblocks()? The only thing that really needs
serialisation is the xfS_bmapi_read() call, and that's done under
the ILOCK not the IOLOCK. Sure, xfs_free_eofblocks() needs the
IOLOCK because it's effectively a truncate w.r.t. extending writes,
but races with extending writes while checking if we need to do that
operation aren't really a big deal. Worst case is we take the
lock and free the EOF blocks beyond the writes we raced with.

What am I missing here?

i.e. it seems to me that the logic here could be:

	if (inode->i_nlink &&
	    (file->f_mode & FMODE_WRITE) &&
	    (!(ip->i_flags & XFS_EOFBLOCKS_RELEASED)) &&
	    xfs_can_free_eofblocks(ip) &&
	    xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL) &&
	    !xfs_iflags_test_and_set(ip, XFS_EOFBLOCKS_RELEASED)) {
		xfs_free_eofblocks(ip);
		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
	}

And so avoids attempting to take or taking locks in all the cases
where locks can be avoided.

-Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx