Re: [PATCH v9 06/14] xfs: recheck direct write alignment after write checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] xfs: sector-unaligned O_DIRECT writes after write checks
  bounce to buffered I/O
--

commit abd43cb75b989255524945b08ecf0cb03d4da551
Author: Chris Mason <mason@kernel.org>

xfs: recheck direct write alignment after write checks

This commit rechecks block alignment in xfs_file_dio_write_aligned() after
xfs_file_write_checks() adjusts the write range for O_APPEND and
RLIMIT_FSIZE, falling back to the unaligned direct write path or buffered
I/O if the updated range is no longer block aligned.

> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 153f53f964b99..8ee88910b487d 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -710,6 +710,17 @@ xfs_file_dio_write_aligned(
>  	if (ret)
>  		goto out_unlock;
>  
> +	/*
> +	 * O_APPEND moves the position and the file size limits shorten the
> +	 * write, so only now is the range final.  If it is not block aligned,
> +	 * this is the wrong path for it.  Return -EAGAIN to
> +	 * xfs_file_dio_write(), which picks the path.
> +	 */
> +	if ((iocb->ki_pos | iov_iter_count(from)) & ip->i_mount->m_blockmask) {
> +		ret = -EAGAIN;
> +		goto out_unlock;
> +	}
> +
>  	/*
>  	 * We don't need to hold the IOLOCK exclusively across the IO, so demote
>  	 * the iolock back to shared if we had to take the exclusive lock in
> @@ -921,6 +932,7 @@ xfs_file_dio_write(
>  	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
>  	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
>  	size_t			count = iov_iter_count(from);
> +	ssize_t			ret;
>  
>  	/* direct I/O must be aligned to device logical sector size */
>  	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
> @@ -937,12 +949,32 @@ xfs_file_dio_write(
>  		return xfs_file_dio_write_atomic(ip, iocb, from);
>  	}
>  
> -	if ((iocb->ki_pos | count) & ip->i_mount->m_blockmask)
> -		return xfs_file_dio_write_unaligned(ip, iocb, from);
> -	if (xfs_is_zoned_inode(ip))
> -		return xfs_file_dio_write_zoned(ip, iocb, from);
> -	return xfs_file_dio_write_aligned(ip, iocb, from,
> -			&xfs_direct_write_iomap_ops, &xfs_dio_write_ops, NULL);
> +	/*
> +	 * O_APPEND and the file size limits can still change the range, in
> +	 * xfs_file_write_checks().  So a range that is block aligned here may
> +	 * not stay so, and the write then comes back with -EAGAIN.  If we are
> +	 * doing nonblocking user I/O, propagate the error.
> +	 */
> +	if (!((iocb->ki_pos | count) & ip->i_mount->m_blockmask)) {
> +		if (xfs_is_zoned_inode(ip))
> +			ret = xfs_file_dio_write_zoned(ip, iocb, from);
> +		else
> +			ret = xfs_file_dio_write_aligned(ip, iocb, from,
> +					&xfs_direct_write_iomap_ops,
> +					&xfs_dio_write_ops, NULL);
> +		if (ret != -EAGAIN || (iocb->ki_flags & IOCB_NOWAIT))
> +			return ret;
> +	}
> +
> +	/*
> +	 * Zoned inodes always write out of place, which does not work for a
> +	 * part of a block.  Fall back to buffered I/O.
> +	 */
> +	if (xfs_is_zoned_inode(ip)) {
> +		trace_xfs_reflink_bounce_dio_write(iocb, from);
> +		return -ENOTBLK;
> +	}
> +	return xfs_file_dio_write_unaligned(ip, iocb, from);
>  }

[Severity: Medium]
Does returning -EAGAIN from xfs_file_dio_write_aligned() without checking
target->bt_logical_sectormask cause sector-unaligned O_DIRECT writes to
bounce to buffered I/O instead of failing with -EINVAL?

When a block-aligned O_DIRECT write passes the initial sector check in
xfs_file_dio_write() and xfs_file_write_checks() then moves iocb->ki_pos to
a non-sector-aligned EOF for O_APPEND (or shortens the write via
RLIMIT_FSIZE to a non-sector-aligned length), xfs_file_dio_write_aligned()
now returns -EAGAIN before iomap_dio_rw() can reject the write with
-EINVAL.  On the fallback path, neither xfs_file_dio_write() nor
xfs_file_dio_write_unaligned() rechecks target->bt_logical_sectormask
before returning -ENOTBLK for zoned or CoW inodes, causing
xfs_file_write_iter() to complete the sector-unaligned O_DIRECT write
through the page cache.  For non-CoW inodes, xfs_file_dio_write_unaligned()
proceeds into iomap_dio_rw() where extending writes allocate unwritten
blocks in xfs_direct_write_iomap_begin() before iomap_dio_bio_iter()
rejects the bio alignment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112214604.67875-1-mason@kernel.org?part=6