[PATCH 0/9 v7] btrfs direct-io using iomap

[PATCH 0/9 v7] btrfs direct-io using iomap

[Goldwyn Rodrigues]

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.6-rc7. I have tested it against
xfstests.

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

Changes since v6
- Fixed hangs due to underlying device failures
- Removed the patch to wait while releasing pages

 fs/btrfs/btrfs_inode.h |   18 --
 fs/btrfs/ctree.h       |    4
 fs/btrfs/file.c        |  124 +++++++++++++-
 fs/btrfs/inode.c       |  418 +++++++++++++++----------------------------------
 fs/direct-io.c         |   19 --
 fs/iomap/direct-io.c   |   17 +
 include/linux/fs.h     |    4
 include/linux/iomap.h  |    2
 mm/filemap.c           |   13 -
 9 files changed, 274 insertions(+), 345 deletions(-)

[PATCH 1/9] fs: Export generic_file_buffered_read()

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Export generic_file_buffered_read() to be used to
supplement incomplete direct reads.

While we are at it, correct the comments and variable names.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/fs.h |  2 ++
 mm/filemap.c       | 13 +++++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index abedbffe2c9e..8a29389884bc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3117,6 +3117,8 @@ extern int generic_file_rw_checks(struct file *file_in, struct file *file_out);
 extern int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
 				    struct file *file_out, loff_t pos_out,
 				    size_t *count, unsigned int flags);
+extern ssize_t generic_file_buffered_read(struct kiocb *iocb,
+		struct iov_iter *to, ssize_t already_read);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);
diff --git a/mm/filemap.c b/mm/filemap.c
index 1784478270e1..4266ec3e9d24 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1972,7 +1972,7 @@ static void shrink_readahead_size_eio(struct file *filp,
  * generic_file_buffered_read - generic file read routine
  * @iocb:	the iocb to read
  * @iter:	data destination
- * @written:	already copied
+ * @copied:	already copied
  *
  * This is a generic file read routine, and uses the
  * mapping->a_ops->readpage() function for the actual low-level stuff.
@@ -1981,11 +1981,11 @@ static void shrink_readahead_size_eio(struct file *filp,
  * of the logic when it comes to error handling etc.
  *
  * Return:
- * * total number of bytes copied, including those the were already @written
+ * * total number of bytes copied, including those that were @copied
  * * negative error code if nothing was copied
  */
-static ssize_t generic_file_buffered_read(struct kiocb *iocb,
-		struct iov_iter *iter, ssize_t written)
+ssize_t generic_file_buffered_read(struct kiocb *iocb,
+		struct iov_iter *iter, ssize_t copied)
 {
 	struct file *filp = iocb->ki_filp;
 	struct address_space *mapping = filp->f_mapping;
@@ -2126,7 +2126,7 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb,
 		prev_offset = offset;
 
 		put_page(page);
-		written += ret;
+		copied += ret;
 		if (!iov_iter_count(iter))
 			goto out;
 		if (ret < nr) {
@@ -2234,8 +2234,9 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb,
 
 	*ppos = ((loff_t)index << PAGE_SHIFT) + offset;
 	file_accessed(filp);
-	return written ? written : error;
+	return copied ? copied : error;
 }
+EXPORT_SYMBOL_GPL(generic_file_buffered_read);
 
 /**
  * generic_file_read_iter - generic filesystem read routine
-- 
2.25.0

[PATCH 2/9] iomap: add a filesystem hook for direct I/O bio submission

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This helps filesystems to perform tasks on the bio while submitting for
I/O. This could be post-write operations such as data CRC or data
replication for fs-handled RAID.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/iomap/direct-io.c  | 15 ++++++++++-----
 include/linux/iomap.h |  2 ++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 23837926c0c5..63967fbd8b32 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -59,7 +59,7 @@ int iomap_dio_iopoll(struct kiocb *kiocb, bool spin)
 EXPORT_SYMBOL_GPL(iomap_dio_iopoll);
 
 static void iomap_dio_submit_bio(struct iomap_dio *dio, struct iomap *iomap,
-		struct bio *bio)
+		struct bio *bio, loff_t pos)
 {
 	atomic_inc(&dio->ref);
 
@@ -67,7 +67,12 @@ static void iomap_dio_submit_bio(struct iomap_dio *dio, struct iomap *iomap,
 		bio_set_polled(bio, dio->iocb);
 
 	dio->submit.last_queue = bdev_get_queue(iomap->bdev);
-	dio->submit.cookie = submit_bio(bio);
+	if (dio->dops && dio->dops->submit_io)
+		dio->submit.cookie = dio->dops->submit_io(
+				file_inode(dio->iocb->ki_filp),
+				iomap, bio, pos);
+	else
+		dio->submit.cookie = submit_bio(bio);
 }
 
 static ssize_t iomap_dio_complete(struct iomap_dio *dio)
@@ -191,7 +196,7 @@ iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
 	get_page(page);
 	__bio_add_page(bio, page, len, 0);
 	bio_set_op_attrs(bio, REQ_OP_WRITE, flags);
-	iomap_dio_submit_bio(dio, iomap, bio);
+	iomap_dio_submit_bio(dio, iomap, bio, pos);
 }
 
 static loff_t
@@ -299,11 +304,11 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		}
 
 		dio->size += n;
-		pos += n;
 		copied += n;
 
 		nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
-		iomap_dio_submit_bio(dio, iomap, bio);
+		iomap_dio_submit_bio(dio, iomap, bio, pos);
+		pos += n;
 	} while (nr_pages);
 
 	/*
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 8b09463dae0d..5b4875344874 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -252,6 +252,8 @@ int iomap_writepages(struct address_space *mapping,
 struct iomap_dio_ops {
 	int (*end_io)(struct kiocb *iocb, ssize_t size, int error,
 		      unsigned flags);
+	blk_qc_t (*submit_io)(struct inode *inode, struct iomap *iomap,
+			struct bio *bio, loff_t file_offset);
 };
 
 ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
-- 
2.25.0

[PATCH 3/9] iomap: Remove lockdep_assert_held()

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Filesystems such as btrfs can perform direct I/O without holding the
inode->i_rwsem in some of the cases like writing within i_size.
So, remove the check for lockdep_assert_held() in iomap_dio_rw()

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/iomap/direct-io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 63967fbd8b32..1cac159e912b 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -416,8 +416,6 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct blk_plug plug;
 	struct iomap_dio *dio;
 
-	lockdep_assert_held(&inode->i_rwsem);
-
 	if (!count)
 		return 0;
 
-- 
2.25.0

[PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Switch from __blockdev_direct_IO() to iomap_dio_rw().
Rename btrfs_get_blocks_direct() to btrfs_dio_iomap_begin() and use it
as iomap_begin() for iomap direct I/O functions. This function
allocates and locks all the blocks required for the I/O.
btrfs_submit_direct() is used as the submit_io() hook for direct I/O
ops.

Since we need direct I/O reads to go through iomap_dio_rw(), we change
file_operations.read_iter() to a btrfs_file_read_iter() which calls
btrfs_direct_IO() for direct reads and falls back to
generic_file_buffered_read() for incomplete reads and buffered reads.

We don't need address_space.direct_IO() anymore so set it to noop.
Similarly, we don't need flags used in __blockdev_direct_IO(). iomap is
capable of direct I/O reads from a hole, so we don't need to return
-ENOENT.

BTRFS direct I/O is now done under i_rwsem, shared in case of
reads and exclusive in case of writes. This guards against simultaneous
truncates.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ctree.h |   1 +
 fs/btrfs/file.c  |  21 +++++-
 fs/btrfs/inode.c | 190 ++++++++++++++++++++++-------------------------
 3 files changed, 109 insertions(+), 103 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 36df977b64d9..0cf65cf1d84f 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2949,6 +2949,7 @@ int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end);
 void btrfs_writepage_endio_finish_ordered(struct page *page, u64 start,
 					  u64 end, int uptodate);
 extern const struct dentry_operations btrfs_dentry_operations;
+ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
 
 /* ioctl.c */
 long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index a16da274c9aa..e76be6472652 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1831,7 +1831,7 @@ static ssize_t __btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	loff_t endbyte;
 	int err;
 
-	written = generic_file_direct_write(iocb, from);
+	written = btrfs_direct_IO(iocb, from);
 
 	if (written < 0 || !iov_iter_count(from))
 		return written;
@@ -3443,9 +3443,26 @@ static int btrfs_file_open(struct inode *inode, struct file *filp)
 	return generic_file_open(inode, filp);
 }
 
+static ssize_t btrfs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	ssize_t ret = 0;
+
+	if (iocb->ki_flags & IOCB_DIRECT) {
+		struct inode *inode = file_inode(iocb->ki_filp);
+
+		inode_lock_shared(inode);
+		ret = btrfs_direct_IO(iocb, to);
+		inode_unlock_shared(inode);
+		if (ret < 0)
+			return ret;
+	}
+
+	return generic_file_buffered_read(iocb, to, ret);
+}
+
 const struct file_operations btrfs_file_operations = {
 	.llseek		= btrfs_file_llseek,
-	.read_iter      = generic_file_read_iter,
+	.read_iter      = btrfs_file_read_iter,
 	.splice_read	= generic_file_splice_read,
 	.write_iter	= btrfs_file_write_iter,
 	.mmap		= btrfs_file_mmap,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d267eb5caa7b..bb7b3cfd24e8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -29,6 +29,7 @@
 #include <linux/iversion.h>
 #include <linux/swap.h>
 #include <linux/sched/mm.h>
+#include <linux/iomap.h>
 #include <asm/unaligned.h>
 #include "misc.h"
 #include "ctree.h"
@@ -6943,7 +6944,7 @@ noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
 }
 
 static int lock_extent_direct(struct inode *inode, u64 lockstart, u64 lockend,
-			      struct extent_state **cached_state, int writing)
+			      struct extent_state **cached_state, bool writing)
 {
 	struct btrfs_ordered_extent *ordered;
 	int ret = 0;
@@ -7081,30 +7082,7 @@ static struct extent_map *create_io_em(struct inode *inode, u64 start, u64 len,
 }
 
 
-static int btrfs_get_blocks_direct_read(struct extent_map *em,
-					struct buffer_head *bh_result,
-					struct inode *inode,
-					u64 start, u64 len)
-{
-	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-
-	if (em->block_start == EXTENT_MAP_HOLE ||
-			test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
-		return -ENOENT;
-
-	len = min(len, em->len - (start - em->start));
-
-	bh_result->b_blocknr = (em->block_start + (start - em->start)) >>
-		inode->i_blkbits;
-	bh_result->b_size = len;
-	bh_result->b_bdev = fs_info->fs_devices->latest_bdev;
-	set_buffer_mapped(bh_result);
-
-	return 0;
-}
-
 static int btrfs_get_blocks_direct_write(struct extent_map **map,
-					 struct buffer_head *bh_result,
 					 struct inode *inode,
 					 struct btrfs_dio_data *dio_data,
 					 u64 start, u64 len)
@@ -7166,7 +7144,6 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
 	}
 
 	/* this will cow the extent */
-	len = bh_result->b_size;
 	free_extent_map(em);
 	*map = em = btrfs_new_extent_direct(inode, start, len);
 	if (IS_ERR(em)) {
@@ -7177,15 +7154,6 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
 	len = min(len, em->len - (start - em->start));
 
 skip_cow:
-	bh_result->b_blocknr = (em->block_start + (start - em->start)) >>
-		inode->i_blkbits;
-	bh_result->b_size = len;
-	bh_result->b_bdev = fs_info->fs_devices->latest_bdev;
-	set_buffer_mapped(bh_result);
-
-	if (!test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
-		set_buffer_new(bh_result);
-
 	/*
 	 * Need to update the i_size under the extent lock so buffered
 	 * readers will get the updated i_size when we unlock.
@@ -7201,24 +7169,37 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
 	return ret;
 }
 
-static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
-				   struct buffer_head *bh_result, int create)
+static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
+		loff_t length, unsigned flags, struct iomap *iomap,
+		struct iomap *srcmap)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct extent_map *em;
 	struct extent_state *cached_state = NULL;
 	struct btrfs_dio_data *dio_data = NULL;
-	u64 start = iblock << inode->i_blkbits;
 	u64 lockstart, lockend;
-	u64 len = bh_result->b_size;
+	bool write = !!(flags & IOMAP_WRITE);
 	int ret = 0;
+	u64 len = length;
+	bool unlock_extents = false;
 
-	if (!create)
+	if (!write)
 		len = min_t(u64, len, fs_info->sectorsize);
 
 	lockstart = start;
 	lockend = start + len - 1;
 
+	/*
+	 * The generic stuff only does filemap_write_and_wait_range, which
+	 * isn't enough if we've written compressed pages to this area, so
+	 * we need to flush the dirty pages again to make absolutely sure
+	 * that any outstanding dirty pages are on disk.
+	 */
+	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
+		     &BTRFS_I(inode)->runtime_flags))
+		ret = filemap_fdatawrite_range(inode->i_mapping, start,
+					 start + length - 1);
+
 	if (current->journal_info) {
 		/*
 		 * Need to pull our outstanding extents and set journal_info to NULL so
@@ -7234,7 +7215,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 	 * this range and we need to fallback to buffered.
 	 */
 	if (lock_extent_direct(inode, lockstart, lockend, &cached_state,
-			       create)) {
+			       write)) {
 		ret = -ENOTBLK;
 		goto err;
 	}
@@ -7266,36 +7247,53 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 		goto unlock_err;
 	}
 
