direct-io: only inc/dec inode->i_dio_count for file systems

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.

Quoted 15 lines · On Wed, 15 Apr 2015 12:22:56 -0600 Jens Axboe <[email protected]> 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.

Quoted 2 lines

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?

Quoted 8 lines

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

Quoted 8 lines

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.

I haven't checked pure file, but without extending, I suspect that we should see similar benefits there. In any case, it'd make sense doing, having twice the atomic inc/dec is just a bad idea in general, if we can get rid of it.

A quick grep doesn't show this use case, or I'm just blind. Where do you see that?

Quoted 1 line

But that's piling hack on top of hack. Can we change the

I'd more view it as reducing the hack, the real hack is the way that we manually do atomic_inc() on i_dio_count, and then call a magic inode_dio_done() that decrements it again. It's not very pretty, I'm just reducing the scope of the hack :-)

Quoted 5 lines

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.

That would clean it up, at the expense of touching more churn. I'd be fine with doing it that way.

Quoted 2 lines

inode_dio_begin() would be a good place to assert that i_mutex is held, btw.

How would you check it? If the i_dio_count is bumped, then you'd not need to hold i_mutex.

Quoted 22 lines · On Wed, 15 Apr 2015 13:26:51 -0600 Jens Axboe <[email protected]> wrote:

Quoted 8 lines

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

Quoted 8 lines

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.

I haven't checked pure file, but without extending, I suspect that we should see similar benefits there. In any case, it'd make sense doing, having twice the atomic inc/dec is just a bad idea in general, if we can get rid of it.

Quoted 2 lines

A quick grep doesn't show this use case, or I'm just blind. Where do you see that?

btrfs_direct_IO() holds i_dio_count across its call to __blockdev_direct_IO() for writes. That makes the dio_bio_count manipulation in do_blockdev_direct_IO() unneeded? ext4 is similar.

Reducing from 4 ops to 2 probably won't make as much difference as reducing from 2 to 0 - most of the cost comes from initially grabbing that cacheline from a different CPU.

Quoted 6 lines

Quoted 1 line

But that's piling hack on top of hack. Can we change the

I'd more view it as reducing the hack, the real hack is the way that we manually do atomic_inc() on i_dio_count, and then call a magic inode_dio_done() that decrements it again. It's not very pretty, I'm just reducing the scope of the hack :-)

A magic flag which says "you don't need to do this in that case because I know this inode is special". direct-io already has too much of this :(

Quoted 8 lines

Quoted 5 lines

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.

That would clean it up, at the expense of touching more churn. I'd be fine with doing it that way.

OK, could be done later I suppose.

Quoted 5 lines

Quoted 2 lines

inode_dio_begin() would be a good place to assert that i_mutex is held, btw.

How would you check it? If the i_dio_count is bumped, then you'd not need to hold i_mutex.

	if (atomic_add_return() == 1)
		assert()

I guess. It was just a thought. Having wandered around the code, I'm not 100% confident that everyone is holding i_mutex - it's not all obviously correct.

otoh, the caller doesn't *have* to choose i_mutex for the external exclusion, and perhaps some callers have used something else.

Quoted 17 lines · On Wed, 15 Apr 2015 12:22:56 -0600 Jens Axboe <[email protected]> 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);

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