[PATCH 0/3] kill async counters

[PATCH 0/3] kill async counters

[Liu Bo]

%async_delalloc_pages and %nr_async_submits are used to indicate that
compressed writeback actually starts its work, but since we have
already used double filemap_flush() alike, we don't need those two any
more.

%nr_async_bios was used to show congestion for writeback, but a later
change adopts bdi_write_congested() to do the same job, it's not
needed, either.

Liu Bo (3):
  Btrfs: remove nr_async_bios
  Btrfs: do not make defrag wait on async_delalloc_pages
  Btrfs: remove nr_async_submits and async_submit_draining

 fs/btrfs/ctree.h   |  3 ---
 fs/btrfs/disk-io.c | 25 -------------------------
 fs/btrfs/inode.c   | 28 ----------------------------
 fs/btrfs/ioctl.c   | 15 ---------------
 fs/btrfs/volumes.c | 14 --------------
 5 files changed, 85 deletions(-)

-- 
2.9.4

[PATCH 1/3] Btrfs: remove nr_async_bios

[Liu Bo]

This was intended to congest higher layers to not send bios, but as

1) the congested bit has been taken by writeback
2) and no one is waiting for %nr_async_bios down to zero,

we can safely remove this now.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/ctree.h   |  1 -
 fs/btrfs/disk-io.c |  1 -
 fs/btrfs/volumes.c | 14 --------------
 3 files changed, 16 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 3f3eb7b..27cd882 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -881,7 +881,6 @@ struct btrfs_fs_info {
 
 	atomic_t nr_async_submits;
 	atomic_t async_submit_draining;
-	atomic_t nr_async_bios;
 	atomic_t async_delalloc_pages;
 	atomic_t open_ioctl_trans;
 
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f45b61f..95583e2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2657,7 +2657,6 @@ int open_ctree(struct super_block *sb,
 	atomic_set(&fs_info->nr_async_submits, 0);
 	atomic_set(&fs_info->async_delalloc_pages, 0);
 	atomic_set(&fs_info->async_submit_draining, 0);
-	atomic_set(&fs_info->nr_async_bios, 0);
 	atomic_set(&fs_info->defrag_running, 0);
 	atomic_set(&fs_info->qgroup_op_seq, 0);
 	atomic_set(&fs_info->reada_works_cnt, 0);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bd679bc..6e9df4d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -450,13 +450,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
 		pending = pending->bi_next;
 		cur->bi_next = NULL;
 
-		/*
-		 * atomic_dec_return implies a barrier for waitqueue_active
-		 */
-		if (atomic_dec_return(&fs_info->nr_async_bios) < limit &&
-		    waitqueue_active(&fs_info->async_submit_wait))
-			wake_up(&fs_info->async_submit_wait);
-
 		BUG_ON(atomic_read(&cur->__bi_cnt) == 0);
 
 		/*
@@ -6132,13 +6125,6 @@ static noinline void btrfs_schedule_bio(struct btrfs_device *device,
 		return;
 	}
 
-	/*
-	 * nr_async_bios allows us to reliably return congestion to the
-	 * higher layers.  Otherwise, the async bio makes it appear we have
-	 * made progress against dirty pages when we've really just put it
-	 * on a queue for later
-	 */
-	atomic_inc(&fs_info->nr_async_bios);
 	WARN_ON(bio->bi_next);
 	bio->bi_next = NULL;
 
-- 
2.9.4

[PATCH 2/3] Btrfs: do not make defrag wait on async_delalloc_pages

[Liu Bo]

By setting compression for a defrag task, the task will start IO at
the end of defrag.

After the combo of filemap_flush(), we've already made sure that
dirty pages have made progress via async compress thread because the
second filemap_flush() will wait for page lock, which won't be
unlocked until those pages have been marked as writeback and ordered
extents have been queued.

And this is for per-inode defrag, it's not helpful to wait on a global
%async_delalloc_pages and %nr_async_submits from fs_info.

Although waiting on %nr_async_submits means that all bios are
submitted down to per-device schedule IO lists, it doesn't wait for
their completions, thus users still need to do fsync/sync to make sure
the data is on disk.  While with this change, it makes sure that pages
have been marked with writeback bits and will be submitted
asynchronously shortly, therefore, the behavior of defrag option '-c'
remains unchanged.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/ioctl.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index fa1b78c..6dbedd8 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1449,21 +1449,6 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 			filemap_flush(inode->i_mapping);
 	}
 
-	if ((range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
-		/* the filemap_flush will queue IO into the worker threads, but
-		 * we have to make sure the IO is actually started and that
-		 * ordered extents get created before we return
-		 */
-		atomic_inc(&fs_info->async_submit_draining);
-		while (atomic_read(&fs_info->nr_async_submits) ||
-		       atomic_read(&fs_info->async_delalloc_pages)) {
-			wait_event(fs_info->async_submit_wait,
-				   (atomic_read(&fs_info->nr_async_submits) == 0 &&
-				    atomic_read(&fs_info->async_delalloc_pages) == 0));
-		}
-		atomic_dec(&fs_info->async_submit_draining);
-	}
-
 	if (range->compress_type == BTRFS_COMPRESS_LZO) {
 		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
 	}
-- 
2.9.4

[PATCH 3/3] Btrfs: remove nr_async_submits and async_submit_draining

[Liu Bo]

Now that we have the combo of flushing twice, which can make sure IO
have started since the second flush will wait for page lock which
won't be unlocked unless setting page writeback and queuing ordered
extents, we don't need %async_submit_draining, %async_delalloc_pages
and %nr_async_submits to tell whether IO have actually started.

Moreover, all the flushers in use are followed by functions that wait
for ordered extents to complete, so %nr_async_submits, which tracks
whether bio's async submit has made progress, doesn't really make
sense.

However, %async_delalloc_pages is still required by shrink_delalloc()
as that function doesn't flush twice in the normal case (just issues a
writeback with WB_REASON_FS_FREE_SPACE).

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/ctree.h   |  2 --
 fs/btrfs/disk-io.c | 24 ------------------------
 fs/btrfs/inode.c   | 28 ----------------------------
 3 files changed, 54 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 27cd882..a226097 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -879,8 +879,6 @@ struct btrfs_fs_info {
 	rwlock_t tree_mod_log_lock;
 	struct rb_root tree_mod_log;
 
-	atomic_t nr_async_submits;
-	atomic_t async_submit_draining;
 	atomic_t async_delalloc_pages;
 	atomic_t open_ioctl_trans;
 
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 95583e2..fabe302 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -879,22 +879,9 @@ static void run_one_async_start(struct btrfs_work *work)
 
 static void run_one_async_done(struct btrfs_work *work)
 {
-	struct btrfs_fs_info *fs_info;
 	struct async_submit_bio *async;
-	int limit;
 
 	async = container_of(work, struct  async_submit_bio, work);
-	fs_info = async->fs_info;
-
-	limit = btrfs_async_submit_limit(fs_info);
-	limit = limit * 2 / 3;
-
-	/*
-	 * atomic_dec_return implies a barrier for waitqueue_active
-	 */
-	if (atomic_dec_return(&fs_info->nr_async_submits) < limit &&
-	    waitqueue_active(&fs_info->async_submit_wait))
-		wake_up(&fs_info->async_submit_wait);
 
 	/* If an error occurred we just want to clean up the bio and move on */
 	if (async->status) {
@@ -942,19 +929,10 @@ blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 
 	async->status = 0;
 
-	atomic_inc(&fs_info->nr_async_submits);
-
 	if (op_is_sync(bio->bi_opf))
 		btrfs_set_work_high_priority(&async->work);
 
 	btrfs_queue_work(fs_info->workers, &async->work);
-
-	while (atomic_read(&fs_info->async_submit_draining) &&
-	      atomic_read(&fs_info->nr_async_submits)) {
-		wait_event(fs_info->async_submit_wait,
-			   (atomic_read(&fs_info->nr_async_submits) == 0));
-	}
-
 	return 0;
 }
 
@@ -2654,9 +2632,7 @@ int open_ctree(struct super_block *sb,
 	btrfs_init_block_rsv(&fs_info->empty_block_rsv, BTRFS_BLOCK_RSV_EMPTY);
 	btrfs_init_block_rsv(&fs_info->delayed_block_rsv,
 			     BTRFS_BLOCK_RSV_DELOPS);
-	atomic_set(&fs_info->nr_async_submits, 0);
 	atomic_set(&fs_info->async_delalloc_pages, 0);
-	atomic_set(&fs_info->async_submit_draining, 0);
 	atomic_set(&fs_info->defrag_running, 0);
 	atomic_set(&fs_info->qgroup_op_seq, 0);
 	atomic_set(&fs_info->reada_works_cnt, 0);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 24bcd5c..f58528b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1209,13 +1209,6 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 
 		btrfs_queue_work(fs_info->delalloc_workers, &async_cow->work);
 
-		while (atomic_read(&fs_info->async_submit_draining) &&
-		       atomic_read(&fs_info->async_delalloc_pages)) {
-			wait_event(fs_info->async_submit_wait,
-				   (atomic_read(&fs_info->async_delalloc_pages) ==
-				    0));
-		}
-
 		*nr_written += nr_pages;
 		start = cur_end + 1;
 	}
@@ -10235,19 +10228,6 @@ int btrfs_start_delalloc_inodes(struct btrfs_root *root, int delay_iput)
 	ret = __start_delalloc_inodes(root, delay_iput, -1);
 	if (ret > 0)
 		ret = 0;
-	/*
-	 * the filemap_flush will queue IO into the worker threads, but
-	 * we have to make sure the IO is actually started and that
-	 * ordered extents get created before we return
-	 */
-	atomic_inc(&fs_info->async_submit_draining);
-	while (atomic_read(&fs_info->nr_async_submits) ||
-	       atomic_read(&fs_info->async_delalloc_pages)) {
-		wait_event(fs_info->async_submit_wait,
-			   (atomic_read(&fs_info->nr_async_submits) == 0 &&
-			    atomic_read(&fs_info->async_delalloc_pages) == 0));
-	}
-	atomic_dec(&fs_info->async_submit_draining);
 	return ret;
 }
 
@@ -10289,14 +10269,6 @@ int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, int delay_iput,
 	spin_unlock(&fs_info->delalloc_root_lock);
 
 	ret = 0;
-	atomic_inc(&fs_info->async_submit_draining);
-	while (atomic_read(&fs_info->nr_async_submits) ||
-	      atomic_read(&fs_info->async_delalloc_pages)) {
-		wait_event(fs_info->async_submit_wait,
-		   (atomic_read(&fs_info->nr_async_submits) == 0 &&
-		    atomic_read(&fs_info->async_delalloc_pages) == 0));
-	}
-	atomic_dec(&fs_info->async_submit_draining);
 out:
 	if (!list_empty_careful(&splice)) {
 		spin_lock(&fs_info->delalloc_root_lock);
-- 
2.9.4

Re: [PATCH 1/3] Btrfs: remove nr_async_bios

[David Sterba]

On Thu, Sep 07, 2017 at 11:22:20AM -0600, Liu Bo wrote:
> This was intended to congest higher layers to not send bios, but as
> 
> 1) the congested bit has been taken by writeback

Can you please be more specific here?

> 2) and no one is waiting for %nr_async_bios down to zero,
> 
> we can safely remove this now.

>From the original commit it looks like mechanism to avoid some write
patterns (streaming not becoming random), but the commit is from 2008,
lot of things have changed.

I think we should at least document whats' the congestion behaviour we
rely on nowadays, so that' sfor the 1). Otherwise patch looks ok.

> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/ctree.h   |  1 -
>  fs/btrfs/disk-io.c |  1 -
>  fs/btrfs/volumes.c | 14 --------------
>  3 files changed, 16 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 3f3eb7b..27cd882 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -881,7 +881,6 @@ struct btrfs_fs_info {
>  
>  	atomic_t nr_async_submits;
>  	atomic_t async_submit_draining;
> -	atomic_t nr_async_bios;
>  	atomic_t async_delalloc_pages;
>  	atomic_t open_ioctl_trans;
>  
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index f45b61f..95583e2 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2657,7 +2657,6 @@ int open_ctree(struct super_block *sb,
>  	atomic_set(&fs_info->nr_async_submits, 0);
>  	atomic_set(&fs_info->async_delalloc_pages, 0);
>  	atomic_set(&fs_info->async_submit_draining, 0);
> -	atomic_set(&fs_info->nr_async_bios, 0);
>  	atomic_set(&fs_info->defrag_running, 0);
>  	atomic_set(&fs_info->qgroup_op_seq, 0);
>  	atomic_set(&fs_info->reada_works_cnt, 0);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bd679bc..6e9df4d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -450,13 +450,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
>  		pending = pending->bi_next;
>  		cur->bi_next = NULL;
>  
> -		/*
> -		 * atomic_dec_return implies a barrier for waitqueue_active
> -		 */
> -		if (atomic_dec_return(&fs_info->nr_async_bios) < limit &&

And after that the variable 'limit' becomes unused, please remove it as
well.

> -		    waitqueue_active(&fs_info->async_submit_wait))
> -			wake_up(&fs_info->async_submit_wait);
> -
>  		BUG_ON(atomic_read(&cur->__bi_cnt) == 0);
>  
>  		/*
> @@ -6132,13 +6125,6 @@ static noinline void btrfs_schedule_bio(struct btrfs_device *device,
>  		return;
>  	}
>  
> -	/*
> -	 * nr_async_bios allows us to reliably return congestion to the
> -	 * higher layers.  Otherwise, the async bio makes it appear we have
> -	 * made progress against dirty pages when we've really just put it
> -	 * on a queue for later
> -	 */
> -	atomic_inc(&fs_info->nr_async_bios);
>  	WARN_ON(bio->bi_next);
>  	bio->bi_next = NULL;
>  
> -- 
> 2.9.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Btrfs: remove nr_async_submits and async_submit_draining

[David Sterba]

On Thu, Sep 07, 2017 at 11:22:22AM -0600, Liu Bo wrote:
> Now that we have the combo of flushing twice, which can make sure IO
> have started since the second flush will wait for page lock which
> won't be unlocked unless setting page writeback and queuing ordered
> extents, we don't need %async_submit_draining, %async_delalloc_pages
> and %nr_async_submits to tell whether IO have actually started.
> 
> Moreover, all the flushers in use are followed by functions that wait
> for ordered extents to complete, so %nr_async_submits, which tracks
> whether bio's async submit has made progress, doesn't really make
> sense.
> 
> However, %async_delalloc_pages is still required by shrink_delalloc()
> as that function doesn't flush twice in the normal case (just issues a
> writeback with WB_REASON_FS_FREE_SPACE).
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Patches like this are almost impossible to review just from the code.
This has runtime implications that we'd need to observe in real on
various workloads.

So, the patches "look good", but I did not try to forsee all the
scenarios where threads interact through the counters. I think it should
be safe to add them to for-next and give enough time to test them.

The performance has varied wildly in the last cycle(s) so it's hard to
get a baseline, other than another major release. I do expect changes in
performance characteristics but still hope it'll be the same on average.

Re: [PATCH 1/3] Btrfs: remove nr_async_bios

[Liu Bo]

On Wed, Sep 27, 2017 at 01:30:13PM +0200, David Sterba wrote:
> On Thu, Sep 07, 2017 at 11:22:20AM -0600, Liu Bo wrote:
> > This was intended to congest higher layers to not send bios, but as
> > 
> > 1) the congested bit has been taken by writeback
> 
> Can you please be more specific here?
>

Sure, async bios comes from buffered writes and DIO writes.

For DIO writes, we want to submit them ASAP, while for buffered
writes, writeback uses balance_dirty_pages() to throttle how much
dirty pages we can have.

> > 2) and no one is waiting for %nr_async_bios down to zero,
> > 
> > we can safely remove this now.
> 
> From the original commit it looks like mechanism to avoid some write
> patterns (streaming not becoming random), but the commit is from 2008,
> lot of things have changed.