-	if (create) {
-		ret = btrfs_get_blocks_direct_write(&em, bh_result, inode,
+	len = min(len, em->len - (start - em->start));
+	if (write) {
+		ret = btrfs_get_blocks_direct_write(&em, inode,
 						    dio_data, start, len);
 		if (ret < 0)
 			goto unlock_err;
-
-		unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart,
-				     lockend, &cached_state);
+		unlock_extents = true;
+		/* Recalc len in case the new em is smaller than requested */
+		len = min(len, em->len - (start - em->start));
+	} else if (em->block_start == EXTENT_MAP_HOLE ||
+			test_bit(EXTENT_FLAG_PREALLOC, &em->flags)) {
+		/* Unlock in case of direct reading from a hole */
+		unlock_extents = true;
 	} else {
-		ret = btrfs_get_blocks_direct_read(em, bh_result, inode,
-						   start, len);
-		/* Can be negative only if we read from a hole */
-		if (ret < 0) {
-			ret = 0;
-			free_extent_map(em);
-			goto unlock_err;
-		}
 		/*
 		 * We need to unlock only the end area that we aren't using.
 		 * The rest is going to be unlocked by the endio routine.
 		 */
-		lockstart = start + bh_result->b_size;
-		if (lockstart < lockend) {
-			unlock_extent_cached(&BTRFS_I(inode)->io_tree,
-					     lockstart, lockend, &cached_state);
-		} else {
-			free_extent_state(cached_state);
-		}
+		lockstart = start + len;
+		if (lockstart < lockend)
+			unlock_extents = true;
 	}
 
+	if (unlock_extents)
+		unlock_extent_cached(&BTRFS_I(inode)->io_tree,
+				lockstart, lockend, &cached_state);
+	else
+		free_extent_state(cached_state);
+
+	/*
+	 * Translate extent map information to iomap
+	 * We trim the extents (and move the addr) even though
+	 * iomap code does that, since we have locked only the parts
+	 * we are performing I/O in.
+	 */
+	if ((em->block_start == EXTENT_MAP_HOLE) ||
+	    (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) && !write)) {
+		iomap->addr = IOMAP_NULL_ADDR;
+		iomap->type = IOMAP_HOLE;
+	} else {
+		iomap->addr = em->block_start + (start - em->start);
+		iomap->type = IOMAP_MAPPED;
+	}
+	iomap->offset = start;
+	iomap->bdev = fs_info->fs_devices->latest_bdev;
+	iomap->length = len;
+
 	free_extent_map(em);
 
 	return 0;
@@ -7662,9 +7660,9 @@ static void btrfs_endio_direct_read(struct bio *bio)
 
 	kfree(dip);
 
