On 21/05/19 11:19AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Because this happens at high thread counts on high IOPS devices
> doing mixed read/write AIO-DIO to a single file at about a million
> iops:
>
> 64.09% 0.21% [kernel] [k] io_submit_one
> - 63.87% io_submit_one
> - 44.33% aio_write
> - 42.70% xfs_file_write_iter
> - 41.32% xfs_file_dio_write_aligned
> - 25.51% xfs_file_write_checks
> - 21.60% _raw_spin_lock
> - 21.59% do_raw_spin_lock
> - 19.70% __pv_queued_spin_lock_slowpath
>
> This also happens of the IO completion IO path:
>
> 22.89% 0.69% [kernel] [k] xfs_dio_write_end_io
> - 22.49% xfs_dio_write_end_io
> - 21.79% _raw_spin_lock
> - 20.97% do_raw_spin_lock
> - 20.10% __pv_queued_spin_lock_slowpath ▒
>
> IOWs, fio is burning ~14 whole CPUs on this spin lock.
>
> So, do an unlocked check against inode size first, then if we are
> at/beyond EOF, take the spinlock and recheck. This makes the
> spinlock disappear from the overwrite fastpath.
>
> I'd like to report that fixing this makes things go faster. It
> doesn't - it just exposes the the XFS_ILOCK as the next severe
> contention point doing extent mapping lookups, and that now burns
> all the 14 CPUs this spinlock was burning.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_file.c | 42 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 396ef36dcd0a..c068dcd414f4 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -384,21 +384,30 @@ xfs_file_write_checks(
> }
> goto restart;
> }
> +
> /*
> * If the offset is beyond the size of the file, we need to zero any
> * blocks that fall between the existing EOF and the start of this
> - * write. If zeroing is needed and we are currently holding the
> - * iolock shared, we need to update it to exclusive which implies
> - * having to redo all checks before.
> + * write. If zeroing is needed and we are currently holding the iolock
> + * shared, we need to update it to exclusive which implies having to
> + * redo all checks before.
> + *
> + * We need to serialise against EOF updates that occur in IO completions
> + * here. We want to make sure that nobody is changing the size while we
> + * do this check until we have placed an IO barrier (i.e. hold the
> + * XFS_IOLOCK_EXCL) that prevents new IO from being dispatched. The
> + * spinlock effectively forms a memory barrier once we have the
> + * XFS_IOLOCK_EXCL so we are guaranteed to see the latest EOF value and
> + * hence be able to correctly determine if we need to run zeroing.
> *
> - * We need to serialise against EOF updates that occur in IO
> - * completions here. We want to make sure that nobody is changing the
> - * size while we do this check until we have placed an IO barrier (i.e.
> - * hold the XFS_IOLOCK_EXCL) that prevents new IO from being dispatched.
> - * The spinlock effectively forms a memory barrier once we have the
> - * XFS_IOLOCK_EXCL so we are guaranteed to see the latest EOF value
> - * and hence be able to correctly determine if we need to run zeroing.
> + * We can do an unlocked check here safely as IO completion can only
> + * extend EOF. Truncate is locked out at this point, so the EOF can
> + * not move backwards, only forwards. Hence we only need to take the
> + * slow path and spin locks when we are at or beyond the current EOF.
> */
> + if (iocb->ki_pos <= i_size_read(inode))
> + goto out;
> +
> spin_lock(&ip->i_flags_lock);
> isize = i_size_read(inode);
> if (iocb->ki_pos > isize) {
Hello Dave/Jan,
Sorry about some silly queries here. But locking sometimes can get confusing and
needs a background context/history.
So,
I was going through the XFS DIO path and I couldn't completely get this below
difference between xfs_file_dio_write_unaligned() v/s
xfs_file_dio_write_aligned() checks for taking xfs iolock (inode rwsem)
with different exclusivity(exclusive v/s shared).
I in xfs_**_unaligned() function, we also check if (ki_pos + count >= isize()).
If yes, then we go for an exclusive iolock.
While in xfs_**_aligned() function, we always take shared iolock.
Can you please help me understand why is that? In case of an extending aligned
write, won't we need an exclusive iolock for XFS?
Or IIUC, this exclusive lock is mostly needed to prevent two sub-bock zeroing
from running in parallel (which if this happens could cause corruption)
and this can only happen with unaligned writes.
Whereas, I guess ext4, still does exclusive lock even with aligned extending
writes, possibly because of updation of inode->i_size and orphan inode
handling requires it to take exclusive inode rwsem.
While for XFS inode->i_size updation happens with a different spinlock which is
ip->i_flags_lock.
Is my understanding complete and correct?
Or did I miss anything here?
Thanks
ritesh
> @@ -426,7 +435,7 @@ xfs_file_write_checks(
> drained_dio = true;
> goto restart;
> }
> -
> +
> trace_xfs_zero_eof(ip, isize, iocb->ki_pos - isize);
> error = iomap_zero_range(inode, isize, iocb->ki_pos - isize,
> NULL, &xfs_buffered_write_iomap_ops);
> @@ -435,6 +444,7 @@ xfs_file_write_checks(
> } else
> spin_unlock(&ip->i_flags_lock);
>
> +out:
> return file_modified(file);
> }
>
> @@ -500,7 +510,17 @@ xfs_dio_write_end_io(
> * other IO completions here to update the EOF. Failing to serialise
> * here can result in EOF moving backwards and Bad Things Happen when
> * that occurs.
> + *
> + * As IO completion only ever extends EOF, we can do an unlocked check
> + * here to avoid taking the spinlock. If we land within the current EOF,
> + * then we do not need to do an extending update at all, and we don't
> + * need to take the lock to check this. If we race with an update moving
> + * EOF, then we'll either still be beyond EOF and need to take the lock,
> + * or we'll be within EOF and we don't need to take it at all.
> */
> + if (offset + size <= i_size_read(inode))
> + goto out;
> +
> spin_lock(&ip->i_flags_lock);
> if (offset + size > i_size_read(inode)) {
> i_size_write(inode, offset + size);
> --
> 2.31.1
>
