[PATCH] Btrfs: make plug in writing meta blocks really work

[PATCH] 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

Re: [PATCH] Btrfs: make plug in writing meta blocks really work

[Josef Bacik]

On Fri, Aug 18, 2017 at 11:42:07AM -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.
> 

We're plugging when we do the per-device scheduled IO right?  So we aren't
really gaining anything by it being async.  Also we do a lot of work between the
time that we start writing the marked extents for the tree-log and when we
actually wait for them, so we really don't want to do a synchronous write out in
that case.  Instead move the sync_writers into write_and_wait_marked_extents.
Thanks,

Josef

Re: [PATCH] Btrfs: make plug in writing meta blocks really work

[Liu Bo]

On Mon, Aug 21, 2017 at 01:48:01PM -0400, Josef Bacik wrote:
> On Fri, Aug 18, 2017 at 11:42:07AM -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.
> > 
> 
> We're plugging when we do the per-device scheduled IO right?

Yes, we are.

> So we aren't
> really gaining anything by it being async.  Also we do a lot of work between the
> time that we start writing the marked extents for the tree-log and when we
> actually wait for them, so we really don't want to do a synchronous write out in
> that case.

Hmm, we've always been doing sync write for meta blocks of log
tree/log root tree, because of EXTENT_BIO_TREE_LOG (introduced in
commit de0022b9da616b95ea5b41eab32da825b0b5150f), and the commit log
claimed about 15% performance gaining in O_SYNC workloads (maybe we
need to re-evaluate it?).

> Instead move the sync_writers into write_and_wait_marked_extents.
> Thanks,

I'm OK with the change, but if sync write benefits both transaction
commit case and log tree case, we can unify them to %sync_writers
instead of a bio_flag.

thanks,
-liubo

Re: [PATCH] Btrfs: make plug in writing meta blocks really work

[Josef Bacik]

On Mon, Aug 21, 2017 at 12:14:16PM -0700, Liu Bo wrote:
> On Mon, Aug 21, 2017 at 01:48:01PM -0400, Josef Bacik wrote:
> > On Fri, Aug 18, 2017 at 11:42:07AM -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.
> > > 
> > 
> > We're plugging when we do the per-device scheduled IO right?
> 
> Yes, we are.
> 
> > So we aren't
> > really gaining anything by it being async.  Also we do a lot of work between the
> > time that we start writing the marked extents for the tree-log and when we
> > actually wait for them, so we really don't want to do a synchronous write out in
> > that case.
> 
> Hmm, we've always been doing sync write for meta blocks of log
> tree/log root tree, because of EXTENT_BIO_TREE_LOG (introduced in
> commit de0022b9da616b95ea5b41eab32da825b0b5150f), and the commit log
> claimed about 15% performance gaining in O_SYNC workloads (maybe we
> need to re-evaluate it?).
> 
> > Instead move the sync_writers into write_and_wait_marked_extents.
> > Thanks,
> 
> I'm OK with the change, but if sync write benefits both transaction
> commit case and log tree case, we can unify them to %sync_writers
> instead of a bio_flag.
> 

Sigh you're right, I forgot about all of that.  Just delete the magic bio flags
stuff and then this is fine.  Thanks,

Josef

Re: [PATCH] Btrfs: make plug in writing meta blocks really work

[Liu Bo]

On Mon, Aug 21, 2017 at 03:23:30PM -0400, Josef Bacik wrote:
> On Mon, Aug 21, 2017 at 12:14:16PM -0700, Liu Bo wrote:
> > On Mon, Aug 21, 2017 at 01:48:01PM -0400, Josef Bacik wrote:
> > > On Fri, Aug 18, 2017 at 11:42:07AM -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.
> > > > 
> > > 
> > > We're plugging when we do the per-device scheduled IO right?
> > 
> > Yes, we are.
> > 
> > > So we aren't
> > > really gaining anything by it being async.  Also we do a lot of work between the
> > > time that we start writing the marked extents for the tree-log and when we
> > > actually wait for them, so we really don't want to do a synchronous write out in
> > > that case.
> > 
> > Hmm, we've always been doing sync write for meta blocks of log
> > tree/log root tree, because of EXTENT_BIO_TREE_LOG (introduced in
> > commit de0022b9da616b95ea5b41eab32da825b0b5150f), and the commit log
> > claimed about 15% performance gaining in O_SYNC workloads (maybe we
> > need to re-evaluate it?).
> > 
> > > Instead move the sync_writers into write_and_wait_marked_extents.
> > > Thanks,
> > 
> > I'm OK with the change, but if sync write benefits both transaction
> > commit case and log tree case, we can unify them to %sync_writers
> > instead of a bio_flag.
> > 
> 
> Sigh you're right, I forgot about all of that.  Just delete the magic bio flags
> stuff and then this is fine.  Thanks,
>

OK, good to know it, thanks for the comments.

thanks,
-liubo