I did check the history, but have to check it again before typing the
following...

IIUC, it was introduced along with changes which makes checksumming
workload spread accross different cpus.  And at that time, seems
pdflush is used instead of per-bdi flush, perhaps pdflush doesn't have
the necessary information for writeback to do throttling, Chris should
answer this better.

> 
> I think we should at least document whats' the congestion behaviour we
> rely on nowadays, so that' sfor the 1). Otherwise patch looks ok.
>

Sounds good.

> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/ctree.h   |  1 -
> >  fs/btrfs/disk-io.c |  1 -
> >  fs/btrfs/volumes.c | 14 --------------
> >  3 files changed, 16 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 3f3eb7b..27cd882 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -881,7 +881,6 @@ struct btrfs_fs_info {
> >  
> >  	atomic_t nr_async_submits;
> >  	atomic_t async_submit_draining;
> > -	atomic_t nr_async_bios;
> >  	atomic_t async_delalloc_pages;
> >  	atomic_t open_ioctl_trans;
> >  
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index f45b61f..95583e2 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -2657,7 +2657,6 @@ int open_ctree(struct super_block *sb,
> >  	atomic_set(&fs_info->nr_async_submits, 0);
> >  	atomic_set(&fs_info->async_delalloc_pages, 0);
> >  	atomic_set(&fs_info->async_submit_draining, 0);
> > -	atomic_set(&fs_info->nr_async_bios, 0);
> >  	atomic_set(&fs_info->defrag_running, 0);
> >  	atomic_set(&fs_info->qgroup_op_seq, 0);
> >  	atomic_set(&fs_info->reada_works_cnt, 0);
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index bd679bc..6e9df4d 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -450,13 +450,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
> >  		pending = pending->bi_next;
> >  		cur->bi_next = NULL;
> >  
> > -		/*
> > -		 * atomic_dec_return implies a barrier for waitqueue_active
> > -		 */
> > -		if (atomic_dec_return(&fs_info->nr_async_bios) < limit &&
> 
> And after that the variable 'limit' becomes unused, please remove it as
> well.
>

OK, thanks for the comments.

thanks,

-liubo
> > -		    waitqueue_active(&fs_info->async_submit_wait))
> > -			wake_up(&fs_info->async_submit_wait);
> > -
> >  		BUG_ON(atomic_read(&cur->__bi_cnt) == 0);
> >  
> >  		/*
> > @@ -6132,13 +6125,6 @@ static noinline void btrfs_schedule_bio(struct btrfs_device *device,
> >  		return;
> >  	}
> >  
> > -	/*
> > -	 * nr_async_bios allows us to reliably return congestion to the
> > -	 * higher layers.  Otherwise, the async bio makes it appear we have
> > -	 * made progress against dirty pages when we've really just put it
> > -	 * on a queue for later
> > -	 */
> > -	atomic_inc(&fs_info->nr_async_bios);
> >  	WARN_ON(bio->bi_next);
> >  	bio->bi_next = NULL;
> >  
> > -- 
> > 2.9.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Btrfs: remove nr_async_submits and async_submit_draining

[Liu Bo]

On Wed, Oct 11, 2017 at 07:20:32PM +0200, David Sterba wrote:
> On Wed, Sep 27, 2017 at 02:31:17PM +0200, David Sterba wrote:
> > On Thu, Sep 07, 2017 at 11:22:22AM -0600, Liu Bo wrote:
> > > Now that we have the combo of flushing twice, which can make sure IO
> > > have started since the second flush will wait for page lock which
> > > won't be unlocked unless setting page writeback and queuing ordered
> > > extents, we don't need %async_submit_draining, %async_delalloc_pages
> > > and %nr_async_submits to tell whether IO have actually started.
> > > 
> > > Moreover, all the flushers in use are followed by functions that wait
> > > for ordered extents to complete, so %nr_async_submits, which tracks
> > > whether bio's async submit has made progress, doesn't really make
> > > sense.
> > > 
> > > However, %async_delalloc_pages is still required by shrink_delalloc()
> > > as that function doesn't flush twice in the normal case (just issues a
> > > writeback with WB_REASON_FS_FREE_SPACE).
> > > 
> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > 
> > Patches like this are almost impossible to review just from the code.
> > This has runtime implications that we'd need to observe in real on
> > various workloads.
> > 
> > So, the patches "look good", but I did not try to forsee all the
> > scenarios where threads interact through the counters. I think it should
> > be safe to add them to for-next and give enough time to test them.
> > 
> > The performance has varied wildly in the last cycle(s) so it's hard to
> > get a baseline, other than another major release. I do expect changes in
> > performance characteristics but still hope it'll be the same on average.
> 
> Testing appears normal, we get more weirdnes just by enabling the
> writeback throttling, but without that I haven't observed anything
> unusual. Patches go to the misc-next part, meaning I don't expect
> changes other than fixups, as separate patches. Thanks.

Thank you Dave, I'm interesting in that weirdness part, will look into
it.

Thanks,

-liubo

Re: [PATCH 3/3] Btrfs: remove nr_async_submits and async_submit_draining

[David Sterba]

On Wed, Sep 27, 2017 at 02:31:17PM +0200, David Sterba wrote:
> On Thu, Sep 07, 2017 at 11:22:22AM -0600, Liu Bo wrote:
> > Now that we have the combo of flushing twice, which can make sure IO
> > have started since the second flush will wait for page lock which
> > won't be unlocked unless setting page writeback and queuing ordered
> > extents, we don't need %async_submit_draining, %async_delalloc_pages
> > and %nr_async_submits to tell whether IO have actually started.
> > 
> > Moreover, all the flushers in use are followed by functions that wait
> > for ordered extents to complete, so %nr_async_submits, which tracks
> > whether bio's async submit has made progress, doesn't really make
> > sense.
> > 
> > However, %async_delalloc_pages is still required by shrink_delalloc()
> > as that function doesn't flush twice in the normal case (just issues a
> > writeback with WB_REASON_FS_FREE_SPACE).
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> 
> Patches like this are almost impossible to review just from the code.
> This has runtime implications that we'd need to observe in real on
> various workloads.
> 
> So, the patches "look good", but I did not try to forsee all the
> scenarios where threads interact through the counters. I think it should
> be safe to add them to for-next and give enough time to test them.
> 
> The performance has varied wildly in the last cycle(s) so it's hard to
> get a baseline, other than another major release. I do expect changes in
> performance characteristics but still hope it'll be the same on average.

