[0/8,v6] btrfs direct-io using iomap
mbox series

Message ID 20191213195750.32184-1-rgoldwyn@suse.de
Headers show
Series
  • btrfs direct-io using iomap
Related show

Message

Goldwyn Rodrigues Dec. 13, 2019, 7:57 p.m. UTC
This is an effort to use iomap for direct I/O in btrfs. This would
change the call from __blockdev_direct_io() to iomap_dio_rw().

The main objective is to lose the buffer head and use bio defined by
iomap code, and hopefully to use more of generic-FS codebase.

These patches are based and tested on v5.5-rc1. I have tested it against
xfstests/btrfs.

The tree is available at
https://github.com/goldwynr/linux/tree/btrfs-iomap-dio

Changes since v1
- Incorporated back the efficiency change for inode locking
- Review comments about coding style and git comments
- Merge related patches into one
- Direct read to go through btrfs_direct_IO()
- Removal of no longer used function dio_end_io()

Changes since v2
- aligning iomap offset/length to the position/length of I/O
- Removed btrfs_dio_data
- Removed BTRFS_INODE_READDIO_NEED_LOCK
- Re-incorporating write efficiency changes caused lockdep_assert() in
  iomap to be triggered, remove that code.

Changes since v3
- Fixed freeze on generic/095. Use iomap_end() to account for
  failed/incomplete dio instead of btrfs_dio_data

Changes since v4
- moved lockdep_assert_held() to functions calling iomap_dio_rw()
  This may be called immidiately after calling inode lock and
  may feel not required, but it seems important.
- Removed comments which are no longer required
- Changed commit comments to make them more appropriate

Changes since v5
- restore inode_dio_wait() in truncate
- Removed lockdep_assert_held() near callers

--
Goldwyn

Comments

Nikolay Borisov Dec. 16, 2019, 12:01 a.m. UTC | #1
On 13.12.19 г. 21:57 ч., Goldwyn Rodrigues wrote:
> This is an effort to use iomap for direct I/O in btrfs. This would
> change the call from __blockdev_direct_io() to iomap_dio_rw().
> 
> The main objective is to lose the buffer head and use bio defined by
> iomap code, and hopefully to use more of generic-FS codebase.
> 
> These patches are based and tested on v5.5-rc1. I have tested it against
> xfstests/btrfs.
> 
> The tree is available at
> https://github.com/goldwynr/linux/tree/btrfs-iomap-dio
> 
> Changes since v1
> - Incorporated back the efficiency change for inode locking
> - Review comments about coding style and git comments
> - Merge related patches into one
> - Direct read to go through btrfs_direct_IO()
> - Removal of no longer used function dio_end_io()
> 
> Changes since v2
> - aligning iomap offset/length to the position/length of I/O
> - Removed btrfs_dio_data
> - Removed BTRFS_INODE_READDIO_NEED_LOCK
> - Re-incorporating write efficiency changes caused lockdep_assert() in
>   iomap to be triggered, remove that code.
> 
> Changes since v3
> - Fixed freeze on generic/095. Use iomap_end() to account for
>   failed/incomplete dio instead of btrfs_dio_data
> 
> Changes since v4
> - moved lockdep_assert_held() to functions calling iomap_dio_rw()
>   This may be called immidiately after calling inode lock and
>   may feel not required, but it seems important.
> - Removed comments which are no longer required
> - Changed commit comments to make them more appropriate
> 
> Changes since v5
> - restore inode_dio_wait() in truncate

I'm confused about this - you no longer call inode_dio_begin after patch
4/8 so inode_dio_wait which is left intact in truncate can never trigger
a wait really. Exclusion is provided by the fact that btrfs_direct_IO is
called with rwsem held shared and truncate holds it exclusive? So what
necessitated restoring inode_dio_wait?


Another point, I don't see whether you have explicitly addressed
concerns raised in:

https://lore.kernel.org/linux-btrfs/20191212003043.31093-1-rgoldwyn@suse.de/T/#me7f96506e5a1d921d05b76d01ecf6ea1ebcea594

> - Removed lockdep_assert_held() near callers
> 
> --
> Goldwyn
> 
>
Goldwyn Rodrigues Dec. 16, 2019, 12:41 p.m. UTC | #2
On  2:01 16/12, Nikolay Borisov wrote:
> 
> 
> On 13.12.19 г. 21:57 ч., Goldwyn Rodrigues wrote:
> > This is an effort to use iomap for direct I/O in btrfs. This would
> > change the call from __blockdev_direct_io() to iomap_dio_rw().
> > 
> > The main objective is to lose the buffer head and use bio defined by
> > iomap code, and hopefully to use more of generic-FS codebase.
> > 
> > These patches are based and tested on v5.5-rc1. I have tested it against
> > xfstests/btrfs.
> > 
> > The tree is available at
> > https://github.com/goldwynr/linux/tree/btrfs-iomap-dio
> > 
> > Changes since v1
> > - Incorporated back the efficiency change for inode locking
> > - Review comments about coding style and git comments
> > - Merge related patches into one
> > - Direct read to go through btrfs_direct_IO()
> > - Removal of no longer used function dio_end_io()
> > 
> > Changes since v2
> > - aligning iomap offset/length to the position/length of I/O
> > - Removed btrfs_dio_data
> > - Removed BTRFS_INODE_READDIO_NEED_LOCK
> > - Re-incorporating write efficiency changes caused lockdep_assert() in
> >   iomap to be triggered, remove that code.
> > 
> > Changes since v3
> > - Fixed freeze on generic/095. Use iomap_end() to account for
> >   failed/incomplete dio instead of btrfs_dio_data
> > 
> > Changes since v4
> > - moved lockdep_assert_held() to functions calling iomap_dio_rw()
> >   This may be called immidiately after calling inode lock and
> >   may feel not required, but it seems important.
> > - Removed comments which are no longer required
> > - Changed commit comments to make them more appropriate
> > 
> > Changes since v5
> > - restore inode_dio_wait() in truncate
> 
> I'm confused about this - you no longer call inode_dio_begin after patch
> 4/8 so inode_dio_wait which is left intact in truncate can never trigger
> a wait really. Exclusion is provided by the fact that btrfs_direct_IO is
> called with rwsem held shared and truncate holds it exclusive? So what
> necessitated restoring inode_dio_wait?

The optimization of the write path of direct I/O. The rwsem is
released if write is within i_size. This is also the reason we
removed lockdep_assert_held().

Reference: 38851cc19adb ("Btrfs: implement unlocked dio write")

This was inspired by ext4. However, ext4 after it's switch to
iomap_dio_rw() does not employ this optimization.

> 
> 
> Another point, I don't see whether you have explicitly addressed
> concerns raised in:
> 
> https://lore.kernel.org/linux-btrfs/20191212003043.31093-1-rgoldwyn@suse.de/T/#me7f96506e5a1d921d05b76d01ecf6ea1ebcea594

I did address it by releasing csums (referring io_bio) before calling
bio_endio().