linux-kernel - Re: [PATCH] direct-io: only inc/dec inode->i_dio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20150415115653.f69d970e4b155deea98a0829@linux-foundation.org>
Date:	Wed, 15 Apr 2015 11:56:53 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jens Axboe <axboe@...com>
Cc:	<linux-fsdevel@...r.kernel.org>, <hch@...radead.org>,
	<viro@...iv.linux.org.uk>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] direct-io: only inc/dec inode->i_dio_count for file
 systems

On Wed, 15 Apr 2015 12:22:56 -0600 Jens Axboe <axboe@...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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/