-	dio_bio->bi_status = err;
-	dio_end_io(dio_bio);
 	btrfs_io_bio_free_csum(io_bio);
+	dio_bio->bi_status = err;
+	bio_endio(dio_bio);
 	bio_put(bio);
 }
 
@@ -7722,7 +7720,7 @@ static void btrfs_endio_direct_write(struct bio *bio)
 	kfree(dip);
 
 	dio_bio->bi_status = bio->bi_status;
-	dio_end_io(dio_bio);
+	bio_endio(dio_bio);
 	bio_put(bio);
 }
 
@@ -7957,8 +7955,9 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip)
 	return 0;
 }
 
-static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
-				loff_t file_offset)
+static blk_qc_t btrfs_submit_direct(struct inode *inode,
+		struct iomap *iomap, struct bio *dio_bio,
+		loff_t file_offset)
 {
 	struct btrfs_dio_private *dip = NULL;
 	struct bio *bio = NULL;
@@ -8010,7 +8009,7 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
 
 	ret = btrfs_submit_direct_hook(dip);
 	if (!ret)
-		return;
+		return BLK_QC_T_NONE;
 
 	btrfs_io_bio_free_csum(io_bio);
 
@@ -8029,7 +8028,7 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
 		/*
 		 * The end io callbacks free our dip, do the final put on bio
 		 * and all the cleanup and final put for dio_bio (through
-		 * dio_end_io()).
+		 * end_io()).
 		 */
 		dip = NULL;
 		bio = NULL;
@@ -8044,15 +8043,12 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
 			      file_offset + dio_bio->bi_iter.bi_size - 1);
 
 		dio_bio->bi_status = BLK_STS_IOERR;
-		/*
-		 * Releases and cleans up our dio_bio, no need to bio_put()
-		 * nor bio_endio()/bio_io_error() against dio_bio.
-		 */
-		dio_end_io(dio_bio);
+		bio_endio(dio_bio);
 	}
 	if (bio)
 		bio_put(bio);
 	kfree(dip);
+	return BLK_QC_T_NONE;
 }
 
 static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
@@ -8088,7 +8084,23 @@ static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
 	return retval;
 }
 
-static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
+static const struct iomap_ops btrfs_dio_iomap_ops = {
+	.iomap_begin            = btrfs_dio_iomap_begin,
+};
+
+static const struct iomap_dio_ops btrfs_dops = {
+	.submit_io		= btrfs_submit_direct,
+};
+
+
+/*
+ * btrfs_direct_IO - perform direct I/O
+ * inode->i_rwsem must be locked before calling this function, shared or exclusive.
+ * @iocb - kernel iocb
+ * @iter - iter to/from data is copied
+ */
+
+ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
@@ -8097,28 +8109,13 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	struct extent_changeset *data_reserved = NULL;
 	loff_t offset = iocb->ki_pos;
 	size_t count = 0;
-	int flags = 0;
-	bool wakeup = true;
 	bool relock = false;
 	ssize_t ret;
 
 	if (check_direct_IO(fs_info, iter, offset))
 		return 0;
 
-	inode_dio_begin(inode);
-
-	/*
-	 * The generic stuff only does filemap_write_and_wait_range, which
-	 * isn't enough if we've written compressed pages to this area, so
-	 * we need to flush the dirty pages again to make absolutely sure
-	 * that any outstanding dirty pages are on disk.
-	 */
 	count = iov_iter_count(iter);
