btrfs direct-io using iomap

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.

I have tested it against xfstests. The performance seems to be nearly
the same on my VMs.

-- 
Goldwyn

 fs/btrfs/ctree.h      |    1 
 fs/btrfs/extent_io.c  |    8 ++-
 fs/btrfs/file.c       |   67 +++++++++++++++++++++++++-
 fs/btrfs/inode.c      |  128 +++++++++++++++++++++-----------------------------
 fs/iomap/direct-io.c  |   14 +++--
 include/linux/fs.h    |    2 
 include/linux/iomap.h |    2 
 mm/filemap.c          |   13 ++---
 8 files changed, 147 insertions(+), 88 deletions(-)

[PATCH 1/7] 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>
---
 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 e0d909d35763..5bc75cff3536 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3095,6 +3095,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 85b7d087eb45..6a1be2fcdfe7 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1994,7 +1994,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.
@@ -2003,11 +2003,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 the 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;
@@ -2148,7 +2148,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) {
@@ -2256,8 +2256,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.16.4

[PATCH 2/7] btrfs: basic direct I/O read operation

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

We will be getting rid of calling blockdev_direct_io. So, we need
to make sure we perform btrfs_direct_read() to cover the reads.

Introduce iomap begin function which calls get_iomap() to convert
the position to iomap. Use iomap_dio_rw() to perform the direct
reads.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/file.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 435a502a3226..eede9dcbb4b6 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -16,6 +16,7 @@
 #include <linux/btrfs.h>
 #include <linux/uio.h>
 #include <linux/iversion.h>
+#include <linux/iomap.h>
 #include "ctree.h"
 #include "disk-io.h"
 #include "transaction.h"
@@ -3415,6 +3416,57 @@ static int find_desired_extent(struct inode *inode, loff_t *offset, int whence)
 	return ret;
 }
 
+/*
+ * get_iomap: Get the block map and fill the iomap structure
+ * @pos: file position
+ * @length: I/O length
+ * @iomap: The iomap structure to fill
+ */
+
+static int get_iomap(struct inode *inode, loff_t pos, loff_t length,
+		struct iomap *iomap)
+{
+	struct extent_map *em;
+	iomap->addr = IOMAP_NULL_ADDR;
+	em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, pos, length, 0);
+	if (IS_ERR(em))
+		return PTR_ERR(em);
+	/* XXX Do we need to check for em->flags here? */
+	if (em->block_start == EXTENT_MAP_HOLE) {
+		iomap->type = IOMAP_HOLE;
+	} else {
+		iomap->addr = em->block_start;
+		iomap->type = IOMAP_MAPPED;
+	}
+	iomap->offset = em->start;
+	iomap->bdev = em->bdev;
+	iomap->length = em->len;
+	free_extent_map(em);
+	return 0;
+}
+
+static int btrfs_dio_iomap_begin(struct inode *inode, loff_t pos,
+		loff_t length, unsigned flags, struct iomap *iomap,
+		struct iomap *srcmap)
+{
+	return get_iomap(inode, pos, length, iomap);
+}
+
+static const struct iomap_ops btrfs_dio_iomap_ops = {
+	.iomap_begin            = btrfs_dio_iomap_begin,
+};
+
+static ssize_t btrfs_dio_iomap_read(struct kiocb *iocb, struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+	ssize_t ret;
+	inode_lock_shared(inode);
+	ret = iomap_dio_rw(iocb, to, &btrfs_dio_iomap_ops, NULL,
+			is_sync_kiocb(iocb));
+	inode_unlock_shared(inode);
+	return ret;
+}
+
 static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int whence)
 {
 	struct inode *inode = file->f_mapping->host;
@@ -3452,9 +3504,20 @@ 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)
+		ret = btrfs_dio_iomap_read(iocb, to);
+	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,
-- 
2.16.4

Re: [PATCH 2/7] btrfs: basic direct I/O read operation

[Christoph Hellwig]

On Fri, Nov 15, 2019 at 10:16:55AM -0600, Goldwyn Rodrigues wrote:
> +/*
> + * get_iomap: Get the block map and fill the iomap structure
> + * @pos: file position
> + * @length: I/O length
> + * @iomap: The iomap structure to fill
> + */
> +
> +static int get_iomap(struct inode *inode, loff_t pos, loff_t length,
> +		struct iomap *iomap)

The function name probably wants a btrfs_ prefix.

> +{
> +	struct extent_map *em;
> +	iomap->addr = IOMAP_NULL_ADDR;

Please add an empty line after the variable declaration.

> +static int btrfs_dio_iomap_begin(struct inode *inode, loff_t pos,
> +		loff_t length, unsigned flags, struct iomap *iomap,
> +		struct iomap *srcmap)
> +{
> +	return get_iomap(inode, pos, length, iomap);
> +}

Or do we even need the separate helper for now?

[PATCH 3/7] iomap: use a function pointer for dio submits

[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>
---
 fs/iomap/direct-io.c  | 14 +++++++++-----
 include/linux/iomap.h |  2 ++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 2f88d64c2a4d..1e93a8d99439 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,11 @@ 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(bio,
+				file_inode(dio->iocb->ki_filp),	pos);
+	else
+		dio->submit.cookie = submit_bio(bio);
 }
 
 static ssize_t iomap_dio_complete(struct iomap_dio *dio)
@@ -191,7 +195,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
@@ -297,11 +301,11 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		iov_iter_advance(dio->submit.iter, n);
 
 		dio->size += n;
-		pos += n;
 		copied += n;
 
 		nr_pages = iov_iter_npages(&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..386ac8ff58d0 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 bio *bio, struct inode *inode,
+			  loff_t file_offset);
 };
 
 ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
