Re: [PATCH 01/10] xfs: fix freeing speculative preallocations for preallocated files

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Jun 23, 2024 at 07:34:46AM +0200, Christoph Hellwig wrote:
> xfs_can_free_eofblocks returns false for files that have persistent
> preallocations unless the force flag is passed and there are delayed
> blocks.  This means it won't free delalloc reservations for files
> with persistent preallocations unless the force flag is set, and it
> will also free the persistent preallocations if the force flag is
> set and the file happens to have delayed allocations.
> 
> Both of these are bad, so do away with the force flag and always
> free post-EOF delayed allocations only for files with the
> XFS_DIFLAG_PREALLOC flag set.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks fine
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_bmap_util.c | 30 ++++++++++++++++++++++--------
>  fs/xfs/xfs_bmap_util.h |  2 +-
>  fs/xfs/xfs_icache.c    |  2 +-
>  fs/xfs/xfs_inode.c     | 14 ++++----------
>  4 files changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index ac2e77ebb54c73..a4d9fbc21b8343 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -486,13 +486,11 @@ xfs_bmap_punch_delalloc_range(
>  
>  /*
>   * Test whether it is appropriate to check an inode for and free post EOF
> - * blocks. The 'force' parameter determines whether we should also consider
> - * regular files that are marked preallocated or append-only.
> + * blocks.
>   */
>  bool
>  xfs_can_free_eofblocks(
> -	struct xfs_inode	*ip,
> -	bool			force)
> +	struct xfs_inode	*ip)
>  {
>  	struct xfs_bmbt_irec	imap;
>  	struct xfs_mount	*mp = ip->i_mount;
> @@ -526,11 +524,11 @@ xfs_can_free_eofblocks(
>  		return false;
>  
>  	/*
> -	 * Do not free real preallocated or append-only files unless the file
> -	 * has delalloc blocks and we are forced to remove them.
> +	 * Only free real extents for inodes with persistent preallocations or
> +	 * the append-only flag.
>  	 */
>  	if (ip->i_diflags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND))
> -		if (!force || ip->i_delayed_blks == 0)
> +		if (ip->i_delayed_blks == 0)
>  			return false;
>  
>  	/*
> @@ -584,6 +582,22 @@ xfs_free_eofblocks(
>  	/* Wait on dio to ensure i_size has settled. */
>  	inode_dio_wait(VFS_I(ip));
>  
> +	/*
> +	 * For preallocated files only free delayed allocations.
> +	 *
> +	 * Note that this means we also leave speculative preallocations in
> +	 * place for preallocated files.
> +	 */
> +	if (ip->i_diflags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) {
> +		if (ip->i_delayed_blks) {
> +			xfs_bmap_punch_delalloc_range(ip,
> +				round_up(XFS_ISIZE(ip), mp->m_sb.sb_blocksize),
> +				LLONG_MAX);
> +		}
> +		xfs_inode_clear_eofblocks_tag(ip);
> +		return 0;
> +	}
> +
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
>  	if (error) {
>  		ASSERT(xfs_is_shutdown(mp));
> @@ -891,7 +905,7 @@ xfs_prepare_shift(
>  	 * Trim eofblocks to avoid shifting uninitialized post-eof preallocation
>  	 * into the accessible region of the file.
>  	 */
> -	if (xfs_can_free_eofblocks(ip, true)) {
> +	if (xfs_can_free_eofblocks(ip)) {
>  		error = xfs_free_eofblocks(ip);
>  		if (error)
>  			return error;
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 51f84d8ff372fa..eb0895bfb9dae4 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -63,7 +63,7 @@ int	xfs_insert_file_space(struct xfs_inode *, xfs_off_t offset,
>  				xfs_off_t len);
>  
>  /* EOF block manipulation functions */
> -bool	xfs_can_free_eofblocks(struct xfs_inode *ip, bool force);
> +bool	xfs_can_free_eofblocks(struct xfs_inode *ip);
>  int	xfs_free_eofblocks(struct xfs_inode *ip);
>  
>  int	xfs_swap_extents(struct xfs_inode *ip, struct xfs_inode *tip,
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 0953163a2d8492..9967334ea99f1a 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1155,7 +1155,7 @@ xfs_inode_free_eofblocks(
>  	}
>  	*lockflags |= XFS_IOLOCK_EXCL;
>  
> -	if (xfs_can_free_eofblocks(ip, false))
> +	if (xfs_can_free_eofblocks(ip))
>  		return xfs_free_eofblocks(ip);
>  
>  	/* inode could be preallocated or append-only */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index f36091e1e7f50b..38f946e3be2da3 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1595,7 +1595,7 @@ xfs_release(
>  	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL))
>  		return 0;
>  
> -	if (xfs_can_free_eofblocks(ip, false)) {
> +	if (xfs_can_free_eofblocks(ip)) {
>  		/*
>  		 * Check if the inode is being opened, written and closed
>  		 * frequently and we have delayed allocation blocks outstanding
> @@ -1856,15 +1856,13 @@ xfs_inode_needs_inactive(
>  
>  	/*
>  	 * This file isn't being freed, so check if there are post-eof blocks
> -	 * to free.  @force is true because we are evicting an inode from the
> -	 * cache.  Post-eof blocks must be freed, lest we end up with broken
> -	 * free space accounting.
> +	 * to free.
>  	 *
>  	 * Note: don't bother with iolock here since lockdep complains about
>  	 * acquiring it in reclaim context. We have the only reference to the
>  	 * inode at this point anyways.
>  	 */
> -	return xfs_can_free_eofblocks(ip, true);
> +	return xfs_can_free_eofblocks(ip);
>  }
>  
>  /*
> @@ -1947,15 +1945,11 @@ xfs_inactive(
>  
>  	if (VFS_I(ip)->i_nlink != 0) {
>  		/*
> -		 * force is true because we are evicting an inode from the
> -		 * cache. Post-eof blocks must be freed, lest we end up with
> -		 * broken free space accounting.
> -		 *
>  		 * Note: don't bother with iolock here since lockdep complains
>  		 * about acquiring it in reclaim context. We have the only
>  		 * reference to the inode at this point anyways.
>  		 */
> -		if (xfs_can_free_eofblocks(ip, true))
> +		if (xfs_can_free_eofblocks(ip))
>  			error = xfs_free_eofblocks(ip);
>  
>  		goto out;
> -- 
> 2.43.0
> 
>