-	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
-		     &BTRFS_I(inode)->runtime_flags))
-		filemap_fdatawrite_range(inode->i_mapping, offset,
-					 offset + count - 1);
-
 	if (iov_iter_rw(iter) == WRITE) {
 		/*
 		 * If the write DIO is beyond the EOF, we need update
@@ -8149,17 +8146,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		dio_data.unsubmitted_oe_range_end = (u64)offset;
 		current->journal_info = &dio_data;
 		down_read(&BTRFS_I(inode)->dio_sem);
-	} else if (test_bit(BTRFS_INODE_READDIO_NEED_LOCK,
-				     &BTRFS_I(inode)->runtime_flags)) {
-		inode_dio_end(inode);
-		flags = DIO_LOCKING | DIO_SKIP_HOLES;
-		wakeup = false;
 	}
 
-	ret = __blockdev_direct_IO(iocb, inode,
-				   fs_info->fs_devices->latest_bdev,
-				   iter, btrfs_get_blocks_direct, NULL,
-				   btrfs_submit_direct, flags);
+	ret = iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dops,
+			is_sync_kiocb(iocb));
+
 	if (iov_iter_rw(iter) == WRITE) {
 		up_read(&BTRFS_I(inode)->dio_sem);
 		current->journal_info = NULL;
@@ -8186,11 +8177,8 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		btrfs_delalloc_release_extents(BTRFS_I(inode), count);
 	}
 out:
-	if (wakeup)
-		inode_dio_end(inode);
 	if (relock)
 		inode_lock(inode);
-
 	extent_changeset_free(data_reserved);
 	return ret;
 }
@@ -10471,7 +10459,7 @@ static const struct address_space_operations btrfs_aops = {
 	.writepage	= btrfs_writepage,
 	.writepages	= btrfs_writepages,
 	.readpages	= btrfs_readpages,
-	.direct_IO	= btrfs_direct_IO,
+	.direct_IO	= noop_direct_IO,
 	.invalidatepage = btrfs_invalidatepage,
 	.releasepage	= btrfs_releasepage,
 	.set_page_dirty	= btrfs_set_page_dirty,
-- 
2.25.0

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Christoph Hellwig]

On Thu, Mar 26, 2020 at 04:02:49PM -0500, Goldwyn Rodrigues wrote:
> BTRFS direct I/O is now done under i_rwsem, shared in case of
> reads and exclusive in case of writes. This guards against simultaneous
> truncates.

Btw, you really want to add the optimization of only taking it shared
for all the easy write cases similar to what XFS has done for ages
and what ext4 picked up now.  Without that performance on someworkloads
is going to be horrible.  That could be a patch on top of this one,
though.

> +/*
> + * btrfs_direct_IO - perform direct I/O
> + * inode->i_rwsem must be locked before calling this function, shared or exclusive.
> + * @iocb - kernel iocb
> + * @iter - iter to/from data is copied

This adds a way too long line.  Also kerneldoc comments go below the
arguments.  Last but not least a lockdep_assert_held is much more useful
than comments trying to document locking patterns..

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

On  1:10 27/03, Christoph Hellwig wrote:
> On Thu, Mar 26, 2020 at 04:02:49PM -0500, Goldwyn Rodrigues wrote:
> > BTRFS direct I/O is now done under i_rwsem, shared in case of
> > reads and exclusive in case of writes. This guards against simultaneous
> > truncates.
> 
> Btw, you really want to add the optimization of only taking it shared
> for all the easy write cases similar to what XFS has done for ages
> and what ext4 picked up now.  Without that performance on someworkloads
> is going to be horrible.  That could be a patch on top of this one,
> though.

Yes, I will work on that. The idea of dropping the lock seems a little
weird.

-- 
Goldwyn

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Christoph Hellwig]

What is the status of this series?  I haven't really seen it posted
any time recently, and it would be sad to miss 5.8..

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[David Sterba]

On Wed, May 06, 2020 at 11:14:30PM -0700, Christoph Hellwig wrote:
> What is the status of this series?  I haven't really seen it posted
> any time recently, and it would be sad to miss 5.8..

I've been testing it and reporting to Goldwyn, but there are still
problems that don't seem to be fixable for 5.8 target.

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Christoph Hellwig]

On Thu, May 07, 2020 at 01:37:41PM +0200, David Sterba wrote:
> On Wed, May 06, 2020 at 11:14:30PM -0700, Christoph Hellwig wrote:
> > What is the status of this series?  I haven't really seen it posted
> > any time recently, and it would be sad to miss 5.8..
> 
> I've been testing it and reporting to Goldwyn, but there are still
> problems that don't seem to be fixable for 5.8 target.

What are the issues, and how can we help to resolve them?

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

On  5:10 07/05, Christoph Hellwig wrote:
> On Thu, May 07, 2020 at 01:37:41PM +0200, David Sterba wrote:
> > On Wed, May 06, 2020 at 11:14:30PM -0700, Christoph Hellwig wrote:
> > > What is the status of this series?  I haven't really seen it posted
> > > any time recently, and it would be sad to miss 5.8..
> > 
> > I've been testing it and reporting to Goldwyn, but there are still
> > problems that don't seem to be fixable for 5.8 target.
> 
> What are the issues, and how can we help to resolve them?

The issue we are facing is with generic/475

The problems is that in case of a device error, there are still remnants in
the inode reservation which causes the WARN_ON() during
btrfs_destroy_inode(). I am trying to figure out the precise conditions
that are causing these remnants in only some of the inodes.

-- 
Goldwyn

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

On  5:10 07/05, Christoph Hellwig wrote:
> On Thu, May 07, 2020 at 01:37:41PM +0200, David Sterba wrote:
> > On Wed, May 06, 2020 at 11:14:30PM -0700, Christoph Hellwig wrote:
> > > What is the status of this series?  I haven't really seen it posted
> > > any time recently, and it would be sad to miss 5.8..
> > 
> > I've been testing it and reporting to Goldwyn, but there are still
> > problems that don't seem to be fixable for 5.8 target.
> 
> What are the issues, and how can we help to resolve them?

I investigated further and here are my findings.

geenric/475 fails because there are reservations left in the inode's
block reservations system which are not cleared out. So the system
triggers WARN_ON() while performing destroy_inode.

The problem is similar to described in:
50745b0a7f46 ("Btrfs: Direct I/O: Fix space accounting")

To test the theory, I framed an ugly patch of using an extra field
in current-> task_struct to store a number which carries the reservation
currently remaining like the patch does and it works. So what we need is
a way to carry reservation information from btrfs_direct_write() to
iomap's direct ops ->submit_io() where the reservations are consumed.

We cannot use a similar solution of using current->journal_info
because fdatawrite sequence in iomap_dio_rw() uses
current->journal_info.

We cannot perform data reservations and release in iomap_begin() and
iomap_end() for performance and accounting issues.

Ideas welcome.

-- 
Goldwyn

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Christoph Hellwig]

On Thu, May 07, 2020 at 10:14:05PM -0500, Goldwyn Rodrigues wrote:
> geenric/475 fails because there are reservations left in the inode's
> block reservations system which are not cleared out. So the system
> triggers WARN_ON() while performing destroy_inode.
> 
> The problem is similar to described in:
> 50745b0a7f46 ("Btrfs: Direct I/O: Fix space accounting")
> 
> To test the theory, I framed an ugly patch of using an extra field
> in current-> task_struct to store a number which carries the reservation
> currently remaining like the patch does and it works. So what we need is
> a way to carry reservation information from btrfs_direct_write() to
> iomap's direct ops ->submit_io() where the reservations are consumed.
> 
> We cannot use a similar solution of using current->journal_info
> because fdatawrite sequence in iomap_dio_rw() uses
> current->journal_info.
> 
> We cannot perform data reservations and release in iomap_begin() and
> iomap_end() for performance and accounting issues.

So just drop "btrfs: Use ->iomap_end() instead of btrfs_dio_data"
from the series and be done with it?

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

On  6:59 09/05, Christoph Hellwig wrote:
> On Thu, May 07, 2020 at 10:14:05PM -0500, Goldwyn Rodrigues wrote:
> > geenric/475 fails because there are reservations left in the inode's
> > block reservations system which are not cleared out. So the system
> > triggers WARN_ON() while performing destroy_inode.
> > 
> > The problem is similar to described in:
> > 50745b0a7f46 ("Btrfs: Direct I/O: Fix space accounting")
> > 
> > To test the theory, I framed an ugly patch of using an extra field
> > in current-> task_struct to store a number which carries the reservation
> > currently remaining like the patch does and it works. So what we need is
> > a way to carry reservation information from btrfs_direct_write() to
> > iomap's direct ops ->submit_io() where the reservations are consumed.
> > 
> > We cannot use a similar solution of using current->journal_info
> > because fdatawrite sequence in iomap_dio_rw() uses
> > current->journal_info.
> > 
> > We cannot perform data reservations and release in iomap_begin() and
> > iomap_end() for performance and accounting issues.
> 
> So just drop "btrfs: Use ->iomap_end() instead of btrfs_dio_data"
> from the series and be done with it?

We are using current->journal_info for fdatawrite sequence hence using
that as a temporary pointer does not work since iomap_dio_rw() performs
the fdatawrite sequence.

-- 
Goldwyn

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Christoph Hellwig]

On Sat, May 09, 2020 at 11:06:01PM -0500, Goldwyn Rodrigues wrote:
> > > We cannot perform data reservations and release in iomap_begin() and
> > > iomap_end() for performance and accounting issues.
> > 
> > So just drop "btrfs: Use ->iomap_end() instead of btrfs_dio_data"
> > from the series and be done with it?
> 
> We are using current->journal_info for fdatawrite sequence hence using
> that as a temporary pointer does not work since iomap_dio_rw() performs
> the fdatawrite sequence.

Ok. but in that case they never really should have been separate patches.

Can someone help me to understand who consumes the reservation create by
btrfs_delalloc_reserve_space?  Most importantly if this is done by
something called from btrfs_dio_iomap_begin or from btrfs_submit_direct.

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

On  7:58 12/05, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 11:06:01PM -0500, Goldwyn Rodrigues wrote:
> > > > We cannot perform data reservations and release in iomap_begin() and
> > > > iomap_end() for performance and accounting issues.
> > > 
> > > So just drop "btrfs: Use ->iomap_end() instead of btrfs_dio_data"
> > > from the series and be done with it?
> > 
> > We are using current->journal_info for fdatawrite sequence hence using
> > that as a temporary pointer does not work since iomap_dio_rw() performs
> > the fdatawrite sequence.
> 
> Ok. but in that case they never really should have been separate patches.
> 

Yes, I realized it when I was dealing with this problem.

> Can someone help me to understand who consumes the reservation create by
> btrfs_delalloc_reserve_space?  Most importantly if this is done by
> something called from btrfs_dio_iomap_begin or from btrfs_submit_direct.

It is consumed in
btrfs_finish_ordered_io()->..btrfs_cow_block()->..btrfs_use_block_rsv().
So, it is a queued work from __end_write_update_ordered().

I am also understanding the way this reservation system works so I may
not be 100% correct.

More details are in the starting comments of fs/btrfs/block-rsv.c

-- 
Goldwyn

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Christoph Hellwig]

FYI, generic/475 always fail on me for btrfs, due to the warnings on
transaction abort.

Anyway, I have come up with a version that seems to mostly work.

The main change is that btrfs_sync_file stashes away the journal handle.
I also had to merge parts of the ->iomap_end patch into the main iomap
one.  I also did some cleanups to my iomap changes while looking over it.
Let me know what you thing, the tree is here:

    git://git.infradead.org/users/hch/misc.git btrfs-dio

Gitweb:

    http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-dio

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

On  7:13 15/05, Christoph Hellwig wrote:
> 
> FYI, generic/475 always fail on me for btrfs, due to the warnings on
> transaction abort.
> 
> Anyway, I have come up with a version that seems to mostly work.
> 
> The main change is that btrfs_sync_file stashes away the journal handle.

Unfortunately, this will not fly. I tested this week and found
transactions begin in writepage() etc. We could work on stashing journal
handle in those functions as well, but it looks hackish. We might revert
to it as a last resort.

In the meantime, I am trying to revert the allocations in case of an
error.

> I also had to merge parts of the ->iomap_end patch into the main iomap
> one.  I also did some cleanups to my iomap changes while looking over it.
> Let me know what you thing, the tree is here:
> 
>     git://git.infradead.org/users/hch/misc.git btrfs-dio
> 
> Gitweb:
> 
>     http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-dio

-- 
Goldwyn

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

Hi,

On  7:13 15/05, Christoph Hellwig wrote:
> 
> FYI, generic/475 always fail on me for btrfs, due to the warnings on
> transaction abort.
> 
> Anyway, I have come up with a version that seems to mostly work.
> 
> The main change is that btrfs_sync_file stashes away the journal handle.
> I also had to merge parts of the ->iomap_end patch into the main iomap
> one.  I also did some cleanups to my iomap changes while looking over it.
> Let me know what you thing, the tree is here:
> 
>     git://git.infradead.org/users/hch/misc.git btrfs-dio
> 
> Gitweb:
> 
>     http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-dio

I finally managed to fix the reservation issue and the final tree based
on Dave's for-next is at:
https://github.com/goldwynr/linux/tree/dio-merge

I will test it thoroughly and send another patchset.
I will still need that iomap->private!

-- 
Goldwyn

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Christoph Hellwig]

On Tue, May 19, 2020 at 03:11:16PM -0500, Goldwyn Rodrigues wrote:
> I finally managed to fix the reservation issue and the final tree based
> on Dave's for-next is at:
> https://github.com/goldwynr/linux/tree/dio-merge
> 
> I will test it thoroughly and send another patchset.
> I will still need that iomap->private!

FYI, the reason why I killed ->private is that:

 - it turns out the current pointer one is not used at all
 - the integer one as used in your patch set is maybe a bit cleaner
   handled by a counter maintained in the aio code, as in the version
   of the submit hook patch in my tree.

No a biggie, and we can sort that out later.

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[David Sterba]

On Tue, May 19, 2020 at 03:11:16PM -0500, Goldwyn Rodrigues wrote:
> On  7:13 15/05, Christoph Hellwig wrote:
> > FYI, generic/475 always fail on me for btrfs, due to the warnings on
> > transaction abort.
> > 
> > Anyway, I have come up with a version that seems to mostly work.
> > 
> > The main change is that btrfs_sync_file stashes away the journal handle.
> > I also had to merge parts of the ->iomap_end patch into the main iomap
> > one.  I also did some cleanups to my iomap changes while looking over it.
> > Let me know what you thing, the tree is here:
> > 
> >     git://git.infradead.org/users/hch/misc.git btrfs-dio
> > 
> > Gitweb:
> > 
> >     http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-dio
> 
> I finally managed to fix the reservation issue and the final tree based
> on Dave's for-next is at:
> https://github.com/goldwynr/linux/tree/dio-merge
> 
> I will test it thoroughly and send another patchset.
> I will still need that iomap->private!

With the updated top commit 6cbb7a0c7b33d33e6 it passes fstests in my
setup, so that's the minimum for inclusion.

Regarding merge, I'm willing to add it to 5.8 queue still. In total it's
7 patches, 6 of which are preparatory or cleanups that have been
reviewed by several people. The switch to iomap is one patch and not a
huge one.

Sending the latest version proably makes sense so we have it in the
mailinglist, I can add the patches to misc-next right away so it gets
more testing exposure.

There have been other changes to our direct IO code so the testing focus
will be there anyway and reverting one or two patches as fallback is an
option, I think the risk of including the patches that close to merge
window is manageable.

Re: [PATCH 4/9] btrfs: Switch to iomap_dio_rw() for dio

[Goldwyn Rodrigues]

On 13:36 22/05, David Sterba wrote:
> 
> With the updated top commit 6cbb7a0c7b33d33e6 it passes fstests in my
> setup, so that's the minimum for inclusion.
> 
> Regarding merge, I'm willing to add it to 5.8 queue still. In total it's
> 7 patches, 6 of which are preparatory or cleanups that have been
> reviewed by several people. The switch to iomap is one patch and not a
> huge one.
> 
> Sending the latest version proably makes sense so we have it in the
> mailinglist, I can add the patches to misc-next right away so it gets
> more testing exposure.
> 

Yes, sending them in some time. Thanks for testing.

-- 
Goldwyn

[PATCH 5/9] btrfs: Use ->iomap_end() instead of btrfs_dio_data

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Use iomap->iomap_end() to check for failed or incomplete writes and call
__endio_write_update_ordered(). We don't need btrfs_dio_data anymore so
remove that. The bonus is we don't abuse current->journal_info anymore.

A new structure btrfs_iomap is used to keep a count of submitted I/O
for writes.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/inode.c | 110 +++++++++++++++++------------------------------
 1 file changed, 40 insertions(+), 70 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index bb7b3cfd24e8..7bda178afa87 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -55,11 +55,8 @@ struct btrfs_iget_args {
 	struct btrfs_root *root;
 };
 
-struct btrfs_dio_data {
-	u64 reserve;
-	u64 unsubmitted_oe_range_start;
-	u64 unsubmitted_oe_range_end;
-	int overwrite;
+struct btrfs_iomap {
+	u64 submitted_bytes;
 };
 
 static const struct inode_operations btrfs_dir_inode_operations;
@@ -7084,7 +7081,6 @@ static struct extent_map *create_io_em(struct inode *inode, u64 start, u64 len,
 
 static int btrfs_get_blocks_direct_write(struct extent_map **map,
 					 struct inode *inode,
-					 struct btrfs_dio_data *dio_data,
 					 u64 start, u64 len)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
@@ -7158,13 +7154,9 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
 	 * Need to update the i_size under the extent lock so buffered
 	 * readers will get the updated i_size when we unlock.
 	 */
-	if (!dio_data->overwrite && start + len > i_size_read(inode))
+	if (start + len > i_size_read(inode))
 		i_size_write(inode, start + len);
 
-	WARN_ON(dio_data->reserve < len);
-	dio_data->reserve -= len;
-	dio_data->unsubmitted_oe_range_end = start + len;
-	current->journal_info = dio_data;
 out:
 	return ret;
 }