-- 
2.16.4

Re: [PATCH 3/7] iomap: use a function pointer for dio submits

[Christoph Hellwig]

The subject is a little odd, as we only optionally override submit_bio.
Maybe something like:

iomap: add a file system hook for direct I/O bio submission

On Fri, Nov 15, 2019 at 10:16:56AM -0600, Goldwyn Rodrigues wrote:
> 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.

You can use up to 73 chars for the commit log.

> +	if (dio->dops && dio->dops->submit_io)
> +		dio->submit.cookie = dio->dops->submit_io(bio,
> +				file_inode(dio->iocb->ki_filp),	pos);

Any reason to not simply pass the file?

Re: [PATCH 3/7] iomap: use a function pointer for dio submits

[Goldwyn Rodrigues]

On  8:47 15/11, Christoph Hellwig wrote:
> The subject is a little odd, as we only optionally override submit_bio.
> Maybe something like:
> 
> iomap: add a file system hook for direct I/O bio submission
> 
> On Fri, Nov 15, 2019 at 10:16:56AM -0600, Goldwyn Rodrigues wrote:
> > 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.
> 
> You can use up to 73 chars for the commit log.
> 
> > +	if (dio->dops && dio->dops->submit_io)
> > +		dio->submit.cookie = dio->dops->submit_io(bio,
> > +				file_inode(dio->iocb->ki_filp),	pos);
> 
> Any reason to not simply pass the file?

No, I just didn't change the original btrfs_submit_direct(). Passing
filp will be much neater. I will incorporate this.

I agree with all of your comments, and will implement them in the next
patchset.

-- 
Goldwyn

[PATCH 4/7] btrfs: Use iomap_dio_rw() for direct I/O

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This is the main patch to switch call from
__blockdev_direct_IO() to iomap_dio_rw(). In this patch:

Removed buffer_head references
Removed inode_dio_begin() and inode_dio_end() functions since
they are called in iomap_dio_rw().
Renamed btrfs_get_blocks_direct() to direct_iomap_begin() and
used it as iomap_begin()
address_space.direct_IO now is a noop since direct_IO is called
from __btrfs_write_direct().

Removed flags parameter used for __blockdev_direct_IO(). iomap is
capable of direct I/O reads from a hole, so we don't need to
return -ENOENT.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index fe2b8765d9e6..cde8423673fc 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2903,6 +2903,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 eede9dcbb4b6..d47da00fa61e 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1835,7 +1835,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;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c6dc4dd16cf7..e2e4dfb7a568 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8,6 +8,7 @@
 #include <linux/buffer_head.h>
 #include <linux/file.h>
 #include <linux/fs.h>
+#include <linux/iomap.h>
 #include <linux/pagemap.h>
 #include <linux/highmem.h>
 #include <linux/time.h>
@@ -7619,28 +7620,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)
-{
-	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 = em->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)
@@ -7702,7 +7682,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)) {
@@ -7713,15 +7692,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 = em->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.
@@ -7737,17 +7707,18 @@ 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 direct_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;
+	int create = flags & IOMAP_WRITE;
 	int ret = 0;
+	u64 len = length;
 
 	if (!create)
 		len = min_t(u64, len, fs_info->sectorsize);
@@ -7803,7 +7774,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 	}
 
 	if (create) {
-		ret = btrfs_get_blocks_direct_write(&em, bh_result, inode,
+		ret = btrfs_get_blocks_direct_write(&em, inode,
 						    dio_data, start, len);
 		if (ret < 0)
 			goto unlock_err;
@@ -7811,19 +7782,13 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 		unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart,
 				     lockend, &cached_state);
 	} 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;
