From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755930AbbDOTrM (ORCPT ); Wed, 15 Apr 2015 15:47:12 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:49041 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751965AbbDOTrA (ORCPT ); Wed, 15 Apr 2015 15:47:00 -0400 Date: Wed, 15 Apr 2015 12:46:58 -0700 From: Andrew Morton To: Jens Axboe Cc: , , , Subject: Re: [PATCH] direct-io: only inc/dec inode->i_dio_count for file systems Message-Id: <20150415124658.7ef9bde65965cc3031239522@linux-foundation.org> In-Reply-To: <552EBB7B.4030107@fb.com> References: <20150415182256.GA30339@kernel.dk> <20150415115653.f69d970e4b155deea98a0829@linux-foundation.org> <552EBB7B.4030107@fb.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 15 Apr 2015 13:26:51 -0600 Jens Axboe wrote: > > 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. > > 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? 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. > > 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 :( > > 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. > > 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.