lkml.org 
[lkml]   [2015]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] direct-io: only inc/dec inode->i_dio_count for file systems
On Wed, Apr 15, 2015 at 11:56:53AM -0700, Andrew Morton wrote:
> On Wed, 15 Apr 2015 12:22:56 -0600 Jens Axboe <axboe@fb.com> wrote:
>
> > Hi,
> >
> > This is a reposting of a patch that was originally in the blk-mq series.
> > It has huge upside on shared access to a multiqueue device doing
> > O_DIRECT, it's basically the scaling block that ends up killing
> > performance. A quick test here reveals that we spend 30% of all system
> > time just incrementing and decremening inode->i_dio_count. For block
> > devices this isn't useful at all, as we don't need protection against
> > truncate. For that test case, performance increases about 3.6x (!!) by
> > getting rid of this inc/dec per IO.
> >
> > I've cleaned it up a bit since last time, integrating the checks in
> > inode_dio_done() and adding a inode_dio_begin() so that callers don't
> > need to know about this.
> >
> > We've been running a variant of this patch in the FB kernel for a while.
> > I'd like to finally get this upstream.
....
> Is there similar impact to direct-io-to-file? It would be nice to fix
> that up also. Many filesystems do something along the lines of
>
> atomic_inc(i_dio_count);
> wibble()
> atomic_dev(i_dio_count);
> __blockdev_direct_IO(...);
>
> and with your patch I think we could change them to
>
> atomic_inc(i_dio_count);
> wibble()
> __blockdev_direct_IO(..., flags|DIO_IGNORE_TRUNCATE);
> atomic_dev(i_dio_count);

Can't do it quite that way.

AIO requires the i_dio_count to held until IO completion for all
outstanding IOs. i.e. the increment needs to be in the submission
path, the decrement needs to be in the dio_complete() path,
otherwise we have AIO DIO in progress without a reference count we
can wait on in truncate.

Yes, we might be able to pull it up to the filesystem level now that
dio_complete() is only ever called once per __blockdev_direct_IO()
call, so that may be a solution we can use via filesystem ->end_io
callbacks provided to __blockdev_direct_IO.

> which would halve the atomic op load.

XFS doesn't touch i_dio_count, so it would make no difference to it
at all, which is important, given the DIO rates I can drive through
a single file on XFS - it becomes rwsem cacheline bound on the
shared IO lock at about 2 million IOPS (random 4k read) to a single
file.

FWIW, keep in mind that this i_dio_count originally came from XFS in
the first place, and was pushed into the DIO layer to solve all the
DIO vs extent manipulation problems other fileystems were having...

> But that's piling hack on top of hack. Can we change the
> do_blockdev_direct_IO() interface to "caller shall hold i_mutex, or
> increment i_dio_count"? ie: exclusion against truncate is wholly the
> caller's responsibility. That way, this awkward sharing of
> responsibility between caller and callee gets cleaned up and
> DIO_IGNORE_TRUNCATE goes away.
>
> inode_dio_begin() would be a good place to assert that i_mutex is held,
> btw.

Can't do that, either, as filesystems like XFS don't hold the
i_mutex during direct IO submission.

> This whole i_dio_count thing is pretty nasty, really. If you stand
> back and squint, it's basically an rwsem. I wonder if we can use an
> rwsem...

That doesn't avoid the atomic operations that limit performance.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com


\
 
 \ /
  Last update: 2015-04-16 01:01    [W:0.091 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site