@@ -7176,7 +7168,6 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct extent_map *em;
 	struct extent_state *cached_state = NULL;
-	struct btrfs_dio_data *dio_data = NULL;
 	u64 lockstart, lockend;
 	bool write = !!(flags & IOMAP_WRITE);
 	int ret = 0;
@@ -7200,14 +7191,12 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 		ret = filemap_fdatawrite_range(inode->i_mapping, start,
 					 start + length - 1);
 
-	if (current->journal_info) {
-		/*
-		 * Need to pull our outstanding extents and set journal_info to NULL so
-		 * that anything that needs to check if there's a transaction doesn't get
-		 * confused.
-		 */
-		dio_data = current->journal_info;
-		current->journal_info = NULL;
+	if (write) {
+		iomap->private = kzalloc(sizeof(struct btrfs_iomap), GFP_NOFS);
+		if (!iomap->private) {
+			ret = -ENOMEM;
+			goto err;
+		}
 	}
 
 	/*
@@ -7217,7 +7206,7 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 	if (lock_extent_direct(inode, lockstart, lockend, &cached_state,
 			       write)) {
 		ret = -ENOTBLK;
-		goto err;
+		goto release;
 	}
 
 	em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, start, len);
@@ -7250,7 +7239,7 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 	len = min(len, em->len - (start - em->start));
 	if (write) {
 		ret = btrfs_get_blocks_direct_write(&em, inode,
-						    dio_data, start, len);
+						    start, len);
 		if (ret < 0)
 			goto unlock_err;
 		unlock_extents = true;
@@ -7301,9 +7290,31 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 unlock_err:
 	unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart, lockend,
 			     &cached_state);
+release:
+	if (iomap->private)
+		kfree(iomap->private);
 err:
-	if (dio_data)
-		current->journal_info = dio_data;
+	return ret;
+}
+
+static int btrfs_dio_iomap_end(struct inode *inode, loff_t pos, loff_t length,
+		ssize_t written, unsigned flags, struct iomap *iomap)
+{
+	struct btrfs_iomap *btrfs_iomap = iomap->private;
+	int ret = 0;
+
+	if (!(flags & IOMAP_WRITE))
+		return 0;
+
+	if (btrfs_iomap->submitted_bytes < length) {
+		__endio_write_update_ordered(inode,
+				pos + btrfs_iomap->submitted_bytes,
+				length - btrfs_iomap->submitted_bytes,
+				false);
+		ret = -ENOTBLK;
+	}
+
+	kfree(btrfs_iomap);
 	return ret;
 }
 
@@ -7963,6 +7974,7 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode,
 	struct bio *bio = NULL;
 	struct btrfs_io_bio *io_bio;
 	bool write = (bio_op(dio_bio) == REQ_OP_WRITE);
+	struct btrfs_iomap *btrfs_iomap = iomap->private;
 	int ret = 0;
 
 	bio = btrfs_bio_clone(dio_bio);
@@ -7977,6 +7989,8 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode,
 	dip->inode = inode;
 	dip->logical_offset = file_offset;
 	dip->bytes = dio_bio->bi_iter.bi_size;
+	if (write)
+		btrfs_iomap->submitted_bytes += dip->bytes;
 	dip->disk_bytenr = (u64)dio_bio->bi_iter.bi_sector << 9;
 	bio->bi_private = dip;
 	dip->orig_bio = bio;
@@ -7992,21 +8006,6 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode,
 		dip->subio_endio = btrfs_subio_endio_read;
 	}
 
-	/*
-	 * Reset the range for unsubmitted ordered extents (to a 0 length range)
-	 * even if we fail to submit a bio, because in such case we do the
-	 * corresponding error handling below and it must not be done a second
-	 * time by btrfs_direct_IO().
-	 */
-	if (write) {
-		struct btrfs_dio_data *dio_data = current->journal_info;
-
-		dio_data->unsubmitted_oe_range_end = dip->logical_offset +
-			dip->bytes;
-		dio_data->unsubmitted_oe_range_start =
-			dio_data->unsubmitted_oe_range_end;
-	}
-
 	ret = btrfs_submit_direct_hook(dip);
 	if (!ret)
 		return BLK_QC_T_NONE;
@@ -8086,6 +8085,7 @@ static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
 
 static const struct iomap_ops btrfs_dio_iomap_ops = {
 	.iomap_begin            = btrfs_dio_iomap_begin,
+	.iomap_end		= btrfs_dio_iomap_end,
 };
 
 static const struct iomap_dio_ops btrfs_dops = {
@@ -8105,7 +8105,6 @@ ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	struct btrfs_dio_data dio_data = { 0 };
 	struct extent_changeset *data_reserved = NULL;
 	loff_t offset = iocb->ki_pos;
 	size_t count = 0;
@@ -8123,7 +8122,6 @@ ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		 * not unlock the i_mutex at this case.
 		 */
 		if (offset + count <= inode->i_size) {
-			dio_data.overwrite = 1;
 			inode_unlock(inode);
 			relock = true;
 		} else if (iocb->ki_flags & IOCB_NOWAIT) {
@@ -8135,16 +8133,6 @@ ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		if (ret)
 			goto out;
 
-		/*
-		 * We need to know how many extents we reserved so that we can
-		 * do the accounting properly if we go over the number we
-		 * originally calculated.  Abuse current->journal_info for this.
-		 */
-		dio_data.reserve = round_up(count,
-					    fs_info->sectorsize);
-		dio_data.unsubmitted_oe_range_start = (u64)offset;
-		dio_data.unsubmitted_oe_range_end = (u64)offset;
-		current->journal_info = &dio_data;
 		down_read(&BTRFS_I(inode)->dio_sem);
 	}
 
@@ -8153,25 +8141,7 @@ ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	if (iov_iter_rw(iter) == WRITE) {
 		up_read(&BTRFS_I(inode)->dio_sem);
-		current->journal_info = NULL;
-		if (ret < 0 && ret != -EIOCBQUEUED) {
-			if (dio_data.reserve)
-				btrfs_delalloc_release_space(inode, data_reserved,
-					offset, dio_data.reserve, true);
-			/*
-			 * On error we might have left some ordered extents
-			 * without submitting corresponding bios for them, so
-			 * cleanup them up to avoid other tasks getting them
-			 * and waiting for them to complete forever.
-			 */
-			if (dio_data.unsubmitted_oe_range_start <
-			    dio_data.unsubmitted_oe_range_end)
-				__endio_write_update_ordered(inode,
-					dio_data.unsubmitted_oe_range_start,
-					dio_data.unsubmitted_oe_range_end -
-					dio_data.unsubmitted_oe_range_start,
-					false);
-		} else if (ret >= 0 && (size_t)ret < count)
+		if (ret >= 0 && (size_t)ret < count)
 			btrfs_delalloc_release_space(inode, data_reserved,
 					offset, count - (size_t)ret, true);
 		btrfs_delalloc_release_extents(BTRFS_I(inode), count);
-- 
2.25.0

Re: [PATCH 5/9] btrfs: Use ->iomap_end() instead of btrfs_dio_data

[Christoph Hellwig]

On Thu, Mar 26, 2020 at 04:02:50PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Use iomap->iomap_end() to check for failed or incomplete writes and call
> __endio_write_update_ordered(). We don't need btrfs_dio_data anymore so
> remove that. The bonus is we don't abuse current->journal_info anymore.
> 
> A new structure btrfs_iomap is used to keep a count of submitted I/O
> for writes.

I don't think you need a new structure.  As writes are limited to a
size_t (aka long) you can just case iomap->private.  That is a little
ugly, but we can just switch the private field to an union, something
like the patch below.  If I'm missing a reason why it has to be 64-bit
even on 32-bit kernels we can also grow the size a little on 32-bit
kernels, but right now I don't think that is needed unless I'm missing
something.

---
From e496cd3db3e7420050be19c5fe68e4675f5a2abc Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 27 Mar 2020 09:14:34 +0100
Subject: iomap: turn iomap->private into an union

Make using the union a little easier for scalar values.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/iomap.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 8b09463dae0d..61fea687d93d 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -85,7 +85,10 @@ struct iomap {
 	struct block_device	*bdev;	/* block device for I/O */
 	struct dax_device	*dax_dev; /* dax_dev for dax operations */
 	void			*inline_data;
-	void			*private; /* filesystem private */
+	union {				/* filesystem private data */
+		void		*ptr;
+		uintptr_t	uint;
+	} private;
 	const struct iomap_page_ops *page_ops;
 };
 
-- 
2.25.1

Re: [PATCH 5/9] btrfs: Use ->iomap_end() instead of btrfs_dio_data

[Goldwyn Rodrigues]

On  1:16 27/03, Christoph Hellwig wrote:
> On Thu, Mar 26, 2020 at 04:02:50PM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Use iomap->iomap_end() to check for failed or incomplete writes and call
> > __endio_write_update_ordered(). We don't need btrfs_dio_data anymore so
> > remove that. The bonus is we don't abuse current->journal_info anymore.
> > 
> > A new structure btrfs_iomap is used to keep a count of submitted I/O
> > for writes.
> 
> I don't think you need a new structure.  As writes are limited to a
> size_t (aka long) you can just case iomap->private.  That is a little
> ugly, but we can just switch the private field to an union, something
> like the patch below.  If I'm missing a reason why it has to be 64-bit
> even on 32-bit kernels we can also grow the size a little on 32-bit
> kernels, but right now I don't think that is needed unless I'm missing
> something.

This would be an better approach as opposed to allocating and
deallocating. I was trying not to disrupt the iomap code ;)
Yes, 32-bits works just as well.

> 
> ---
> From e496cd3db3e7420050be19c5fe68e4675f5a2abc Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 27 Mar 2020 09:14:34 +0100
> Subject: iomap: turn iomap->private into an union
> 
> Make using the union a little easier for scalar values.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/iomap.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 8b09463dae0d..61fea687d93d 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -85,7 +85,10 @@ struct iomap {
>  	struct block_device	*bdev;	/* block device for I/O */
>  	struct dax_device	*dax_dev; /* dax_dev for dax operations */
>  	void			*inline_data;
> -	void			*private; /* filesystem private */
> +	union {				/* filesystem private data */
> +		void		*ptr;
> +		uintptr_t	uint;
> +	} private;
>  	const struct iomap_page_ops *page_ops;
>  };
>  


Thanks. Will include this.

-- 
Goldwyn

[PATCH 6/9] fs: Remove dio_end_io()

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Since we removed the last user of dio_end_io(), remove the helper
function dio_end_io().

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/direct-io.c     | 19 -------------------
 include/linux/fs.h |  2 --
 2 files changed, 21 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 00b4d15bb811..c44d60f375bc 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -386,25 +386,6 @@ static void dio_bio_end_io(struct bio *bio)
 	spin_unlock_irqrestore(&dio->bio_lock, flags);
 }
 
-/**
- * dio_end_io - handle the end io action for the given bio
- * @bio: The direct io bio thats being completed
- *
- * This is meant to be called by any filesystem that uses their own dio_submit_t
- * so that the DIO specific endio actions are dealt with after the filesystem
- * has done it's completion work.
- */
-void dio_end_io(struct bio *bio)
-{
-	struct dio *dio = bio->bi_private;
-
-	if (dio->is_async)
-		dio_bio_end_aio(bio);
-	else
-		dio_bio_end_io(bio);
-}
-EXPORT_SYMBOL_GPL(dio_end_io);
-
 static inline void
 dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
 	      struct block_device *bdev,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8a29389884bc..84436f8c0973 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3180,8 +3180,6 @@ enum {
 	DIO_SKIP_HOLES	= 0x02,
 };
 
-void dio_end_io(struct bio *bio);
-
 ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 			     struct block_device *bdev, struct iov_iter *iter,
 			     get_block_t get_block,
-- 
2.25.0

[PATCH 7/9] btrfs: remove BTRFS_INODE_READDIO_NEED_LOCK

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Since we now perform direct reads using i_rwsem, we can remove this
inode flag used to co-ordinate unlocked reads.

The truncate call takes i_rwsem. This means it is correctly synchronized
with concurrent direct reads.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Johannes Thumshirn <jth@kernel.org>
---
 fs/btrfs/btrfs_inode.h | 18 ------------------
 fs/btrfs/inode.c       |  3 ---
 2 files changed, 21 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 4e12a477d32e..cd8f378ed8e7 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -27,7 +27,6 @@ enum {
 	BTRFS_INODE_NEEDS_FULL_SYNC,
 	BTRFS_INODE_COPY_EVERYTHING,
 	BTRFS_INODE_IN_DELALLOC_LIST,
-	BTRFS_INODE_READDIO_NEED_LOCK,
 	BTRFS_INODE_HAS_PROPS,
 	BTRFS_INODE_SNAPSHOT_FLUSH,
 };
@@ -317,23 +316,6 @@ struct btrfs_dio_private {
 			blk_status_t);
 };
 
