[PATCH 0/7] Preemptive flushing improvements

[PATCH 0/7] Preemptive flushing improvements

[Josef Bacik]

The following patch series is a set of improvements around our preemptive
flushing infrastructure.  A user reported[1] a problem where the preemptive
flushing was running non-stop on his full file system.  I was not able to
reproduce this behavior, but noticed a few fundamental issues with how we
decided to do preemptive flushing.  The main relevant thing is that we were not
taking into account the global reserve when deciding if we needed to
preemptively flush.  In cases such as the users file system where the majority
of the free metadata space is taken up by the global reserve we could
potentially always want to flush, which is annoying and not what we want.

Furthermore I noticed issues where we would clamp entirely too quickly, and
where we made some poor decisions around delalloc.  None of these are ground
breaking or huge improvements, but offer some better performance in some of the
fsperf test cases.  This is the results of my recent run with the whole
patchset.  You'll notice a "regression" in the 16g buffered write, this is
mostly because of the test, if you look at the results on the nightly
performance tests you'll see this test varies more than others.  Re-testing with
just that test didn't show the same regression after multiple runs, so it's just
noise.  I could have chosen to run all the tests multiple times to get an
average over several runs, but that takes a fair bit of time.  Individual runs
of the test showed no regression and often showed an improvement, so I feel
comfortable calling it noise.  The full results are as follows

dbench60 results
  metric     baseline   current         diff
==================================================
qpathinfo       11.53     13.25    14.90%
throughput     446.23    434.52    -2.62%
flush         2502.92   1682.43   -32.78%
qfileinfo        0.92      1.17    27.29%
ntcreatex     1359.76    519.42   -61.80%
qfsinfo          1.77      3.76   112.64%
close            1.90      1.64   -13.91%å
sfileinfo      209.76     76.43   -63.56%
rename        1110.08    518.40   -53.30%
find            13.84     13.13    -5.15%
unlink        1192.89    521.53   -56.28%
writex        1713.39   1321.39   -22.88%
deltree        280.34    296.33     5.70%
readx            3.16      2.91    -8.10%
mkdir            0.03      0.02   -46.67%
lockx            0.78      0.20   -73.89%
unlockx          0.16      0.12   -23.81%

emptyfiles500k results
     metric         baseline   current         diff
=========================================================
write_io_kbytes       125000     125000    0.00%
read_clat_ns_p99           0          0    0.00%
write_bw_bytes      1.79e+08   1.85e+08    3.04%
read_iops                  0          0    0.00%
write_clat_ns_p50      17728      17280   -2.53%
read_io_kbytes             0          0    0.00%
read_io_bytes              0          0    0.00%
write_clat_ns_p99      72704      68096   -6.34%
read_bw_bytes              0          0    0.00%
elapsed                    1          1    0.00%
write_lat_ns_min           0          0    0.00%
sys_cpu                91.23      89.16   -2.27%
write_lat_ns_max           0          0    0.00%
read_lat_ns_min            0          0    0.00%
write_iops          43763.97   45093.80    3.04%
read_lat_ns_max            0          0    0.00%
read_clat_ns_p50           0          0    0.00%

smallfiles100k results
     metric         baseline   current         diff
=========================================================
write_io_kbytes     2.04e+08   2.04e+08    0.00%
read_clat_ns_p99           0          0    0.00%
write_bw_bytes      1.40e+08   1.40e+08    0.50%
read_iops                  0          0    0.00%
write_clat_ns_p50       6712       6944    3.46%
read_io_kbytes             0          0    0.00%
read_io_bytes              0          0    0.00%
write_clat_ns_p99      16000      16512    3.20%
read_bw_bytes              0          0    0.00%
elapsed              1498.88       1491   -0.53%
write_lat_ns_min     2858.38       2919    2.12%
sys_cpu                 6.22       6.51    4.61%
write_lat_ns_max    1.31e+08   1.27e+08   -2.77%
read_lat_ns_min            0          0    0.00%
write_iops          34081.44   34253.51    0.50%
read_lat_ns_max            0          0    0.00%
read_clat_ns_p50           0          0    0.00%

dio4kbs16threads results
     metric          baseline     current          diff
