From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail06.adl2.internode.on.net ([150.101.137.129]:42481 "EHLO ipmail06.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750799AbeBTUr7 (ORCPT ); Tue, 20 Feb 2018 15:47:59 -0500 Date: Wed, 21 Feb 2018 07:47:56 +1100 From: Dave Chinner To: Nikolay Borisov Cc: linux-fsdevel , Jens Axboe , Goldwyn Rodrigues Subject: Re: Correctness of inode_dio_end in generic DIO code Message-ID: <20180220204756.GA7000@dastard> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Feb 20, 2018 at 10:59:46AM +0200, Nikolay Borisov wrote: > Hello, > > Currently the generic DIO code calls inode_dio_begin/inode_dio_end if > DIO_SKIP_DIO_COUNT is not set. DIO_SKIP_DIO_COUNT is not used by anyone. It's dead code, so probably should be removed. > However, te generic ode doesn't really > know if there is a lock synchronizing all the various inode_dio_* > operations. As per inode_dio_wait comment : > > Must be called under a lock that serializes taking new references to > i_dio_count, usually by inode->i_mutex. Yup. DIO_LOCKING fileystems all use inode->i_rwsem. Filesystems that don't use DIO_LOCKING need to provide their own locking. Locking for DIO submission is all explained in the comment above do_blockdev_direct_IO(). inode_dio_begin() is covered by the IO submission locking scheme... > So is it at all correct to increment i_dio_count in generic dio code > without imposing strict locking requirement? Most filesystems call blockdev_direct_IO() which sets DIO_LOCKING. > Currently, most major > filesystems (Ext4/xfs/btrfs) do modify i_dio_count under their own > locks. Sort of. Both btrfs and ext4 use DIO_LOCKING directly, except in certain configs ext4 doesn't do any locking at all. XFS uses it's "own locking", but that's actually inode->i_rwsem now, Also, XFS also uses iomap_dio_rw(), which is a new, more efficient direct IO code path with a separate call to inode_dio_begin() under "caller must lock IO submission" rules.... > Perhaps it's best if i_dio_count modification are removed from > the generic code, what do people think about that? IMO, it's in the correct spot - it's always accounted and called under the correct IO submission locks where it is. Removing it from the generic code will simply introduce bugs in new/lesser travelled filesystems where they forget to call it or call it incorrectly. Cheers, Dave. -- Dave Chinner david@fromorbit.com