-/*
- * Disable DIO read nolock optimization, so new dio readers will be forced
- * to grab i_mutex. It is used to avoid the endless truncate due to
- * nonlocked dio read.
- */
-static inline void btrfs_inode_block_unlocked_dio(struct btrfs_inode *inode)
-{
-	set_bit(BTRFS_INODE_READDIO_NEED_LOCK, &inode->runtime_flags);
-	smp_mb();
-}
-
-static inline void btrfs_inode_resume_unlocked_dio(struct btrfs_inode *inode)
-{
-	smp_mb__before_atomic();
-	clear_bit(BTRFS_INODE_READDIO_NEED_LOCK, &inode->runtime_flags);
-}
-
 /* Array of bytes with variable length, hexadecimal format 0x1234 */
 #define CSUM_FMT				"0x%*phN"
 #define CSUM_FMT_VALUE(size, bytes)		size, bytes
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 7bda178afa87..97ff3a38d704 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4701,10 +4701,7 @@ static int btrfs_setsize(struct inode *inode, struct iattr *attr)
 
 		truncate_setsize(inode, newsize);
 
-		/* Disable nonlocked read DIO to avoid the endless truncate */
-		btrfs_inode_block_unlocked_dio(BTRFS_I(inode));
 		inode_dio_wait(inode);