=============================================================
write_io_kbytes         4360879    5312908    21.83%
read_clat_ns_p99              0          0     0.00%
write_bw_bytes      74302497.38   90667585    22.02%
read_iops                     0          0     0.00%
write_clat_ns_p50        243968     238592    -2.20%
read_io_kbytes                0          0     0.00%
read_io_bytes                 0          0     0.00%
write_clat_ns_p99      21094400   15007744   -28.85%
read_bw_bytes                 0          0     0.00%
elapsed                      61         61     0.00%
write_lat_ns_min       38183.25      37949    -0.61%
sys_cpu                    4.03       4.72    17.11%
write_lat_ns_max       1.68e+09   8.46e+08   -49.55%
read_lat_ns_min               0          0     0.00%
write_iops             18140.26   22135.64    22.02%
read_lat_ns_max               0          0     0.00%
read_clat_ns_p50              0          0     0.00%

randwrite2xram results
     metric          baseline     current          diff
=============================================================
write_io_kbytes        27720434   36563300    31.90%
read_clat_ns_p99              0          0     0.00%
write_bw_bytes      93268100.75   1.16e+08    24.83%
read_iops                     0          0     0.00%
write_clat_ns_p50         13168      16512    25.39%
read_io_kbytes                0          0     0.00%
read_io_bytes                 0          0     0.00%
write_clat_ns_p99         39360     125440   218.70%
read_bw_bytes                 0          0     0.00%
elapsed                  334.25        333    -0.37%
write_lat_ns_min        5436.75       5682     4.51%
sys_cpu                    8.22      12.57    52.96%
write_lat_ns_max       4.05e+10   2.73e+10   -32.65%
read_lat_ns_min               0          0     0.00%
write_iops             22770.53   28425.17    24.83%
read_lat_ns_max               0          0     0.00%
read_clat_ns_p50              0          0     0.00%

untarfirefox results
metric    baseline   current        diff
==============================================
elapsed      47.23     46.82   -0.87%

I'm still waiting on feedback from the user to make sure the patches fix the
reported problem, but they're worthy on their own if they do not resolve the
original reported issue.  Thanks,

Josef

[1] https://bugzilla.kernel.org/show_bug.cgi?id=212185

Josef Bacik (7):
  btrfs: check worker before need_preemptive_reclaim
  btrfs: only clamp the first time we have to start flushing
  btrfs: take into account global rsv in need_preemptive_reclaim
  btrfs: use the global rsv size in the preemptive thresh calculation
  btrfs: don't include the global rsv size in the preemptive used amount
  btrfs: only ignore delalloc if delalloc is much smaller than ordered
  btrfs: handle preemptive delalloc flushing slightly differently

 fs/btrfs/space-info.c | 56 +++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 18 deletions(-)

-- 
2.26.3

[PATCH 1/7] btrfs: check worker before need_preemptive_reclaim

[Josef Bacik]

need_preemptive_reclaim() does some calculations, which aren't heavy,
but if we're already running preemptive reclaim there's no reason to do
them at all, so re-order the checks so that we don't do the calculation
if we're already doing reclaim.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 2dc674b7c3b1..c9a5e003bcfa 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1588,8 +1588,8 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
 		 * the async reclaim as we will panic.
 		 */
 		if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags) &&
