[05/36] btrfs: only count ref heads run in __btrfs_run_delayed_refs
diff mbox series

Message ID 20180911175807.26181-6-josef@toxicpanda.com
State New
Headers show
Series
  • My current patch queue
Related show

Commit Message

Josef Bacik Sept. 11, 2018, 5:57 p.m. UTC
We pick the number of ref's to run based on the number of ref heads, and
only make the decision to stop once we've processed entire ref heads, so
only count the ref heads we've run and bail once we've hit the number of
ref heads we wanted to process.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent-tree.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Omar Sandoval Sept. 11, 2018, 11:07 p.m. UTC | #1
On Tue, Sep 11, 2018 at 01:57:36PM -0400, Josef Bacik wrote:
> We pick the number of ref's to run based on the number of ref heads, and
> only make the decision to stop once we've processed entire ref heads, so
> only count the ref heads we've run and bail once we've hit the number of
> ref heads we wanted to process.

Despite Nikolay's comment, it seems wrong to me to split this patch up
from the previous one. After the first one, you have this nonsensical
middle ground where the counter is number of heads but this counter is
number of refs.

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent-tree.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 98f36dfeccb0..b32bd38390dd 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2592,6 +2592,7 @@ static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
>  				spin_unlock(&delayed_refs->lock);
>  				break;
>  			}
> +			count++;
>  
>  			/* grab the lock that says we are going to process
>  			 * all the refs for this head */
> @@ -2605,7 +2606,6 @@ static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
>  			 */
>  			if (ret == -EAGAIN) {
>  				locked_ref = NULL;
> -				count++;
>  				continue;
>  			}
>  		}
> @@ -2633,7 +2633,6 @@ static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
>  			unselect_delayed_ref_head(delayed_refs, locked_ref);
>  			locked_ref = NULL;
>  			cond_resched();
> -			count++;
>  			continue;
>  		}
>  
> @@ -2651,7 +2650,6 @@ static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
>  				return ret;
>  			}
>  			locked_ref = NULL;
> -			count++;
>  			continue;
>  		}
>  
> @@ -2702,7 +2700,6 @@ static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
>  		}
>  
>  		btrfs_put_delayed_ref(ref);
> -		count++;
>  		cond_resched();
>  	}
>  
> -- 
> 2.14.3
>
David Sterba Sept. 12, 2018, 5:46 p.m. UTC | #2
On Tue, Sep 11, 2018 at 04:07:30PM -0700, Omar Sandoval wrote:
> On Tue, Sep 11, 2018 at 01:57:36PM -0400, Josef Bacik wrote:
> > We pick the number of ref's to run based on the number of ref heads, and
> > only make the decision to stop once we've processed entire ref heads, so
> > only count the ref heads we've run and bail once we've hit the number of
> > ref heads we wanted to process.
> 
> Despite Nikolay's comment, it seems wrong to me to split this patch up
> from the previous one. After the first one, you have this nonsensical
> middle ground where the counter is number of heads but this counter is
> number of refs.

If the changes now split in two are really just one, then this needs to
be explained in the changelog. Nikolay's comment can be read in two
ways, split the unrelated change, or add the reason why.

https://patchwork.kernel.org/patch/10534671/

As v1 od the patch says "We use this number ..." and the patch changes
two variables, I find it ambiguous. Though the last sentence looks like
it refers to the 'count', it's not very clear and I got lost there too.

Patch
diff mbox series

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 98f36dfeccb0..b32bd38390dd 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2592,6 +2592,7 @@  static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 				spin_unlock(&delayed_refs->lock);
 				break;
 			}
+			count++;
 
 			/* grab the lock that says we are going to process
 			 * all the refs for this head */
@@ -2605,7 +2606,6 @@  static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 			 */
 			if (ret == -EAGAIN) {
 				locked_ref = NULL;
-				count++;
 				continue;
 			}
 		}
@@ -2633,7 +2633,6 @@  static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 			unselect_delayed_ref_head(delayed_refs, locked_ref);
 			locked_ref = NULL;
 			cond_resched();
-			count++;
 			continue;
 		}
 
@@ -2651,7 +2650,6 @@  static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 				return ret;
 			}
 			locked_ref = NULL;
-			count++;
 			continue;
 		}
 
@@ -2702,7 +2700,6 @@  static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 		}
 
 		btrfs_put_delayed_ref(ref);
-		count++;
 		cond_resched();
 	}