-		btrfs_inode_resume_unlocked_dio(BTRFS_I(inode));
 
 		ret = btrfs_truncate(inode, newsize == oldsize);
 		if (ret && inode->i_nlink) {
-- 
2.25.0

[PATCH 8/9] btrfs: btrfs: split btrfs_direct_IO

[Goldwyn Rodrigues]

From: Christoph Hellwig <hch@lst.de>

The read and write versions don't have anything in common except
for the call to iomap_dio_rw.  So split this function, and merge
each half into its only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ctree.h |   3 ++
 fs/btrfs/file.c  | 103 ++++++++++++++++++++++++++++++++++++++++++-----
 fs/btrfs/inode.c |  99 +--------------------------------------------
 3 files changed, 99 insertions(+), 106 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0cf65cf1d84f..9f6f2df27b4c 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -28,6 +28,7 @@
 #include <linux/dynamic_debug.h>
 #include <linux/refcount.h>
 #include <linux/crc32c.h>
+#include <linux/iomap.h>
 #include "extent-io-tree.h"
 #include "extent_io.h"
 #include "extent_map.h"
@@ -2950,6 +2951,8 @@ void btrfs_writepage_endio_finish_ordered(struct page *page, u64 start,
 					  u64 end, int uptodate);
 extern const struct dentry_operations btrfs_dentry_operations;
 ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
+extern const struct iomap_ops btrfs_dio_iomap_ops;
+extern const struct iomap_dio_ops btrfs_dops;
 
 /* ioctl.c */
 long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index e76be6472652..491951ebfb5d 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1821,17 +1821,73 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 	return num_written ? num_written : ret;
 }
 
-static ssize_t __btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
+static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
+                               const struct iov_iter *iter, loff_t offset)
+{
+        unsigned int blocksize_mask = fs_info->sectorsize - 1;
+
+        if (offset & blocksize_mask)
+                return -EINVAL;
+
+        if (iov_iter_alignment(iter) & blocksize_mask)
+                return -EINVAL;
+
+	return 0;
+}
+
+static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(file);
-	loff_t pos;
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+	loff_t pos = iocb->ki_pos;
 	ssize_t written;
 	ssize_t written_buffered;
 	loff_t endbyte;
 	int err;
+	struct extent_changeset *data_reserved = NULL;
+	size_t count = 0;
+	bool relock = false;
+
+	if (check_direct_IO(fs_info, from, pos))
+		return 0;
+
+	count = iov_iter_count(from);
+	/*
+	 * If the write DIO is beyond the EOF, we need update
+	 * the isize, but it is protected by i_mutex. So we can
+	 * not unlock the i_mutex at this case.
+	 */
+	if (pos + count <= inode->i_size) {
+		inode_unlock(inode);
+		relock = true;
+	} else if (iocb->ki_flags & IOCB_NOWAIT) {
+		return -EAGAIN;
+	}
+	err = btrfs_delalloc_reserve_space(inode, &data_reserved,
+			pos, count);
+	if (err) {
+		extent_changeset_free(data_reserved);
+		if (relock)
+			inode_lock(inode);
+		return err;
+	}
+
+	down_read(&BTRFS_I(inode)->dio_sem);
+	written = iomap_dio_rw(iocb, from, &btrfs_dio_iomap_ops, &btrfs_dops,
+			is_sync_kiocb(iocb));
+	up_read(&BTRFS_I(inode)->dio_sem);
 
-	written = btrfs_direct_IO(iocb, from);
+	if (written < count) {
+		size_t w = written < 0 ? 0 : written;
+		btrfs_delalloc_release_space(inode, data_reserved,
+				pos, count - (size_t)w, true);
+	}
+	btrfs_delalloc_release_extents(BTRFS_I(inode), count);
+
+	if (relock)
+		inode_lock(inode);
+	extent_changeset_free(data_reserved);
 
 	if (written < 0 || !iov_iter_count(from))
 		return written;
@@ -1974,7 +2030,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 		atomic_inc(&BTRFS_I(inode)->sync_writers);
 
 	if (iocb->ki_flags & IOCB_DIRECT) {
-		num_written = __btrfs_direct_write(iocb, from);
+		num_written = btrfs_direct_write(iocb, from);
 	} else {
 		num_written = btrfs_buffered_write(iocb, from);
 		if (num_written > 0)
@@ -3443,16 +3499,45 @@ static int btrfs_file_open(struct inode *inode, struct file *filp)
 	return generic_file_open(inode, filp);
 }
 