-		    need_preemptive_reclaim(fs_info, space_info) &&
-		    !work_busy(&fs_info->preempt_reclaim_work)) {
+		    !work_busy(&fs_info->preempt_reclaim_work) &&
+		    need_preemptive_reclaim(fs_info, space_info)) {
 			trace_btrfs_trigger_flush(fs_info, space_info->flags,
 						  orig_bytes, flush, "preempt");
 			queue_work(system_unbound_wq,
-- 
2.26.3

[PATCH 2/7] btrfs: only clamp the first time we have to start flushing

[Josef Bacik]

We were clamping the threshold for preemptive reclaim any time we added
a ticket to wait on, which if we have a lot of threads means we'd
essentially max out the clamp the first time we start to flush.  Instead
of doing this, simply do it every time we have to start flushing, this
will make us ramp up gradually instead of going to max clamping as soon
as we start needing to do flushing.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index c9a5e003bcfa..33edab17af0d 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1561,6 +1561,15 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
 		    flush == BTRFS_RESERVE_FLUSH_DATA) {
 			list_add_tail(&ticket.list, &space_info->tickets);
 			if (!space_info->flush) {
+				/*
+				 * We were forced to add a reserve ticket, so
+				 * our preemptive flushing is unable to keep
+				 * up.  Clamp down on the threshold for the
+				 * preemptive flushing in order to keep up with
+				 * the workload.
+				 */
+				maybe_clamp_preempt(fs_info, space_info);
+
 				space_info->flush = 1;
 				trace_btrfs_trigger_flush(fs_info,
 							  space_info->flags,
@@ -1572,14 +1581,6 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
 			list_add_tail(&ticket.list,
 				      &space_info->priority_tickets);
 		}
-
-		/*
-		 * We were forced to add a reserve ticket, so our preemptive
-		 * flushing is unable to keep up.  Clamp down on the threshold
-		 * for the preemptive flushing in order to keep up with the
-		 * workload.
-		 */
-		maybe_clamp_preempt(fs_info, space_info);
 	} else if (!ret && space_info->flags & BTRFS_BLOCK_GROUP_METADATA) {
 		used += orig_bytes;
 		/*
-- 
2.26.3

[PATCH 3/7] btrfs: take into account global rsv in need_preemptive_reclaim

[Josef Bacik]

Global rsv can't be used for normal allocations, and for very full file
systems we can decide to try and async flush constantly even though
there's really not a lot of space to reclaim.  Deal with this by
including the global block rsv size in the "total used" calculation.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 33edab17af0d..e341f995a7dd 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -792,12 +792,14 @@ btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
 static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 				    struct btrfs_space_info *space_info)
 {
+	u64 global_rsv_size = fs_info->global_block_rsv.reserved;
 	u64 ordered, delalloc;
 	u64 thresh = div_factor_fine(space_info->total_bytes, 98);
 	u64 used;
 
 	/* If we're just plain full then async reclaim just slows us down. */
-	if ((space_info->bytes_used + space_info->bytes_reserved) >= thresh)
+	if ((space_info->bytes_used + space_info->bytes_reserved +
+	     global_rsv_size) >= thresh)
 		return false;
 
 	/*
-- 
2.26.3

[PATCH 4/7] btrfs: use the global rsv size in the preemptive thresh calculation

[Josef Bacik]

We calculate the amount of "free" space available for normal
reservations by taking the total space and subtracting out the hard used
space, which is readonly, used, and reserved space.  However we weren't
taking into account the global block rsv, which is essentially hard used
space.  Handle this by subtracting it from the available free space, so
that our threshold more closely mirrors reality.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index e341f995a7dd..b0dd9b57d352 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -840,8 +840,10 @@ static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 
 	thresh = calc_available_free_space(fs_info, space_info,
 					   BTRFS_RESERVE_FLUSH_ALL);
-	thresh += (space_info->total_bytes - space_info->bytes_used -
-		   space_info->bytes_reserved - space_info->bytes_readonly);
+	used = space_info->bytes_used + space_info->bytes_reserved +
+		space_info->bytes_readonly + global_rsv_size;
+	if (used < space_info->total_bytes)
+		thresh += space_info->total_bytes - used;
 	thresh >>= space_info->clamp;
 
 	used = space_info->bytes_pinned;
-- 
2.26.3

[PATCH 5/7] btrfs: don't include the global rsv size in the preemptive used amount

[Josef Bacik]

When deciding if we should preemptively flush space, we will add in the
amount of space used by all block rsvs.  However this also includes the
global block rsv, which isn't flushable so shouldn't be accounted for in
this calculation.  If we decide to use ->bytes_may_use in our used
calculation we need to subtract the global rsv size from this amount so
it most closely matches the flushable space.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index b0dd9b57d352..4e3857474cfd 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -871,7 +871,7 @@ static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 		used += fs_info->delayed_refs_rsv.reserved +
 			fs_info->delayed_block_rsv.reserved;
 	else
-		used += space_info->bytes_may_use;
+		used += space_info->bytes_may_use - global_rsv_size;
 
 	return (used >= thresh && !btrfs_fs_closing(fs_info) &&
 		!test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));
-- 
2.26.3

[PATCH 6/7] btrfs: only ignore delalloc if delalloc is much smaller than ordered

[Josef Bacik]

While testing heavy delalloc workloads I noticed that sometimes we'd
just stop preemptively flushing when we had loads of delalloc available
to flush.  This is because we skip preemptive flushing if delalloc <=
ordered.  However if we start with say 4gib of delalloc, and we flush
2gib of that, we'll stop flushing there, when we still have 2gib of
delalloc to flush.

Instead adjust the ordered bytes down by half, this way if 2/3 of our
outstanding delalloc reservations are tied up by ordered extents we
don't bother preemptive flushing, as we're getting close to the state
where we need to wait on ordered extents.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 4e3857474cfd..cf09b23f3448 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -864,8 +864,14 @@ static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 	 * clearly be heavy enough to warrant preemptive flushing.  In the case
 	 * of heavy DIO or ordered reservations, preemptive flushing will just
 	 * waste time and cause us to slow down.
+	 *
+	 * We want to make sure we truly are maxed out on ordered however, so
+	 * cut ordered in half, and if it's still higher than delalloc then we
+	 * can keep flushing.  This is to avoid the case where we start
+	 * flushing, and now delalloc == ordered and we stop preemptively
+	 * flushing when we could still have several gigs of delalloc to flush.
 	 */
-	ordered = percpu_counter_read_positive(&fs_info->ordered_bytes);
+	ordered = percpu_counter_read_positive(&fs_info->ordered_bytes) >> 1;
 	delalloc = percpu_counter_read_positive(&fs_info->delalloc_bytes);
 	if (ordered >= delalloc)
 		used += fs_info->delayed_refs_rsv.reserved +
-- 
2.26.3

[PATCH 7/7] btrfs: handle preemptive delalloc flushing slightly differently

[Josef Bacik]

If we decide to flush delalloc from the preemptive flusher, we really do
not want to wait on ordered extents, as it gains us nothing.  However
there was logic to go ahead and wait on ordered extents if there was
more ordered bytes than delalloc bytes.  We do not want this behavior,
so pass through whether this flushing is for preemption, and do not wait
for ordered extents if that's the case.  Also break out of the shrink
loop after the first flushing, as we just want to one shot shrink
delalloc.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index cf09b23f3448..b2d834b92811 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -495,7 +495,8 @@ static inline u64 calc_reclaim_items_nr(struct btrfs_fs_info *fs_info,
  */
 static void shrink_delalloc(struct btrfs_fs_info *fs_info,
 			    struct btrfs_space_info *space_info,
-			    u64 to_reclaim, bool wait_ordered)
+			    u64 to_reclaim, bool wait_ordered,
+			    bool for_preempt)
 {
 	struct btrfs_trans_handle *trans;
 	u64 delalloc_bytes;
@@ -532,7 +533,7 @@ static void shrink_delalloc(struct btrfs_fs_info *fs_info,
 	 * ordered extents, otherwise we'll waste time trying to flush delalloc
 	 * that likely won't give us the space back we need.
 	 */
-	if (ordered_bytes > delalloc_bytes)
+	if (ordered_bytes > delalloc_bytes && !for_preempt)
 		wait_ordered = true;
 
 	loops = 0;
@@ -551,6 +552,14 @@ static void shrink_delalloc(struct btrfs_fs_info *fs_info,
 				break;
 		}
 
+		/*
+		 * If we are for preemption we just want a one-shot of delalloc
+		 * flushing so we can stop flushing if we decide we don't need
+		 * to anymore.
+		 */
+		if (for_preempt)
+			break;
+
 		spin_lock(&space_info->lock);
 		if (list_empty(&space_info->tickets) &&
 		    list_empty(&space_info->priority_tickets)) {
@@ -702,7 +711,7 @@ static void flush_space(struct btrfs_fs_info *fs_info,
 	case FLUSH_DELALLOC:
 	case FLUSH_DELALLOC_WAIT:
 		shrink_delalloc(fs_info, space_info, num_bytes,
-				state == FLUSH_DELALLOC_WAIT);
+				state == FLUSH_DELALLOC_WAIT, for_preempt);
 		break;
 	case FLUSH_DELAYED_REFS_NR:
 	case FLUSH_DELAYED_REFS:
-- 
2.26.3

Re: [PATCH 1/7] btrfs: check worker before need_preemptive_reclaim

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/7] btrfs: take into account global rsv in need_preemptive_reclaim

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/7] btrfs: use the global rsv size in the preemptive thresh calculation

[Johannes Thumshirn]

On 28/04/2021 19:40, Josef Bacik wrote:
> -	thresh += (space_info->total_bytes - space_info->bytes_used -
> -		   space_info->bytes_reserved - space_info->bytes_readonly);
> +	used = space_info->bytes_used + space_info->bytes_reserved +
> +		space_info->bytes_readonly + global_rsv_size;
> +	if (used < space_info->total_bytes)
> +		thresh += space_info->total_bytes - used;
>  	thresh >>= space_info->clamp;


I don't quite understand why you introduced the 'if' here. Now we're only
adding the new free space to the threshold if we're using less than our
total free space, which kinda makes sense that we're not going over our
total free space.

Re: [PATCH 5/7] btrfs: don't include the global rsv size in the preemptive used amount

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/7] Preemptive flushing improvements

[Josef Bacik]

On 4/28/21 9:48 PM, Wang Yugui wrote:
> Hi,
> 
> xfstests generic/562 failed with this patch.
> 
> This is the file 562.out.bad.
> 

It's not failing for me, what is your config like?  Both the local.config and 
the type/size of your devices.  Thanks,

Josef

Re: [PATCH 4/7] btrfs: use the global rsv size in the preemptive thresh calculation

[Josef Bacik]

On 4/29/21 10:04 AM, Johannes Thumshirn wrote:
> On 28/04/2021 19:40, Josef Bacik wrote:
>> -	thresh += (space_info->total_bytes - space_info->bytes_used -
>> -		   space_info->bytes_reserved - space_info->bytes_readonly);
>> +	used = space_info->bytes_used + space_info->bytes_reserved +
>> +		space_info->bytes_readonly + global_rsv_size;
>> +	if (used < space_info->total_bytes)
>> +		thresh += space_info->total_bytes - used;
>>   	thresh >>= space_info->clamp;
> 
> 
> I don't quite understand why you introduced the 'if' here. Now we're only
> adding the new free space to the threshold if we're using less than our
> total free space, which kinda makes sense that we're not going over our
> total free space.
> 

Because it's possible that the global_rsv_size + used is > total_bytes, because 
sometimes the global reserve can end up being calculated as larger than the 
available size (think stupid small fs'es where we only have the original 8mib 
chunk of metadata).  It doesn't usually happen, but that sort of thinking gets 
me into trouble, so this is safer.  Thanks,

Josef

Re: [PATCH 3/7] btrfs: take into account global rsv in need_preemptive_reclaim

[David Sterba]

On Wed, Apr 28, 2021 at 01:38:44PM -0400, Josef Bacik wrote:
> Global rsv can't be used for normal allocations, and for very full file
> systems we can decide to try and async flush constantly even though
> there's really not a lot of space to reclaim.  Deal with this by
> including the global block rsv size in the "total used" calculation.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/space-info.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 33edab17af0d..e341f995a7dd 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -792,12 +792,14 @@ btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
>  static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
>  				    struct btrfs_space_info *space_info)
>  {
> +	u64 global_rsv_size = fs_info->global_block_rsv.reserved;
>  	u64 ordered, delalloc;
>  	u64 thresh = div_factor_fine(space_info->total_bytes, 98);
>  	u64 used;
>  
>  	/* If we're just plain full then async reclaim just slows us down. */
> -	if ((space_info->bytes_used + space_info->bytes_reserved) >= thresh)
> +	if ((space_info->bytes_used + space_info->bytes_reserved +
> +	     global_rsv_size) >= thresh)

That's an extra variable for one time use. Do you have plans to use it
in further patches? Regarding readability, it has to be put on a
separate line anyway so there's space for the whole identifier bit if
you intend to use it then it's ok to keep it.

Re: [PATCH 3/7] btrfs: take into account global rsv in need_preemptive_reclaim

[David Sterba]

On Thu, Apr 29, 2021 at 10:03:10PM +0200, David Sterba wrote:
> On Wed, Apr 28, 2021 at 01:38:44PM -0400, Josef Bacik wrote:
> > Global rsv can't be used for normal allocations, and for very full file
> > systems we can decide to try and async flush constantly even though
> > there's really not a lot of space to reclaim.  Deal with this by
> > including the global block rsv size in the "total used" calculation.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/space-info.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> > index 33edab17af0d..e341f995a7dd 100644
> > --- a/fs/btrfs/space-info.c
> > +++ b/fs/btrfs/space-info.c
> > @@ -792,12 +792,14 @@ btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
> >  static bool need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
> >  				    struct btrfs_space_info *space_info)
> >  {
> > +	u64 global_rsv_size = fs_info->global_block_rsv.reserved;
> >  	u64 ordered, delalloc;
> >  	u64 thresh = div_factor_fine(space_info->total_bytes, 98);
> >  	u64 used;
> >  
> >  	/* If we're just plain full then async reclaim just slows us down. */
> > -	if ((space_info->bytes_used + space_info->bytes_reserved) >= thresh)
> > +	if ((space_info->bytes_used + space_info->bytes_reserved +
> > +	     global_rsv_size) >= thresh)
> 
> That's an extra variable for one time use. Do you have plans to use it
> in further patches? Regarding readability, it has to be put on a
> separate line anyway so there's space for the whole identifier bit if
> you intend to use it then it's ok to keep it.

Never mind, I got fooled by the diff -p that shows the name of the
previous function (btrfs_calc_reclaim_metadata_size) and not the actual
function where it is being added (need_preemptive_reclaim) in case it's
in the hunk context.

Re: [PATCH 0/7] Preemptive flushing improvements

[Wang Yugui]

Hi,

> On 4/28/21 9:48 PM, Wang Yugui wrote:
> > Hi,
> >
> > xfstests generic/562 failed with this patch.
> >
> > This is the file 562.out.bad.
> > 
> It's not failing for me, what is your config like?  Both the local.config and the type/size of your devices.  Thanks,
> 

# cat xfstests/local.config

dev_base_disk=/dev/disk/by-id/nvme-Dell_Express_Flash_NVMe_SM1715_800GB_SFF__S29HNYAH500207

export TEST_DIR=/mnt/test

export TEST_DEV=${dev_base_disk}-part6
export TEST_DEV=$(realpath $TEST_DEV)


export SCRATCH_MNT=/mnt/scratch

export SCRATCH_DEV_POOL="${dev_base_disk}-part1 ${dev_base_disk}-part2
${dev_base_disk}-part3 ${dev_base_disk}-part4 ${dev_base_disk}-part5"
export SCRATCH_DEV_POOL=$(realpath $SCRATCH_DEV_POOL | xargs /bin/echo )

#LOGWRITES_DEV=


# parted /dev/disk/by-id/nvme-Dell_Express_Flash_NVMe_SM1715_800GB_SFF__S29HNYAH500207 unit s print
Model: NVMe Device (nvme)
Disk /dev/nvme0n1: 1562824368s
Sector size (logical/physical): 512B/512B
Partition Table: gpt
Disk Flags:

Number  Start       End         Size       File system  Name     Flags
 1      2097152s    54525951s   52428800s  btrfs        primary
 2      54525952s   106954751s  52428800s               primary
 3      106954752s  159383551s  52428800s               primary
 4      159383552s  211812351s  52428800s               primary
 5      211812352s  264241151s  52428800s               primary
 6      264241152s  316669951s  52428800s  btrfs        primary

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/04/30

Re: [PATCH 0/7] Preemptive flushing improvements

[Wang Yugui]

Hi,

> > On 4/28/21 9:48 PM, Wang Yugui wrote:
> > > Hi,
> > >
> > > xfstests generic/562 failed with this patch.
> > >
> > > This is the file 562.out.bad.
> > > 
> > It's not failing for me, what is your config like?  Both the local.config and the type/size of your devices.  Thanks,
> > 
> 
> # cat xfstests/local.config
> 
> dev_base_disk=/dev/disk/by-id/nvme-Dell_Express_Flash_NVMe_SM1715_800GB_SFF__S29HNYAH500207
> 
> export TEST_DIR=/mnt/test
> 
> export TEST_DEV=${dev_base_disk}-part6
> export TEST_DEV=$(realpath $TEST_DEV)
> 
> 
> export SCRATCH_MNT=/mnt/scratch
> 
> export SCRATCH_DEV_POOL="${dev_base_disk}-part1 ${dev_base_disk}-part2
> ${dev_base_disk}-part3 ${dev_base_disk}-part4 ${dev_base_disk}-part5"
> export SCRATCH_DEV_POOL=$(realpath $SCRATCH_DEV_POOL | xargs /bin/echo )
> 
> #LOGWRITES_DEV=
> 
> 
> # parted /dev/disk/by-id/nvme-Dell_Express_Flash_NVMe_SM1715_800GB_SFF__S29HNYAH500207 unit s print
> Model: NVMe Device (nvme)
> Disk /dev/nvme0n1: 1562824368s
> Sector size (logical/physical): 512B/512B
> Partition Table: gpt
> Disk Flags:
> 
> Number  Start       End         Size       File system  Name     Flags
>  1      2097152s    54525951s   52428800s  btrfs        primary
>  2      54525952s   106954751s  52428800s               primary
>  3      106954752s  159383551s  52428800s               primary
>  4      159383552s  211812351s  52428800s               primary
>  5      211812352s  264241151s  52428800s               primary
>  6      264241152s  316669951s  52428800s  btrfs        primary


By the way, We enable '-O no-holes -R free-space-tree' default for
mkfs.btrfs.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/04/30

Re: [PATCH 0/7] Preemptive flushing improvements

[David Sterba]

On Fri, Apr 30, 2021 at 06:23:08AM +0800, Wang Yugui wrote:
> > > > This is the file 562.out.bad.

Can you please post again the error?

> By the way, We enable '-O no-holes -R free-space-tree' default for
> mkfs.btrfs.

The fstests cases are not robust enough to catch differences in the
output when various features are enabled so it depends on the exact
error.

Re: [PATCH 0/7] Preemptive flushing improvements

[Wang Yugui]

Hi,

> On Fri, Apr 30, 2021 at 06:23:08AM +0800, Wang Yugui wrote:
> > > > > This is the file 562.out.bad.
> 
> Can you please post again the error?

This is the head part of the file of 562.out.bad(whole file is 2.55M).

QA output created by 562
XFS_IOC_CLONE: No space left on device
File foo data after cloning and remount:
0000000 c7 c7 c7 c7 c7 c7 c7 c7 c7 c7 c7 c7 c7 c7 c7 c7
*
25509888 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
*
25513984 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5
*
25518080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
*
25522176 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5 e5
<repeat of 00 00.../e5 e5...>

> > By the way, We enable '-O no-holes -R free-space-tree' default for
> > mkfs.btrfs.
> 
> The fstests cases are not robust enough to catch differences in the
> output when various features are enabled so it depends on the exact
> error.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/04/30

Re: [PATCH 0/7] Preemptive flushing improvements

[David Sterba]

On Wed, Apr 28, 2021 at 01:38:41PM -0400, Josef Bacik wrote:
> The following patch series is a set of improvements around our preemptive
> flushing infrastructure.  A user reported[1] a problem where the preemptive
> flushing was running non-stop on his full file system.  I was not able to
> reproduce this behavior, but noticed a few fundamental issues with how we
> decided to do preemptive flushing.  The main relevant thing is that we were not
> taking into account the global reserve when deciding if we needed to
> preemptively flush.  In cases such as the users file system where the majority
> of the free metadata space is taken up by the global reserve we could
> potentially always want to flush, which is annoying and not what we want.
> 
> Furthermore I noticed issues where we would clamp entirely too quickly, and
> where we made some poor decisions around delalloc.  None of these are ground
> breaking or huge improvements, but offer some better performance in some of the
> fsperf test cases.  This is the results of my recent run with the whole
> patchset.  You'll notice a "regression" in the 16g buffered write, this is
> mostly because of the test, if you look at the results on the nightly
> performance tests you'll see this test varies more than others.  Re-testing with
> just that test didn't show the same regression after multiple runs, so it's just
> noise.  I could have chosen to run all the tests multiple times to get an
> average over several runs, but that takes a fair bit of time.  Individual runs
> of the test showed no regression and often showed an improvement, so I feel
> comfortable calling it noise.  The full results are as follows
> 
> dbench60 results
>   metric     baseline   current         diff
> ==================================================
> qpathinfo       11.53     13.25    14.90%
> throughput     446.23    434.52    -2.62%
> flush         2502.92   1682.43   -32.78%
> qfileinfo        0.92      1.17    27.29%
> ntcreatex     1359.76    519.42   -61.80%
> qfsinfo          1.77      3.76   112.64%
> close            1.90      1.64   -13.91%å
> sfileinfo      209.76     76.43   -63.56%
> rename        1110.08    518.40   -53.30%
> find            13.84     13.13    -5.15%
> unlink        1192.89    521.53   -56.28%
> writex        1713.39   1321.39   -22.88%
> deltree        280.34    296.33     5.70%
> readx            3.16      2.91    -8.10%
> mkdir            0.03      0.02   -46.67%
> lockx            0.78      0.20   -73.89%
> unlockx          0.16      0.12   -23.81%
> 
> emptyfiles500k results
>      metric         baseline   current         diff
> =========================================================
> write_io_kbytes       125000     125000    0.00%
> read_clat_ns_p99           0          0    0.00%
> write_bw_bytes      1.79e+08   1.85e+08    3.04%
> read_iops                  0          0    0.00%
> write_clat_ns_p50      17728      17280   -2.53%
> read_io_kbytes             0          0    0.00%
> read_io_bytes              0          0    0.00%
> write_clat_ns_p99      72704      68096   -6.34%
> read_bw_bytes              0          0    0.00%
> elapsed                    1          1    0.00%
> write_lat_ns_min           0          0    0.00%
> sys_cpu                91.23      89.16   -2.27%
> write_lat_ns_max           0          0    0.00%
> read_lat_ns_min            0          0    0.00%
> write_iops          43763.97   45093.80    3.04%
> read_lat_ns_max            0          0    0.00%
> read_clat_ns_p50           0          0    0.00%
> 
> smallfiles100k results
>      metric         baseline   current         diff
> =========================================================
> write_io_kbytes     2.04e+08   2.04e+08    0.00%
> read_clat_ns_p99           0          0    0.00%
> write_bw_bytes      1.40e+08   1.40e+08    0.50%
> read_iops                  0          0    0.00%
> write_clat_ns_p50       6712       6944    3.46%
> read_io_kbytes             0          0    0.00%
> read_io_bytes              0          0    0.00%
> write_clat_ns_p99      16000      16512    3.20%
> read_bw_bytes              0          0    0.00%
> elapsed              1498.88       1491   -0.53%
> write_lat_ns_min     2858.38       2919    2.12%
> sys_cpu                 6.22       6.51    4.61%
> write_lat_ns_max    1.31e+08   1.27e+08   -2.77%
> read_lat_ns_min            0          0    0.00%
> write_iops          34081.44   34253.51    0.50%
> read_lat_ns_max            0          0    0.00%
> read_clat_ns_p50           0          0    0.00%
> 
> dio4kbs16threads results
>      metric          baseline     current          diff
> =============================================================
> write_io_kbytes         4360879    5312908    21.83%
> read_clat_ns_p99              0          0     0.00%
> write_bw_bytes      74302497.38   90667585    22.02%
> read_iops                     0          0     0.00%
> write_clat_ns_p50        243968     238592    -2.20%
> read_io_kbytes                0          0     0.00%
> read_io_bytes                 0          0     0.00%
> write_clat_ns_p99      21094400   15007744   -28.85%
> read_bw_bytes                 0          0     0.00%
> elapsed                      61         61     0.00%
> write_lat_ns_min       38183.25      37949    -0.61%
> sys_cpu                    4.03       4.72    17.11%
> write_lat_ns_max       1.68e+09   8.46e+08   -49.55%
> read_lat_ns_min               0          0     0.00%
> write_iops             18140.26   22135.64    22.02%
> read_lat_ns_max               0          0     0.00%
> read_clat_ns_p50              0          0     0.00%
> 
> randwrite2xram results
>      metric          baseline     current          diff
> =============================================================
> write_io_kbytes        27720434   36563300    31.90%
> read_clat_ns_p99              0          0     0.00%
> write_bw_bytes      93268100.75   1.16e+08    24.83%
> read_iops                     0          0     0.00%
> write_clat_ns_p50         13168      16512    25.39%
> read_io_kbytes                0          0     0.00%
> read_io_bytes                 0          0     0.00%
> write_clat_ns_p99         39360     125440   218.70%
> read_bw_bytes                 0          0     0.00%
> elapsed                  334.25        333    -0.37%
> write_lat_ns_min        5436.75       5682     4.51%
> sys_cpu                    8.22      12.57    52.96%
> write_lat_ns_max       4.05e+10   2.73e+10   -32.65%
> read_lat_ns_min               0          0     0.00%
> write_iops             22770.53   28425.17    24.83%
> read_lat_ns_max               0          0     0.00%
> read_clat_ns_p50              0          0     0.00%
> 
> untarfirefox results
> metric    baseline   current        diff
> ==============================================
> elapsed      47.23     46.82   -0.87%
> 
> I'm still waiting on feedback from the user to make sure the patches fix the
> reported problem, but they're worthy on their own if they do not resolve the
> original reported issue.  Thanks,
> 
> Josef
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=212185
> 
> Josef Bacik (7):
>   btrfs: check worker before need_preemptive_reclaim
>   btrfs: only clamp the first time we have to start flushing
>   btrfs: take into account global rsv in need_preemptive_reclaim
>   btrfs: use the global rsv size in the preemptive thresh calculation
>   btrfs: don't include the global rsv size in the preemptive used amount
>   btrfs: only ignore delalloc if delalloc is much smaller than ordered
>   btrfs: handle preemptive delalloc flushing slightly differently

It would be good to summarize the noticeable improvements over all the
tests and put it at least into the last patch assuming that's where it
was produced.

The tests passed for me here so I'll add the patches to misc-next for
more coverage.