Correctness of inode_dio_end in generic DIO code

Correctness of inode_dio_end in generic DIO code

[Nikolay Borisov]

Hello,

Currently the generic DIO code calls inode_dio_begin/inode_dio_end if
DIO_SKIP_DIO_COUNT is not set. 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.

So is it at all correct to increment i_dio_count in generic dio code
without imposing strict locking requirement? Currently, most major
filesystems (Ext4/xfs/btrfs) do modify i_dio_count under their own
locks. Perhaps it's best if i_dio_count modification are removed from
the generic code, what do people think about that?

Re: Correctness of inode_dio_end in generic DIO code

[Jan Kara]

Hello,

On Tue 20-02-18 10:59:46, Nikolay Borisov wrote:
> Currently the generic DIO code calls inode_dio_begin/inode_dio_end if
> DIO_SKIP_DIO_COUNT is not set. 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.
> 
> So is it at all correct to increment i_dio_count in generic dio code
> without imposing strict locking requirement? Currently, most major
> filesystems (Ext4/xfs/btrfs) do modify i_dio_count under their own
> locks. Perhaps it's best if i_dio_count modification are removed from
> the generic code, what do people think about that?

Currently the onus is on inode_dio_wait() callers to make sure they cannot
livelock (usually by calling that function in a context which blocks
submission of new direct IO). So in this sense I don't see anything wrong
with calling inode_dio_begin() from do_blockdev_direct_IO(). Whether
calling these functions directly from fs code instead of from
do_blockdev_direct_IO() to make things clearer is worth the additional
code in quite a few filesystems is IMHO a matter of taste. I'm fine with
the current state but then I admit I might have just got used to it :)

							Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Correctness of inode_dio_end in generic DIO code

[Dave Chinner]

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