+static int check_direct_read(struct btrfs_fs_info *fs_info,
+                               const struct iov_iter *iter, loff_t offset)
+{
+	int ret;
+	int i, seg;
+
+	ret = check_direct_IO(fs_info, iter, offset);
+	if (ret < 0)
+		return ret;
+
+	for (seg = 0; seg < iter->nr_segs; seg++)
+		for (i = seg + 1; i < iter->nr_segs; i++)
+			if (iter->iov[seg].iov_base == iter->iov[i].iov_base)
+				return -EINVAL;
+	return 0;
+}
+
+static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+	ssize_t ret;
+
+	ret = check_direct_read(btrfs_sb(inode->i_sb), to, iocb->ki_pos);
+	if (ret < 0)
+		return ret;
+
+	inode_lock_shared(inode);
+        ret = iomap_dio_rw(iocb, to, &btrfs_dio_iomap_ops, &btrfs_dops,
+                        is_sync_kiocb(iocb));
+	inode_unlock_shared(inode);
+	return ret;
+}
+
 static ssize_t btrfs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
 	ssize_t ret = 0;
 
 	if (iocb->ki_flags & IOCB_DIRECT) {
-		struct inode *inode = file_inode(iocb->ki_filp);
-
-		inode_lock_shared(inode);
-		ret = btrfs_direct_IO(iocb, to);
-		inode_unlock_shared(inode);
+		ret = btrfs_direct_read(iocb, to);
 		if (ret < 0)
 			return ret;
 	}
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 97ff3a38d704..4ccd2a23b1b4 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -29,7 +29,6 @@
 #include <linux/iversion.h>
 #include <linux/swap.h>
 #include <linux/sched/mm.h>
-#include <linux/iomap.h>
 #include <asm/unaligned.h>
 #include "misc.h"
 #include "ctree.h"
@@ -8047,109 +8046,15 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode,
 	return BLK_QC_T_NONE;
 }
 
-static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
-			       const struct iov_iter *iter, loff_t offset)
-{
-	int seg;
-	int i;
-	unsigned int blocksize_mask = fs_info->sectorsize - 1;
-	ssize_t retval = -EINVAL;
-
-	if (offset & blocksize_mask)
-		goto out;
-
-	if (iov_iter_alignment(iter) & blocksize_mask)
-		goto out;
-
-	/* If this is a write we don't need to check anymore */
-	if (iov_iter_rw(iter) != READ || !iter_is_iovec(iter))
-		return 0;
-	/*
-	 * Check to make sure we don't have duplicate iov_base's in this
-	 * iovec, if so return EINVAL, otherwise we'll get csum errors
-	 * when reading back.
-	 */
-	for (seg = 0; seg < iter->nr_segs; seg++) {
-		for (i = seg + 1; i < iter->nr_segs; i++) {
-			if (iter->iov[seg].iov_base == iter->iov[i].iov_base)
-				goto out;
-		}
-	}
-	retval = 0;
-out:
-	return retval;
-}
-
-static const struct iomap_ops btrfs_dio_iomap_ops = {
+const struct iomap_ops btrfs_dio_iomap_ops = {
 	.iomap_begin            = btrfs_dio_iomap_begin,
 	.iomap_end		= btrfs_dio_iomap_end,
 };
 
-static const struct iomap_dio_ops btrfs_dops = {
+const struct iomap_dio_ops btrfs_dops = {
 	.submit_io		= btrfs_submit_direct,
 };
 
-
-/*
- * btrfs_direct_IO - perform direct I/O
- * inode->i_rwsem must be locked before calling this function, shared or exclusive.
- * @iocb - kernel iocb
- * @iter - iter to/from data is copied
- */
-
-ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
-{
-	struct file *file = iocb->ki_filp;
-	struct inode *inode = file->f_mapping->host;
-	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	struct extent_changeset *data_reserved = NULL;
-	loff_t offset = iocb->ki_pos;
-	size_t count = 0;
-	bool relock = false;
-	ssize_t ret;
-
-	if (check_direct_IO(fs_info, iter, offset))
-		return 0;
-
-	count = iov_iter_count(iter);
-	if (iov_iter_rw(iter) == WRITE) {
-		/*
-		 * If the write DIO is beyond the EOF, we need update
-		 * the isize, but it is protected by i_mutex. So we can
-		 * not unlock the i_mutex at this case.
-		 */
-		if (offset + count <= inode->i_size) {
-			inode_unlock(inode);
-			relock = true;
-		} else if (iocb->ki_flags & IOCB_NOWAIT) {
-			ret = -EAGAIN;
-			goto out;
-		}
-		ret = btrfs_delalloc_reserve_space(inode, &data_reserved,
-						   offset, count);
-		if (ret)
-			goto out;
-
-		down_read(&BTRFS_I(inode)->dio_sem);
-	}
-
-	ret = iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dops,
-			is_sync_kiocb(iocb));
-
-	if (iov_iter_rw(iter) == WRITE) {
-		up_read(&BTRFS_I(inode)->dio_sem);
-		if (ret >= 0 && (size_t)ret < count)
-			btrfs_delalloc_release_space(inode, data_reserved,
-					offset, count - (size_t)ret, true);
-		btrfs_delalloc_release_extents(BTRFS_I(inode), count);
-	}
-out:
-	if (relock)
-		inode_lock(inode);
-	extent_changeset_free(data_reserved);
-	return ret;
-}
-
 #define BTRFS_FIEMAP_FLAGS	(FIEMAP_FLAG_SYNC)
 
 static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
-- 
2.25.0

[PATCH 9/9] btrfs: unlock extents in ->iomap_end() for DIO reads

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Simplify the locking and unlocking of extents and unlock the extents in
->iomap_end() instead of endio routine.

This makes sure we do not have locked extents in case of device error.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/inode.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4ccd2a23b1b4..3e8e91d202d2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7241,18 +7241,13 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 		unlock_extents = true;
 		/* Recalc len in case the new em is smaller than requested */
 		len = min(len, em->len - (start - em->start));
-	} else if (em->block_start == EXTENT_MAP_HOLE ||
-			test_bit(EXTENT_FLAG_PREALLOC, &em->flags)) {
-		/* Unlock in case of direct reading from a hole */
-		unlock_extents = true;
-	} else {
+	} else if (start + len < lockend) {
 		/*
 		 * We need to unlock only the end area that we aren't using.
 		 * The rest is going to be unlocked by the endio routine.
 		 */
 		lockstart = start + len;
-		if (lockstart < lockend)
-			unlock_extents = true;
+		unlock_extents = true;
 	}
 
 	if (unlock_extents)
@@ -7299,8 +7294,10 @@ static int btrfs_dio_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	struct btrfs_iomap *btrfs_iomap = iomap->private;
 	int ret = 0;
 
-	if (!(flags & IOMAP_WRITE))
+	if (!(flags & IOMAP_WRITE)) {
+		unlock_extent(&BTRFS_I(inode)->io_tree, pos, pos + length - 1);
 		return 0;
+	}
 
 	if (btrfs_iomap->submitted_bytes < length) {
 		__endio_write_update_ordered(inode,
@@ -7661,10 +7658,7 @@ static void btrfs_endio_direct_read(struct bio *bio)
 	if (dip->flags & BTRFS_DIO_ORIG_BIO_SUBMITTED)
 		err = btrfs_subio_endio_read(inode, io_bio, err);
 
-	unlock_extent(&BTRFS_I(inode)->io_tree, dip->logical_offset,
-		      dip->logical_offset + dip->bytes - 1);
 	dio_bio = dip->dio_bio;
-
 	kfree(dip);
 
 	btrfs_io_bio_free_csum(io_bio);
-- 
2.25.0

Re: [PATCH 0/9 v7] btrfs direct-io using iomap

[David Sterba]

On Thu, Mar 26, 2020 at 04:02:45PM -0500, Goldwyn Rodrigues wrote:
> Changes since v6
> - Fixed hangs due to underlying device failures
> - Removed the patch to wait while releasing pages

There were no hangs and all tests finished, but there were some failures
that seem to be caused by the patchset. I'll send you the logs.