Re: [PATCH] direct-io: only inc/dec inode->i_dio_count for file systems

[Andrew Morton <akpm@linux-foundation.org>]

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.

30% overhead for one atomic_inc+atomic_dec+wake_up_bit() per IO?  That
seems very high!  Is there something else going on?



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);

which would halve the atomic op load.

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.



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...



What's the reason for DIO_IGNORE_TRUNCATE rather than boring old
!S_ISBLK?