Re: [PATCH] xfs: don't take a spinlock unconditionally in the DIO fastpath

[Dave Chinner <david@fromorbit.com>]

On Wed, May 19, 2021 at 09:59:29AM +0200, Carlos Maiolino wrote:
> On Wed, May 19, 2021 at 11:19:20AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > 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.
> 
> maybe you meant this does not make things go faster?

Yes, that is what this statement means. That is, I'd -like- to
report that things went faster, but reality doesn't care about what
I'd -like- to have happen, as the next sentence explained... :(

> > 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@redhat.com>
> 
> The patch looks good, and the comments about why it's safe to not take the
> spinlock (specially why the EOF can't be moved back) is much welcomed.
> 
> Feel free to add:
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com