[PATCH 1/2 RESEND] Btrfs: make plug in writing meta blocks really work

[PATCH 1/2 RESEND] Btrfs: make plug in writing meta blocks really work

[Liu Bo]

We have started plug in btrfs_write_and_wait_marked_extents() but the
generated IOs actually go to device's schedule IO list where the work
is doing in another task, thus the started plug doesn't make any
sense.

And since we wait for IOs immediately after writing meta blocks, it's
the same case as writing log tree, doing sync submit can merge more
IOs.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/disk-io.c     | 6 ++++--
 fs/btrfs/transaction.c | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 080e2eb..8d097ba 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1005,8 +1005,10 @@ static blk_status_t __btree_submit_bio_done(void *private_data, struct bio *bio,
 	return ret;
 }
 
-static int check_async_write(unsigned long bio_flags)
+static int check_async_write(struct btrfs_inode *bi, unsigned long bio_flags)
 {
+	if (atomic_read(&bi->sync_writers))
+		return 0;
 	if (bio_flags & EXTENT_BIO_TREE_LOG)
 		return 0;
 #ifdef CONFIG_X86
@@ -1022,7 +1024,7 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
 {
 	struct inode *inode = private_data;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	int async = check_async_write(bio_flags);
+	int async = check_async_write(BTRFS_I(inode), bio_flags);
 	blk_status_t ret;
 
 	if (bio_op(bio) != REQ_OP_WRITE) {
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index f615d59..9c5f126 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -950,6 +950,7 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 	u64 start = 0;
 	u64 end;
 
+	atomic_inc(&BTRFS_I(fs_info->btree_inode)->sync_writers);
 	while (!find_first_extent_bit(dirty_pages, start, &start, &end,
 				      mark, &cached_state)) {
 		bool wait_writeback = false;
@@ -985,6 +986,7 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 		cond_resched();
 		start = end + 1;
 	}
+	atomic_dec(&BTRFS_I(fs_info->btree_inode)->sync_writers);
 	return werr;
 }
 
-- 
2.9.4

[PATCH 2/2] Btrfs: remove bio_flags which indicates a meta block of log-tree

[Liu Bo]

Since both committing transaction and writing log-tree are doing
plugging on metadata IO, we can unify to use %sync_writers to benefit
both cases, instead of checking bio_flags while writing meta blocks of
log-tree.

This removes the bio_flags related stuff for writing log-tree.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/disk-io.c   |  6 ++----
 fs/btrfs/extent_io.c | 13 ++-----------
 fs/btrfs/extent_io.h |  1 -
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8d097ba..b0e353e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1005,12 +1005,10 @@ static blk_status_t __btree_submit_bio_done(void *private_data, struct bio *bio,
 	return ret;
 }
 
-static int check_async_write(struct btrfs_inode *bi, unsigned long bio_flags)
+static int check_async_write(struct btrfs_inode *bi)
 {
 	if (atomic_read(&bi->sync_writers))
 		return 0;
-	if (bio_flags & EXTENT_BIO_TREE_LOG)
-		return 0;
 #ifdef CONFIG_X86
 	if (static_cpu_has(X86_FEATURE_XMM4_2))
 		return 0;
@@ -1024,7 +1022,7 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
 {
 	struct inode *inode = private_data;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	int async = check_async_write(BTRFS_I(inode), bio_flags);
+	int async = check_async_write(BTRFS_I(inode));
 	blk_status_t ret;
 
 	if (bio_op(bio) != REQ_OP_WRITE) {
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0aff9b2..b61d68f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -110,7 +110,6 @@ struct extent_page_data {
 	struct bio *bio;
 	struct extent_io_tree *tree;
 	get_extent_t *get_extent;
-	unsigned long bio_flags;
 
 	/* tells writepage not to lock the state bits for this range
 	 * it still does the unlocking
@@ -3713,7 +3712,6 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 	u64 offset = eb->start;
 	u32 nritems;
 	unsigned long i, num_pages;
-	unsigned long bio_flags = 0;
 	unsigned long start, end;
 	int write_flags = (epd->sync_io ? REQ_SYNC : 0) | REQ_META;
 	int ret = 0;
@@ -3721,8 +3719,6 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 	clear_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags);
 	num_pages = num_extent_pages(eb->start, eb->len);
 	atomic_set(&eb->io_pages, num_pages);
-	if (btrfs_header_owner(eb) == BTRFS_TREE_LOG_OBJECTID)
-		bio_flags = EXTENT_BIO_TREE_LOG;
 
 	/* set btree blocks beyond nritems with 0 to avoid stale content. */
 	nritems = btrfs_header_nritems(eb);
@@ -3749,8 +3745,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 					 p, offset >> 9, PAGE_SIZE, 0, bdev,
 					 &epd->bio,
 					 end_bio_extent_buffer_writepage,
-					 0, epd->bio_flags, bio_flags, false);
-		epd->bio_flags = bio_flags;
+					 0, 0, 0, false);
 		if (ret) {
 			set_btree_ioerr(p);
 			if (PageWriteback(p))
@@ -3787,7 +3782,6 @@ int btree_write_cache_pages(struct address_space *mapping,
 		.tree = tree,
 		.extent_locked = 0,
 		.sync_io = wbc->sync_mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 	int ret = 0;
 	int done = 0;
@@ -4063,7 +4057,7 @@ static void flush_epd_write_bio(struct extent_page_data *epd)
 		bio_set_op_attrs(epd->bio, REQ_OP_WRITE,
 				 epd->sync_io ? REQ_SYNC : 0);
 
-		ret = submit_one_bio(epd->bio, 0, epd->bio_flags);
+		ret = submit_one_bio(epd->bio, 0, 0);
 		BUG_ON(ret < 0); /* -ENOMEM */
 		epd->bio = NULL;
 	}
@@ -4086,7 +4080,6 @@ int extent_write_full_page(struct extent_io_tree *tree, struct page *page,
 		.get_extent = get_extent,
 		.extent_locked = 0,
 		.sync_io = wbc->sync_mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 
 	ret = __extent_writepage(page, wbc, &epd);
@@ -4111,7 +4104,6 @@ int extent_write_locked_range(struct extent_io_tree *tree, struct inode *inode,
 		.get_extent = get_extent,
 		.extent_locked = 1,
 		.sync_io = mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 	struct writeback_control wbc_writepages = {
 		.sync_mode	= mode,
@@ -4151,7 +4143,6 @@ int extent_writepages(struct extent_io_tree *tree,
 		.get_extent = get_extent,
 		.extent_locked = 0,
 		.sync_io = wbc->sync_mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 
 	ret = extent_write_cache_pages(mapping, wbc, __extent_writepage, &epd,
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 4f03091..353dd3b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -33,7 +33,6 @@
  * type for this bio
  */
 #define EXTENT_BIO_COMPRESSED 1
-#define EXTENT_BIO_TREE_LOG 2
 #define EXTENT_BIO_FLAG_SHIFT 16
 
 /* these are bit numbers for test/set bit */
-- 
2.9.4

Re: [PATCH 1/2 RESEND] Btrfs: make plug in writing meta blocks really work

[David Sterba]

On Mon, Aug 21, 2017 at 03:49:59PM -0600, Liu Bo wrote:
> We have started plug in btrfs_write_and_wait_marked_extents() but the
> generated IOs actually go to device's schedule IO list where the work
> is doing in another task, thus the started plug doesn't make any
> sense.
> 
> And since we wait for IOs immediately after writing meta blocks, it's
> the same case as writing log tree, doing sync submit can merge more
> IOs.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>

The original patch c6adc9cc082e3 adds too many randomly scattered
blg_plugs that, we could use some high-level comments about the purpose.

Re: [PATCH 2/2] Btrfs: remove bio_flags which indicates a meta block of log-tree

[David Sterba]

On Mon, Aug 21, 2017 at 03:50:00PM -0600, Liu Bo wrote:
> Since both committing transaction and writing log-tree are doing
> plugging on metadata IO, we can unify to use %sync_writers to benefit
> both cases, instead of checking bio_flags while writing meta blocks of
> log-tree.
> 
> This removes the bio_flags related stuff for writing log-tree.

I'm not sure if this preserves the intentions introduced in
de0022b9da616b95ea5, as it's meant to do the checksums synchronously
(without offloading to other threads), if it's for the tree-log. So now
it depends on the sync_writers count to do that, while with the bio flag
it was independent.

The sync_writers is per-inode, while the bio flag is from the
submit_bio hook, so it's per-context.

Re: [PATCH 1/2 RESEND] Btrfs: make plug in writing meta blocks really work

[Liu Bo]

On Wed, Sep 13, 2017 at 05:51:55PM +0200, David Sterba wrote:
> On Mon, Aug 21, 2017 at 03:49:59PM -0600, Liu Bo wrote:
> > We have started plug in btrfs_write_and_wait_marked_extents() but the
> > generated IOs actually go to device's schedule IO list where the work
> > is doing in another task, thus the started plug doesn't make any
> > sense.
> > 
> > And since we wait for IOs immediately after writing meta blocks, it's
> > the same case as writing log tree, doing sync submit can merge more
> > IOs.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> 
> Reviewed-by: David Sterba <dsterba@suse.com>
> 
> The original patch c6adc9cc082e3 adds too many randomly scattered
> blg_plugs that, we could use some high-level comments about the purpose.

The commit log of that patch wasn't explaining well why the old way is
not efficient, in most cases, cow'd metadata blocks in the log tree
and the log root tree are contiguous on disk, so putting a blk_plug
pair can merge writes on those meta blocks so that we could get better
performance out of it.

And yes, adding comments mentioning the reason will be helpful.

Thanks,

-liubo

Re: [PATCH 2/2] Btrfs: remove bio_flags which indicates a meta block of log-tree

[Liu Bo]

On Wed, Sep 13, 2017 at 06:43:30PM +0200, David Sterba wrote:
> On Mon, Aug 21, 2017 at 03:50:00PM -0600, Liu Bo wrote:
> > Since both committing transaction and writing log-tree are doing
> > plugging on metadata IO, we can unify to use %sync_writers to benefit
> > both cases, instead of checking bio_flags while writing meta blocks of
> > log-tree.
> > 
> > This removes the bio_flags related stuff for writing log-tree.
> 
> I'm not sure if this preserves the intentions introduced in
> de0022b9da616b95ea5, as it's meant to do the checksums synchronously
> (without offloading to other threads), if it's for the tree-log. So now
> it depends on the sync_writers count to do that, while with the bio flag
> it was independent.
>
> The sync_writers is per-inode, while the bio flag is from the
> submit_bio hook, so it's per-context.


Again, blame me for not making the commit log clear enough.

We can remove this bio_flags because in order to write dirty blocks,
log tree also uses btrfs_write_marked_extents(), inside which PATCH 1
has enabled %sync_writers, therefore, every write goes in a
synchronous way, so does checksuming.

WRT per-inode vs per-context, yes, they're a bit different, but the
only overhead I could think of is that

1) while log tree is flushing its dirty blocks via
   btrfs_write_marked_extents(), in which %sync_writers is increased by one.

2) in the meantime, some writeback operations may happen upon btrfs's
   metadata inode, so these writes go synchronously, too.

However, AFAICS, the overhead is not a big one while the win is that
we unify the two places that needs synchronous way and remove a
special hack/flag.


Thanks,

-liubo

[PATCH 2/2 v2] Btrfs: remove bio_flags which indicates a meta block of log-tree

[Liu Bo]

Since both committing transaction and writing log-tree are doing
plugging on metadata IO, we can unify to use %sync_writers to benefit
both cases, instead of checking bio_flags while writing meta blocks of
log-tree.

We can remove this bio_flags because in order to write dirty blocks,
log tree also uses btrfs_write_marked_extents(), inside which we
has enabled %sync_writers, therefore, every write goes in a
synchronous way, so does checksuming.

Please also note that, bio_flags is applied per-context while
%sync_writers is applied per-inode, so this might incur some overhead, ie.

1) while log tree is flushing its dirty blocks via
   btrfs_write_marked_extents(), in which %sync_writers is increased
   by one.

2) in the meantime, some writeback operations may happen upon btrfs's
   metadata inode, so these writes go synchronously, too.

However, AFAICS, the overhead is not a big one while the win is that
we unify the two places that needs synchronous way and remove a
special hack/flag.

This removes the bio_flags related stuff for writing log-tree.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---

v2: Improve the commit log to offer more details why this change makes
    sense.

 fs/btrfs/disk-io.c   |  6 ++----
 fs/btrfs/extent_io.c | 13 ++-----------
 fs/btrfs/extent_io.h |  1 -
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8d097ba..b0e353e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1005,12 +1005,10 @@ static blk_status_t __btree_submit_bio_done(void *private_data, struct bio *bio,
 	return ret;
 }
 
-static int check_async_write(struct btrfs_inode *bi, unsigned long bio_flags)
+static int check_async_write(struct btrfs_inode *bi)
 {
 	if (atomic_read(&bi->sync_writers))
 		return 0;
-	if (bio_flags & EXTENT_BIO_TREE_LOG)
-		return 0;
 #ifdef CONFIG_X86
 	if (static_cpu_has(X86_FEATURE_XMM4_2))
 		return 0;
@@ -1024,7 +1022,7 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
 {
 	struct inode *inode = private_data;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	int async = check_async_write(BTRFS_I(inode), bio_flags);
+	int async = check_async_write(BTRFS_I(inode));
 	blk_status_t ret;
 
 	if (bio_op(bio) != REQ_OP_WRITE) {
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0aff9b2..b61d68f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -110,7 +110,6 @@ struct extent_page_data {
 	struct bio *bio;
 	struct extent_io_tree *tree;
 	get_extent_t *get_extent;
-	unsigned long bio_flags;
 
 	/* tells writepage not to lock the state bits for this range
 	 * it still does the unlocking
@@ -3713,7 +3712,6 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 	u64 offset = eb->start;
 	u32 nritems;
 	unsigned long i, num_pages;
-	unsigned long bio_flags = 0;
 	unsigned long start, end;
 	int write_flags = (epd->sync_io ? REQ_SYNC : 0) | REQ_META;
 	int ret = 0;
@@ -3721,8 +3719,6 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 	clear_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags);
 	num_pages = num_extent_pages(eb->start, eb->len);
 	atomic_set(&eb->io_pages, num_pages);
-	if (btrfs_header_owner(eb) == BTRFS_TREE_LOG_OBJECTID)
-		bio_flags = EXTENT_BIO_TREE_LOG;
 
 	/* set btree blocks beyond nritems with 0 to avoid stale content. */
 	nritems = btrfs_header_nritems(eb);
@@ -3749,8 +3745,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 					 p, offset >> 9, PAGE_SIZE, 0, bdev,
 					 &epd->bio,
 					 end_bio_extent_buffer_writepage,
-					 0, epd->bio_flags, bio_flags, false);
-		epd->bio_flags = bio_flags;
+					 0, 0, 0, false);
 		if (ret) {
 			set_btree_ioerr(p);
 			if (PageWriteback(p))
@@ -3787,7 +3782,6 @@ int btree_write_cache_pages(struct address_space *mapping,
 		.tree = tree,
 		.extent_locked = 0,
 		.sync_io = wbc->sync_mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 	int ret = 0;
 	int done = 0;
@@ -4063,7 +4057,7 @@ static void flush_epd_write_bio(struct extent_page_data *epd)
 		bio_set_op_attrs(epd->bio, REQ_OP_WRITE,
 				 epd->sync_io ? REQ_SYNC : 0);
 
-		ret = submit_one_bio(epd->bio, 0, epd->bio_flags);
+		ret = submit_one_bio(epd->bio, 0, 0);
 		BUG_ON(ret < 0); /* -ENOMEM */
 		epd->bio = NULL;
 	}
@@ -4086,7 +4080,6 @@ int extent_write_full_page(struct extent_io_tree *tree, struct page *page,
 		.get_extent = get_extent,
 		.extent_locked = 0,
 		.sync_io = wbc->sync_mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 
 	ret = __extent_writepage(page, wbc, &epd);
@@ -4111,7 +4104,6 @@ int extent_write_locked_range(struct extent_io_tree *tree, struct inode *inode,
 		.get_extent = get_extent,
 		.extent_locked = 1,
 		.sync_io = mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 	struct writeback_control wbc_writepages = {
 		.sync_mode	= mode,
@@ -4151,7 +4143,6 @@ int extent_writepages(struct extent_io_tree *tree,
 		.get_extent = get_extent,
 		.extent_locked = 0,
 		.sync_io = wbc->sync_mode == WB_SYNC_ALL,
-		.bio_flags = 0,
 	};
 
 	ret = extent_write_cache_pages(mapping, wbc, __extent_writepage, &epd,
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 4f03091..353dd3b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -33,7 +33,6 @@
  * type for this bio
  */
 #define EXTENT_BIO_COMPRESSED 1
-#define EXTENT_BIO_TREE_LOG 2
 #define EXTENT_BIO_FLAG_SHIFT 16
 
 /* these are bit numbers for test/set bit */
-- 
2.9.4

Re: [PATCH 2/2 v2] Btrfs: remove bio_flags which indicates a meta block of log-tree

[David Sterba]

On Wed, Sep 13, 2017 at 12:18:22PM -0600, Liu Bo wrote:
> Since both committing transaction and writing log-tree are doing
> plugging on metadata IO, we can unify to use %sync_writers to benefit
> both cases, instead of checking bio_flags while writing meta blocks of
> log-tree.
> 
> We can remove this bio_flags because in order to write dirty blocks,
> log tree also uses btrfs_write_marked_extents(), inside which we
> has enabled %sync_writers, therefore, every write goes in a
> synchronous way, so does checksuming.
> 
> Please also note that, bio_flags is applied per-context while
> %sync_writers is applied per-inode, so this might incur some overhead, ie.
> 
> 1) while log tree is flushing its dirty blocks via
>    btrfs_write_marked_extents(), in which %sync_writers is increased
>    by one.
> 
> 2) in the meantime, some writeback operations may happen upon btrfs's
>    metadata inode, so these writes go synchronously, too.
> 
> However, AFAICS, the overhead is not a big one while the win is that
> we unify the two places that needs synchronous way and remove a
> special hack/flag.
> 
> This removes the bio_flags related stuff for writing log-tree.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Much better, thanks.

Reviewed-by: David Sterba <dsterba@suse.com>