On Thu, Jan 21, 2021 at 09:59:06AM +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.
>
> Test case is fio randrw of 512 byte IOs using AIO and an iodepth of
> 32 IOs.
>
> Vanilla:
>
> READ: bw=4560KiB/s (4670kB/s), 4560KiB/s-4560KiB/s (4670kB/s-4670kB/s), io=134MiB (140MB), run=30001-30001msec
> WRITE: bw=4567KiB/s (4676kB/s), 4567KiB/s-4567KiB/s (4676kB/s-4676kB/s), io=134MiB (140MB), run=30001-30001msec
>
> Patched:
> READ: bw=37.6MiB/s (39.4MB/s), 37.6MiB/s-37.6MiB/s (39.4MB/s-39.4MB/s), io=1127MiB (1182MB), run=30002-30002msec
> WRITE: bw=37.6MiB/s (39.4MB/s), 37.6MiB/s-37.6MiB/s (39.4MB/s-39.4MB/s), io=1128MiB (1183MB), run=30002-30002msec
>
> That's an improvement from ~18k IOPS to a ~150k IOPS, which is
> about the IOPS limit of the VM block device setup I'm testing on.
>
> 4kB block IO comparison:
>
> READ: bw=296MiB/s (310MB/s), 296MiB/s-296MiB/s (310MB/s-310MB/s), io=8868MiB (9299MB), run=30002-30002msec
> WRITE: bw=296MiB/s (310MB/s), 296MiB/s-296MiB/s (310MB/s-310MB/s), io=8878MiB (9309MB), run=30002-30002msec
>
> Which is ~150k IOPS, same as what the test gets for sub-block
> AIO+DIO writes with this patch.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> [hch: rebased, split unaligned from nowait]
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Looks good, minor nit:
> + /*
> + * For overwrite only I/O, 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.
> + */
> + if (flags & IOMAP_OVERWRITE_ONLY) {
> + error = -EAGAIN;
> + if (imap.br_state != XFS_EXT_NORM &&
> + ((offset & mp->m_blockmask) ||
> + ((offset + length) & mp->m_blockmask)))
> + goto out_unlock;
Why not use the ((offset | length) & mp->blockmask) form of
alignment checking here?
Other than that,
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