-		}
+
+		len = min(len, em->len - (start - em->start));
 		/*
 		 * 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;
+		lockstart = start + len;
 		if (lockstart < lockend) {
 			unlock_extent_cached(&BTRFS_I(inode)->io_tree,
 					     lockstart, lockend, &cached_state);
@@ -7832,6 +7797,18 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 		}
 	}
 
+	if ((em->block_start == EXTENT_MAP_HOLE) ||
+	    (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) && !create)) {
+		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 = em->bdev;
+	iomap->length = min(len, em->len - (start - em->start));
+
 	free_extent_map(em);
 
 	return 0;
@@ -8199,9 +8176,8 @@ static void btrfs_endio_direct_read(struct bio *bio)
 	kfree(dip);
 
 	dio_bio->bi_status = err;
-	dio_end_io(dio_bio);
+	bio_endio(dio_bio);
 	btrfs_io_bio_free_csum(io_bio);
-	bio_put(bio);
 }
 
 static void __endio_write_update_ordered(struct inode *inode,
@@ -8263,8 +8239,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_put(bio);
+	bio_endio(dio_bio);
 }
 
 static blk_status_t btrfs_submit_bio_start_direct_io(void *private_data,
@@ -8568,7 +8543,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;
@@ -8587,7 +8562,7 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
 		 * 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);
@@ -8627,7 +8602,11 @@ 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 dio_iomap_ops = {
+	.iomap_begin		= direct_iomap_begin,
+};
+
+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;
@@ -8637,14 +8616,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	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
@@ -8664,11 +8640,7 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		 * 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) {
-			dio_data.overwrite = 1;
-			inode_unlock(inode);
-			relock = true;
-		} else if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (iocb->ki_flags & IOCB_NOWAIT) {
 			ret = -EAGAIN;
 			goto out;
 		}
@@ -8690,15 +8662,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		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, &dio_iomap_ops, NULL, is_sync_kiocb(iocb));
+
 	if (iov_iter_rw(iter) == WRITE) {
 		up_read(&BTRFS_I(inode)->dio_sem);
 		current->journal_info = NULL;
@@ -8725,11 +8693,6 @@ 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;
 }
-- 
2.16.4

Re: [PATCH 4/7] btrfs: Use iomap_dio_rw() for direct I/O

[Christoph Hellwig]

On Fri, Nov 15, 2019 at 10:16:57AM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This is the main patch to switch call from
> __blockdev_direct_IO() to iomap_dio_rw(). In this patch:
> 
> Removed buffer_head references
> Removed inode_dio_begin() and inode_dio_end() functions since
> they are called in iomap_dio_rw().
> Renamed btrfs_get_blocks_direct() to direct_iomap_begin() and
> used it as iomap_begin()
> address_space.direct_IO now is a noop since direct_IO is called
> from __btrfs_write_direct().
> 
> Removed flags parameter used for __blockdev_direct_IO(). iomap is
> capable of direct I/O reads from a hole, so we don't need to
> return -ENOENT.

There isn't really any need to describe the low-level changes,
but more what this changes at a high level, and more importantly
the reasons for that.

>  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)

Should this function be renamed as well? btrfs_iomap_begin_write?

> +static int direct_iomap_begin(struct inode *inode, loff_t start,
> +		loff_t length, unsigned flags, struct iomap *iomap,
> +		struct iomap *srcmap)

This needs a btrfs_ prefix.

> +	if ((em->block_start == EXTENT_MAP_HOLE) ||

No need for the inner braces.

> -	dio_end_io(dio_bio);

You removed the only users of dio_end_io and the submit hook in the
old dio code.  Please add a patch to remove those at the end of the
series.

> -				   btrfs_submit_direct, flags);
> +	ret = iomap_dio_rw(iocb, iter, &dio_iomap_ops, NULL, is_sync_kiocb(iocb));

This adds a line > 80 chars.

Re: [PATCH 4/7] btrfs: Use iomap_dio_rw() for direct I/O

[Goldwyn Rodrigues]

On  9:06 15/11, Christoph Hellwig wrote:
> On Fri, Nov 15, 2019 at 10:16:57AM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > This is the main patch to switch call from
> > __blockdev_direct_IO() to iomap_dio_rw(). In this patch:
> > 
> > Removed buffer_head references
> > Removed inode_dio_begin() and inode_dio_end() functions since
> > they are called in iomap_dio_rw().
> > Renamed btrfs_get_blocks_direct() to direct_iomap_begin() and
> > used it as iomap_begin()
> > address_space.direct_IO now is a noop since direct_IO is called
> > from __btrfs_write_direct().
> > 
> > Removed flags parameter used for __blockdev_direct_IO(). iomap is
> > capable of direct I/O reads from a hole, so we don't need to
> > return -ENOENT.
> 
> There isn't really any need to describe the low-level changes,
> but more what this changes at a high level, and more importantly
> the reasons for that.
> 
> >  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)
> 
> Should this function be renamed as well? btrfs_iomap_begin_write?
> 
> > +static int direct_iomap_begin(struct inode *inode, loff_t start,
> > +		loff_t length, unsigned flags, struct iomap *iomap,
> > +		struct iomap *srcmap)
> 
> This needs a btrfs_ prefix.
> 
> > +	if ((em->block_start == EXTENT_MAP_HOLE) ||
> 
> No need for the inner braces.
> 
> > -	dio_end_io(dio_bio);
> 
> You removed the only users of dio_end_io and the submit hook in the
> old dio code.  Please add a patch to remove those at the end of the
> series.

The submit hook is used by f2fs. I will remove the dio_end_io()
function.

-- 
Goldwyn

Re: [PATCH 4/7] btrfs: Use iomap_dio_rw() for direct I/O

[Filipe Manana]

On Fri, Nov 15, 2019 at 4:19 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> This is the main patch to switch call from
> __blockdev_direct_IO() to iomap_dio_rw(). In this patch:
>
> Removed buffer_head references
> Removed inode_dio_begin() and inode_dio_end() functions since
> they are called in iomap_dio_rw().
> Renamed btrfs_get_blocks_direct() to direct_iomap_begin() and
> used it as iomap_begin()
> address_space.direct_IO now is a noop since direct_IO is called
> from __btrfs_write_direct().
>
> Removed flags parameter used for __blockdev_direct_IO(). iomap is
> capable of direct I/O reads from a hole, so we don't need to
> return -ENOENT.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ctree.h |   1 +
>  fs/btrfs/file.c  |   2 +-
>  fs/btrfs/inode.c | 105 ++++++++++++++++++-------------------------------------
>  3 files changed, 36 insertions(+), 72 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index fe2b8765d9e6..cde8423673fc 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2903,6 +2903,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 eede9dcbb4b6..d47da00fa61e 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1835,7 +1835,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;
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c6dc4dd16cf7..e2e4dfb7a568 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8,6 +8,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/file.h>
>  #include <linux/fs.h>
> +#include <linux/iomap.h>
>  #include <linux/pagemap.h>
>  #include <linux/highmem.h>
>  #include <linux/time.h>
> @@ -7619,28 +7620,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)
> -{
> -       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 = em->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)
> @@ -7702,7 +7682,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)) {
> @@ -7713,15 +7692,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 = em->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.
> @@ -7737,17 +7707,18 @@ 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 direct_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;
> +       int create = flags & IOMAP_WRITE;
>         int ret = 0;
> +       u64 len = length;
>
>         if (!create)
>                 len = min_t(u64, len, fs_info->sectorsize);
> @@ -7803,7 +7774,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
>         }
>
>         if (create) {
> -               ret = btrfs_get_blocks_direct_write(&em, bh_result, inode,
> +               ret = btrfs_get_blocks_direct_write(&em, inode,
>                                                     dio_data, start, len);
>                 if (ret < 0)
>                         goto unlock_err;
> @@ -7811,19 +7782,13 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
>                 unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart,
>                                      lockend, &cached_state);
>         } 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;
> -               }
> +
> +               len = min(len, em->len - (start - em->start));
>                 /*
>                  * 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;
> +               lockstart = start + len;
>                 if (lockstart < lockend) {
>                         unlock_extent_cached(&BTRFS_I(inode)->io_tree,
>                                              lockstart, lockend, &cached_state);
> @@ -7832,6 +7797,18 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
>                 }
>         }
>
> +       if ((em->block_start == EXTENT_MAP_HOLE) ||
> +           (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) && !create)) {
> +               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 = em->bdev;
> +       iomap->length = min(len, em->len - (start - em->start));
> +
>         free_extent_map(em);
>
>         return 0;
> @@ -8199,9 +8176,8 @@ static void btrfs_endio_direct_read(struct bio *bio)
>         kfree(dip);
>
>         dio_bio->bi_status = err;
> -       dio_end_io(dio_bio);
> +       bio_endio(dio_bio);
>         btrfs_io_bio_free_csum(io_bio);
> -       bio_put(bio);
>  }
>
>  static void __endio_write_update_ordered(struct inode *inode,
> @@ -8263,8 +8239,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_put(bio);
> +       bio_endio(dio_bio);
>  }
>
>  static blk_status_t btrfs_submit_bio_start_direct_io(void *private_data,
> @@ -8568,7 +8543,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;
> @@ -8587,7 +8562,7 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
>                  * 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);

The comment above also needs to be updated. It explicitly mentions
there's no need to call bio_endio(), but now the code it's calling
that function.

>         }
>         if (bio)
>                 bio_put(bio);
> @@ -8627,7 +8602,11 @@ 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 dio_iomap_ops = {
> +       .iomap_begin            = direct_iomap_begin,
> +};
> +
> +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;
> @@ -8637,14 +8616,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>         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
> @@ -8664,11 +8640,7 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>                  * 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) {
> -                       dio_data.overwrite = 1;
> -                       inode_unlock(inode);
> -                       relock = true;

I'm not familiar with iomap, but a quick grep at iomap/*.c (5.4-rc7)
reveals no call to inode_unlock().
So this change is throwing away the optimization done by commit [1],
which makes a lot of sense and when it landed I observed as well that
it made a great difference (as expected).
Correct me if I'm wrong, but that optimization is now gone isn't it?
So concurrent direct IO writes to a file, that don't expand the
inode's i_size and operate on non-overlapping ranges, will now be
completely serialized.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=38851cc19adbfa1def2b47106d8050a80e0a3673

Thanks


> -               } else if (iocb->ki_flags & IOCB_NOWAIT) {
> +               if (iocb->ki_flags & IOCB_NOWAIT) {
>                         ret = -EAGAIN;
>                         goto out;
>                 }
> @@ -8690,15 +8662,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>                 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, &dio_iomap_ops, NULL, is_sync_kiocb(iocb));
> +
>         if (iov_iter_rw(iter) == WRITE) {
>                 up_read(&BTRFS_I(inode)->dio_sem);
>                 current->journal_info = NULL;
> @@ -8725,11 +8693,6 @@ 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;
>  }
> --
> 2.16.4
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 4/7] btrfs: Use iomap_dio_rw() for direct I/O

[Goldwyn Rodrigues]

On 16:54 18/11, Filipe Manana wrote:
> On Fri, Nov 15, 2019 at 4:19 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> >
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > This is the main patch to switch call from
> > __blockdev_direct_IO() to iomap_dio_rw(). In this patch:
> >
> > Removed buffer_head references
> > Removed inode_dio_begin() and inode_dio_end() functions since
> > they are called in iomap_dio_rw().
> > Renamed btrfs_get_blocks_direct() to direct_iomap_begin() and
> > used it as iomap_begin()
> > address_space.direct_IO now is a noop since direct_IO is called
> > from __btrfs_write_direct().
> >
> > Removed flags parameter used for __blockdev_direct_IO(). iomap is
> > capable of direct I/O reads from a hole, so we don't need to
> > return -ENOENT.
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  fs/btrfs/ctree.h |   1 +
> >  fs/btrfs/file.c  |   2 +-
> >  fs/btrfs/inode.c | 105 ++++++++++++++++++-------------------------------------
> >  3 files changed, 36 insertions(+), 72 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index fe2b8765d9e6..cde8423673fc 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -2903,6 +2903,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 eede9dcbb4b6..d47da00fa61e 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1835,7 +1835,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;
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index c6dc4dd16cf7..e2e4dfb7a568 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/buffer_head.h>
> >  #include <linux/file.h>
> >  #include <linux/fs.h>
> > +#include <linux/iomap.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/highmem.h>
> >  #include <linux/time.h>
> > @@ -7619,28 +7620,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)
> > -{
> > -       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 = em->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)
> > @@ -7702,7 +7682,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)) {
> > @@ -7713,15 +7692,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 = em->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.
> > @@ -7737,17 +7707,18 @@ 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 direct_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;
> > +       int create = flags & IOMAP_WRITE;
> >         int ret = 0;
> > +       u64 len = length;
> >
> >         if (!create)
> >                 len = min_t(u64, len, fs_info->sectorsize);
> > @@ -7803,7 +7774,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> >         }
> >
> >         if (create) {
> > -               ret = btrfs_get_blocks_direct_write(&em, bh_result, inode,
> > +               ret = btrfs_get_blocks_direct_write(&em, inode,
> >                                                     dio_data, start, len);
> >                 if (ret < 0)
> >                         goto unlock_err;
> > @@ -7811,19 +7782,13 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> >                 unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart,
> >                                      lockend, &cached_state);
> >         } 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;
> > -               }
> > +
> > +               len = min(len, em->len - (start - em->start));
> >                 /*
> >                  * 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;
> > +               lockstart = start + len;
> >                 if (lockstart < lockend) {
> >                         unlock_extent_cached(&BTRFS_I(inode)->io_tree,
> >                                              lockstart, lockend, &cached_state);
> > @@ -7832,6 +7797,18 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> >                 }
> >         }
> >
> > +       if ((em->block_start == EXTENT_MAP_HOLE) ||
> > +           (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) && !create)) {
> > +               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 = em->bdev;
> > +       iomap->length = min(len, em->len - (start - em->start));
> > +
> >         free_extent_map(em);
> >
> >         return 0;
> > @@ -8199,9 +8176,8 @@ static void btrfs_endio_direct_read(struct bio *bio)
> >         kfree(dip);
> >
> >         dio_bio->bi_status = err;
> > -       dio_end_io(dio_bio);
> > +       bio_endio(dio_bio);
> >         btrfs_io_bio_free_csum(io_bio);
> > -       bio_put(bio);
> >  }
> >
> >  static void __endio_write_update_ordered(struct inode *inode,
> > @@ -8263,8 +8239,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_put(bio);
> > +       bio_endio(dio_bio);
> >  }
> >
> >  static blk_status_t btrfs_submit_bio_start_direct_io(void *private_data,
> > @@ -8568,7 +8543,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;
> > @@ -8587,7 +8562,7 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
> >                  * 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);
> 
> The comment above also needs to be updated. It explicitly mentions
> there's no need to call bio_endio(), but now the code it's calling
> that function.
> 
> >         }
> >         if (bio)
> >                 bio_put(bio);
> > @@ -8627,7 +8602,11 @@ 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 dio_iomap_ops = {
> > +       .iomap_begin            = direct_iomap_begin,
> > +};
> > +
> > +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;
> > @@ -8637,14 +8616,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> >         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
> > @@ -8664,11 +8640,7 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> >                  * 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) {
> > -                       dio_data.overwrite = 1;
> > -                       inode_unlock(inode);
> > -                       relock = true;
> 
> I'm not familiar with iomap, but a quick grep at iomap/*.c (5.4-rc7)
> reveals no call to inode_unlock().
> So this change is throwing away the optimization done by commit [1],
> which makes a lot of sense and when it landed I observed as well that
> it made a great difference (as expected).
> Correct me if I'm wrong, but that optimization is now gone isn't it?
> So concurrent direct IO writes to a file, that don't expand the
> inode's i_size and operate on non-overlapping ranges, will now be
> completely serialized.

Yes, I did not realize the consequences. Let me check the performance
and if we can rope in this optimization with the existing iomap
codebase. Thanks for pointing it out.

-- 
Goldwyn

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=38851cc19adbfa1def2b47106d8050a80e0a3673
> 
> Thanks
> 
> 
> > -               } else if (iocb->ki_flags & IOCB_NOWAIT) {
> > +               if (iocb->ki_flags & IOCB_NOWAIT) {
> >                         ret = -EAGAIN;
> >                         goto out;
> >                 }
> > @@ -8690,15 +8662,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> >                 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, &dio_iomap_ops, NULL, is_sync_kiocb(iocb));
> > +
> >         if (iov_iter_rw(iter) == WRITE) {
> >                 up_read(&BTRFS_I(inode)->dio_sem);
> >                 current->journal_info = NULL;
> > @@ -8725,11 +8693,6 @@ 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;
> >  }
> > --
> > 2.16.4
> >
> 
> 
> -- 
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”

-- 
Goldwyn

Re: [PATCH 4/7] btrfs: Use iomap_dio_rw() for direct I/O

[Darrick J. Wong]

On Tue, Nov 19, 2019 at 11:01:29AM -0600, Goldwyn Rodrigues wrote:
> On 16:54 18/11, Filipe Manana wrote:
> > On Fri, Nov 15, 2019 at 4:19 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > >
> > > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > >
> > > This is the main patch to switch call from
> > > __blockdev_direct_IO() to iomap_dio_rw(). In this patch:
> > >
> > > Removed buffer_head references
> > > Removed inode_dio_begin() and inode_dio_end() functions since
> > > they are called in iomap_dio_rw().
> > > Renamed btrfs_get_blocks_direct() to direct_iomap_begin() and
> > > used it as iomap_begin()
> > > address_space.direct_IO now is a noop since direct_IO is called
> > > from __btrfs_write_direct().
> > >
> > > Removed flags parameter used for __blockdev_direct_IO(). iomap is
> > > capable of direct I/O reads from a hole, so we don't need to
> > > return -ENOENT.
> > >
> > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > > ---
> > >  fs/btrfs/ctree.h |   1 +
> > >  fs/btrfs/file.c  |   2 +-
> > >  fs/btrfs/inode.c | 105 ++++++++++++++++++-------------------------------------
> > >  3 files changed, 36 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > > index fe2b8765d9e6..cde8423673fc 100644
> > > --- a/fs/btrfs/ctree.h
> > > +++ b/fs/btrfs/ctree.h
> > > @@ -2903,6 +2903,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 eede9dcbb4b6..d47da00fa61e 100644
> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -1835,7 +1835,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;
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index c6dc4dd16cf7..e2e4dfb7a568 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -8,6 +8,7 @@
> > >  #include <linux/buffer_head.h>
> > >  #include <linux/file.h>
> > >  #include <linux/fs.h>
> > > +#include <linux/iomap.h>
> > >  #include <linux/pagemap.h>
> > >  #include <linux/highmem.h>
> > >  #include <linux/time.h>
> > > @@ -7619,28 +7620,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)
> > > -{
> > > -       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 = em->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)
> > > @@ -7702,7 +7682,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)) {
> > > @@ -7713,15 +7692,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 = em->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.
> > > @@ -7737,17 +7707,18 @@ 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 direct_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;
> > > +       int create = flags & IOMAP_WRITE;
> > >         int ret = 0;
> > > +       u64 len = length;
> > >
> > >         if (!create)
> > >                 len = min_t(u64, len, fs_info->sectorsize);
> > > @@ -7803,7 +7774,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> > >         }
> > >
> > >         if (create) {
> > > -               ret = btrfs_get_blocks_direct_write(&em, bh_result, inode,
> > > +               ret = btrfs_get_blocks_direct_write(&em, inode,
> > >                                                     dio_data, start, len);
> > >                 if (ret < 0)
> > >                         goto unlock_err;
> > > @@ -7811,19 +7782,13 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> > >                 unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart,
> > >                                      lockend, &cached_state);
> > >         } 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;
> > > -               }
> > > +
> > > +               len = min(len, em->len - (start - em->start));
> > >                 /*
> > >                  * 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;
> > > +               lockstart = start + len;
> > >                 if (lockstart < lockend) {
> > >                         unlock_extent_cached(&BTRFS_I(inode)->io_tree,
> > >                                              lockstart, lockend, &cached_state);
> > > @@ -7832,6 +7797,18 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> > >                 }
> > >         }
> > >
> > > +       if ((em->block_start == EXTENT_MAP_HOLE) ||
> > > +           (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) && !create)) {
> > > +               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 = em->bdev;
> > > +       iomap->length = min(len, em->len - (start - em->start));
> > > +
> > >         free_extent_map(em);
> > >
> > >         return 0;
> > > @@ -8199,9 +8176,8 @@ static void btrfs_endio_direct_read(struct bio *bio)
> > >         kfree(dip);
> > >
> > >         dio_bio->bi_status = err;
> > > -       dio_end_io(dio_bio);
> > > +       bio_endio(dio_bio);
> > >         btrfs_io_bio_free_csum(io_bio);
> > > -       bio_put(bio);
> > >  }
> > >
> > >  static void __endio_write_update_ordered(struct inode *inode,
> > > @@ -8263,8 +8239,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_put(bio);
> > > +       bio_endio(dio_bio);
> > >  }
> > >
> > >  static blk_status_t btrfs_submit_bio_start_direct_io(void *private_data,
> > > @@ -8568,7 +8543,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;
> > > @@ -8587,7 +8562,7 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
> > >                  * 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);
> > 
> > The comment above also needs to be updated. It explicitly mentions
> > there's no need to call bio_endio(), but now the code it's calling
> > that function.
> > 
> > >         }
> > >         if (bio)
> > >                 bio_put(bio);
> > > @@ -8627,7 +8602,11 @@ 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 dio_iomap_ops = {
> > > +       .iomap_begin            = direct_iomap_begin,
> > > +};
> > > +
> > > +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;
> > > @@ -8637,14 +8616,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> > >         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
> > > @@ -8664,11 +8640,7 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> > >                  * 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) {
> > > -                       dio_data.overwrite = 1;
> > > -                       inode_unlock(inode);
> > > -                       relock = true;
> > 
> > I'm not familiar with iomap, but a quick grep at iomap/*.c (5.4-rc7)
> > reveals no call to inode_unlock().

iomap doesn't handle locking at all -- it's the caller's responsibility
to set up the locking correctly before calling iomap_dio_rw.

> > So this change is throwing away the optimization done by commit [1],
> > which makes a lot of sense and when it landed I observed as well that
> > it made a great difference (as expected).
> > Correct me if I'm wrong, but that optimization is now gone isn't it?
> > So concurrent direct IO writes to a file, that don't expand the
> > inode's i_size and operate on non-overlapping ranges, will now be
> > completely serialized.

Yeah, I was thinking that too...

> Yes, I did not realize the consequences. Let me check the performance
> and if we can rope in this optimization with the existing iomap
> codebase. Thanks for pointing it out.

...though I see in the old code you'd drop the inode lock before calling
__blockdev_direct_IO, which (I hope) implies that it's safe to grab an
extent mapping and turn that into a struct bio without holding the inode
lock.  If that's the case, there's no reason why you can't do the same
thing with iomap.

--D

> -- 
> Goldwyn
> 
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=38851cc19adbfa1def2b47106d8050a80e0a3673
> > 
> > Thanks
> > 
> > 
> > > -               } else if (iocb->ki_flags & IOCB_NOWAIT) {
> > > +               if (iocb->ki_flags & IOCB_NOWAIT) {
> > >                         ret = -EAGAIN;
> > >                         goto out;
> > >                 }
> > > @@ -8690,15 +8662,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> > >                 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, &dio_iomap_ops, NULL, is_sync_kiocb(iocb));
> > > +
> > >         if (iov_iter_rw(iter) == WRITE) {
> > >                 up_read(&BTRFS_I(inode)->dio_sem);
> > >                 current->journal_info = NULL;
> > > @@ -8725,11 +8693,6 @@ 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;
> > >  }
> > > --
> > > 2.16.4
> > >
> > 
> > 
> > -- 
> > Filipe David Manana,
> > 
> > “Whether you think you can, or you think you can't — you're right.”
> 
> -- 
> Goldwyn

[PATCH 5/7] btrfs: Use iomap_dio_ops.submit_io()

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Use iomap_dio_ops.submit_io() to submit the bio for btrfs.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e2e4dfb7a568..8e55b0d343bd 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8471,7 +8471,7 @@ 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,
+static blk_qc_t btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
 				loff_t file_offset)
 {
 	struct btrfs_dio_private *dip = NULL;
@@ -8524,7 +8524,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);
 
@@ -8567,6 +8567,7 @@ static void btrfs_submit_direct(struct bio *dio_bio, struct inode *inode,
 	if (bio)
 		bio_put(bio);
 	kfree(dip);
+	return BLK_QC_T_NONE;
 }
 
 static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
@@ -8606,6 +8607,10 @@ static const struct iomap_ops dio_iomap_ops = {
 	.iomap_begin		= direct_iomap_begin,
 };
 
+static struct iomap_dio_ops btrfs_dops = {
+	.submit_io		= btrfs_submit_direct,
+};
+
 ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
@@ -8665,7 +8670,8 @@ ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		flags = DIO_LOCKING | DIO_SKIP_HOLES;
 	}
 
-	ret = iomap_dio_rw(iocb, iter, &dio_iomap_ops, NULL, is_sync_kiocb(iocb));
+	ret = iomap_dio_rw(iocb, iter, &dio_iomap_ops, &btrfs_dops,
+			   is_sync_kiocb(iocb));
 
 	if (iov_iter_rw(iter) == WRITE) {
 		up_read(&BTRFS_I(inode)->dio_sem);
-- 
2.16.4

Re: [PATCH 5/7] btrfs: Use iomap_dio_ops.submit_io()

[Christoph Hellwig]

On Fri, Nov 15, 2019 at 10:16:58AM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Use iomap_dio_ops.submit_io() to submit the bio for btrfs.

Doesn't this belong into the previous patch?

[PATCH 6/7] btrfs: flush dirty pages on compressed I/O for dio

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Port of "41bd9ca459a0 Btrfs: just do dirty page flush for the
inode with compression before direct IO"

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8e55b0d343bd..6654370168ff 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7726,6 +7726,17 @@ static int direct_iomap_begin(struct inode *inode, loff_t start,
 	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))
+		filemap_fdatawrite_range(inode->i_mapping, lockstart, lockend);
+
+
 	if (current->journal_info) {
 		/*
 		 * Need to pull our outstanding extents and set journal_info to NULL so
-- 
2.16.4

Re: [PATCH 6/7] btrfs: flush dirty pages on compressed I/O for dio

[Christoph Hellwig]

On Fri, Nov 15, 2019 at 10:16:59AM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Port of "41bd9ca459a0 Btrfs: just do dirty page flush for the
> inode with compression before direct IO"

Doesn't this belong into the main patch as well?

[PATCH 7/7] btrfs: Wait for extent bits to release page

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

While trying to release a page, the extent containing the page may be locked
which would stop the page from being released. Wait for the
extent lock to be cleared, if blocking is allowed and then clear
the bits.

This is avoid warnings coming iomap->dio_rw() ->
invalidate_inode_pages2_range() -> invalidate_complete_page2() ->
try_to_release_page() results in stale pagecache warning.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

---
 fs/btrfs/extent_io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index cceaf05aada2..57b37463da48 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4370,8 +4370,14 @@ static int try_release_extent_state(struct extent_io_tree *tree,
 	int ret = 1;
 
 	if (test_range_bit(tree, start, end, EXTENT_LOCKED, 0, NULL)) {
-		ret = 0;
+		if (gfpflags_allow_blocking(mask)) {
+			wait_extent_bit(tree, start, end, EXTENT_LOCKED);
+			goto clear_bits;
+		} else {
+			ret = 0;
+		}
 	} else {
+clear_bits:
 		/*
 		 * at this point we can safely clear everything except the
 		 * locked bit and the nodatasum bit
-- 
2.16.4

Re: [PATCH 7/7] btrfs: Wait for extent bits to release page

[Christoph Hellwig]

On Fri, Nov 15, 2019 at 10:17:00AM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> While trying to release a page, the extent containing the page may be locked
> which would stop the page from being released. Wait for the
> extent lock to be cleared, if blocking is allowed and then clear
> the bits.
> 
> This is avoid warnings coming iomap->dio_rw() ->
> invalidate_inode_pages2_range() -> invalidate_complete_page2() ->
> try_to_release_page() results in stale pagecache warning.

I can't really comment on the technical aspects of this patch as I don't
know btrfs well enough for that, but try_release_extent_state already
is written in a rather convoluted way, and the additions in this patch
don't make that any better.

I think we want something like the version below.  I can also send you
a patch with just the cleanup bits, so that you can add the actual
changes on top.

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index cceaf05aada2..4dc4b4c57d7c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4367,28 +4367,24 @@ static int try_release_extent_state(struct extent_io_tree *tree,
 {
 	u64 start = page_offset(page);
 	u64 end = start + PAGE_SIZE - 1;
-	int ret = 1;
 
 	if (test_range_bit(tree, start, end, EXTENT_LOCKED, 0, NULL)) {
-		ret = 0;
-	} else {
-		/*
-		 * at this point we can safely clear everything except the
-		 * locked bit and the nodatasum bit
-		 */
-		ret = __clear_extent_bit(tree, start, end,
-				 ~(EXTENT_LOCKED | EXTENT_NODATASUM),
-				 0, 0, NULL, mask, NULL);
-
-		/* if clear_extent_bit failed for enomem reasons,
-		 * we can't allow the release to continue.
-		 */
-		if (ret < 0)
-			ret = 0;
-		else
-			ret = 1;
+		if (!gfpflags_allow_blocking(mask))
+			return 0;
+		wait_extent_bit(tree, start, end, EXTENT_LOCKED);
 	}
-	return ret;
+
+	/*
+	 * At this point we can safely clear everything except the locked and
+	 * nodatasum bits.  But if clear_extent_bit failed because we are out
+	 * of memory, we can't allow the release to continue.
+	 */
+	if (__clear_extent_bit(tree, start, end,
+			~(EXTENT_LOCKED | EXTENT_NODATASUM), 0, 0, NULL,
+			mask, NULL) < 0)
+		return 0;
+
+	return 1;
 }
 
 /*