On Mon, Jan 18, 2021 at 08:35:16PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Attempt shared locking for unaligned DIO, but only if the the
> underlying extent is already allocated and in written state. On
> failure, retry with the existing exclusive locking.
....
> @@ -590,19 +617,27 @@ xfs_file_dio_write_unaligned(
> goto out_unlock;
>
> /*
> - * If we are doing unaligned I/O, we can't allow any other overlapping
> - * I/O in-flight at the same time or we risk data corruption. Wait for
> - * all other I/O to drain before we submit.
> + * If we are doing exclusive unaligned IO, we can't allow any other
> + * overlapping IO in-flight at the same time or we risk data corruption.
> + * Wait for all other IO to drain before we submit.
> */
> - inode_dio_wait(VFS_I(ip));
> + if (!(flags & IOMAP_DIO_UNALIGNED))
> + inode_dio_wait(VFS_I(ip));
>
> - /*
> - * This must be the only I/O in-flight. Wait on it before we release the
> - * iolock to prevent subsequent overlapping I/O.
> - */
> trace_xfs_file_direct_write(iocb, from);
> ret = iomap_dio_rw(iocb, from, &xfs_direct_write_iomap_ops,
> - &xfs_dio_write_ops, IOMAP_DIO_FORCE_WAIT);
> + &xfs_dio_write_ops, flags);
> + /*
> + * Retry unaligned IO with exclusive blocking semantics if the DIO
> + * layer rejected it for mapping or locking reasons. If we are doing
> + * nonblocking user IO, propagate the error.
> + */
> + if (ret == -EAGAIN && !(iocb->ki_flags & IOCB_NOWAIT)) {
> + ASSERT(flags & IOMAP_DIO_UNALIGNED);
> + xfs_iunlock(ip, iolock);
> + goto retry_exclusive;
> + }
> +
> out_unlock:
> if (iolock)
> xfs_iunlock(ip, iolock);
Do we ever get here without holding the iolock anymore?
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 7b9ff824e82d48..dc8c86e98b99bf 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -784,15 +784,30 @@ xfs_direct_write_iomap_begin(
> goto allocate_blocks;
>
> /*
> - * NOWAIT IO needs to span the entire requested IO with a single map so
> - * that we avoid partial IO failures due to the rest of the IO range not
> - * covered by this map triggering an EAGAIN condition when it is
> - * subsequently mapped and aborting the IO.
> + * NOWAIT and unaligned IO needs to span the entire requested IO with a
> + * single map so that we avoid partial IO failures due to the rest of
> + * the IO range not covered by this map triggering an EAGAIN condition
> + * when it is subsequently mapped and aborting the IO.
> */
> - if ((flags & IOMAP_NOWAIT) &&
> - !imap_spans_range(&imap, offset_fsb, end_fsb)) {
> + if (flags & (IOMAP_NOWAIT | IOMAP_UNALIGNED)) {
> error = -EAGAIN;
> - goto out_unlock;
> + if (!imap_spans_range(&imap, offset_fsb, end_fsb))
> + goto out_unlock;
> + }
> +
> + /*
> + * For unsigned I/O we can't convert an unwritten extents if the I/O is
> + * not block size aligned, as such a conversion would have to do
> + * sub-block zeroing, and that can only be done under an exclusive
> + * IOLOCK. Hence if this is not a written extent, return EAGAIN to tell
> + * the caller to try again.
> + */
A few typos in that comment :)
/*
* For unaligned IO, we cannot convert unwritten extents without
* requiring sub-block zeroing. This can only be done under an exclusive
* IOLOCK, hence return -EAGAIN if this is not a written extent to tell
* the caller to try again.
*/
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