Testing appears normal, we get more weirdnes just by enabling the
writeback throttling, but without that I haven't observed anything
unusual. Patches go to the misc-next part, meaning I don't expect
changes other than fixups, as separate patches. Thanks.

Re: [PATCH 3/3] Btrfs: remove nr_async_submits and async_submit_draining

[David Sterba]

On Wed, Oct 11, 2017 at 10:49:14AM -0600, Liu Bo wrote:
> On Wed, Oct 11, 2017 at 07:20:32PM +0200, David Sterba wrote:
> > On Wed, Sep 27, 2017 at 02:31:17PM +0200, David Sterba wrote:
> > > On Thu, Sep 07, 2017 at 11:22:22AM -0600, Liu Bo wrote:
> > > > Now that we have the combo of flushing twice, which can make sure IO
> > > > have started since the second flush will wait for page lock which
> > > > won't be unlocked unless setting page writeback and queuing ordered
> > > > extents, we don't need %async_submit_draining, %async_delalloc_pages
> > > > and %nr_async_submits to tell whether IO have actually started.
> > > > 
> > > > Moreover, all the flushers in use are followed by functions that wait
> > > > for ordered extents to complete, so %nr_async_submits, which tracks
> > > > whether bio's async submit has made progress, doesn't really make
> > > > sense.
> > > > 
> > > > However, %async_delalloc_pages is still required by shrink_delalloc()
> > > > as that function doesn't flush twice in the normal case (just issues a
> > > > writeback with WB_REASON_FS_FREE_SPACE).
> > > > 
> > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > > 
> > > Patches like this are almost impossible to review just from the code.
> > > This has runtime implications that we'd need to observe in real on
> > > various workloads.
> > > 
> > > So, the patches "look good", but I did not try to forsee all the
> > > scenarios where threads interact through the counters. I think it should
> > > be safe to add them to for-next and give enough time to test them.
> > > 
> > > The performance has varied wildly in the last cycle(s) so it's hard to
> > > get a baseline, other than another major release. I do expect changes in
> > > performance characteristics but still hope it'll be the same on average.
> > 
> > Testing appears normal, we get more weirdnes just by enabling the
> > writeback throttling, but without that I haven't observed anything
> > unusual. Patches go to the misc-next part, meaning I don't expect
> > changes other than fixups, as separate patches. Thanks.
> 
> Thank you Dave, I'm interesting in that weirdness part, will look into
> it.

The symptoms are long stalls, without any IO activity (tens of seconds)
when the superblock is being written, this can be identified in the
kernel stack of the process. This was partially fixed by the REQ_ flags
added to the write requests, but I vaguely remember that even after that
the stalls happen. I don't have more details, sorry.