On Thu, Aug 08, 2024 at 08:27:32AM -0700, Christoph Hellwig wrote:
> From: "Darrick J. Wong" <djwong@xxxxxxxxxx>
>
> Certain workloads fragment files on XFS very badly, such as a software
> package that creates a number of threads, each of which repeatedly run
> the sequence: open a file, perform a synchronous write, and close the
> file, which defeats the speculative preallocation mechanism. We work
> around this problem by only deleting posteof blocks the /first/ time a
> file is closed to preserve the behavior that unpacking a tarball lays
> out files one after the other with no gaps.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> [hch: rebased, updated comment, renamed the flag]
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_file.c | 32 +++++++++++---------------------
> fs/xfs/xfs_inode.h | 4 ++--
> 2 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 60424e64230743..30b553ac8f56bb 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1204,15 +1204,21 @@ xfs_file_release(
> * exposed to that problem.
> */
> if (xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED)) {
> - xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE);
> + xfs_iflags_clear(ip, XFS_EOFBLOCKS_RELEASED);
> if (ip->i_delayed_blks > 0)
> filemap_flush(inode->i_mapping);
> }
This should probably be open coded to minimise lock cycles and lock
contention on the flags lock when concurrent open/sync write/close
cycles are run on the file (as recently reported by Mateusz). i.e:
if (ip->i_flags & XFS_ITRUNCATED) {
spin_lock(&ip->i_flags_lock);
if (ip->i_flags & XFS_ITRUNCATED)
ip->i_flags &= ~(XFS_ITRUNCATED | XFS_EOFBLOCKS_RELEASED);
spin_unlock(&ip->i_flags_lock);
if (ip->i_delayed_blks > 0)
filemap_flush(inode->i_mapping);
}
....
> @@ -1230,25 +1236,9 @@ xfs_file_release(
> (file->f_mode & FMODE_WRITE) &&
> xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
> if (xfs_can_free_eofblocks(ip) &&
> - !xfs_iflags_test(ip, XFS_IDIRTY_RELEASE)) {
> - /*
> - * Check if the inode is being opened, written and
> - * closed frequently and we have delayed allocation
> - * blocks outstanding (e.g. streaming writes from the
> - * NFS server), truncating the blocks past EOF will
> - * cause fragmentation to occur.
> - *
> - * In this case don't do the truncation, but we have to
> - * be careful how we detect this case. Blocks beyond EOF
> - * show up as i_delayed_blks even when the inode is
> - * clean, so we need to truncate them away first before
> - * checking for a dirty release. Hence on the first
> - * dirty close we will still remove the speculative
> - * allocation, but after that we will leave it in place.
> - */
> + !xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED)) {
> xfs_free_eofblocks(ip);
> - if (ip->i_delayed_blks)
> - xfs_iflags_set(ip, XFS_IDIRTY_RELEASE);
> + xfs_iflags_set(ip, XFS_EOFBLOCKS_RELEASED);
!xfs_iflags_test_and_set(ip, XFS_EOFBLOCKS_RELEASED)
xfs_free_eofblocks(ip);
This also avoids an extra lock cycle to set the flag....
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
