[2/2] btrfs: replace hardcoded value with SEQ_NONE macro

Commit Message

[e-mail] March 13, 2017, 8:32 p.m. UTC

From: Edmund Nadolski <[e-mail]>

Define the SEQ_NONE macro to replace (u64)-1 in places where said
value triggers a special-case ref search behavior.

Signed-off-by: Edmund Nadolski <[e-mail]>
Reviewed-by: Jeff Mahoney <[e-mail]>
---
 fs/btrfs/backref.c | 16 ++++++++--------
 fs/btrfs/backref.h |  2 ++
 fs/btrfs/qgroup.c  |  4 ++--
 3 files changed, 12 insertions(+), 10 deletions(-)

Comments

David Sterba March 15, 2017, 2:38 p.m. UTC | #1

On Mon, Mar 13, 2017 at 02:32:04PM -0600, [e-mail] wrote:
> From: Edmund Nadolski <[e-mail]>
> 
> Define the SEQ_NONE macro to replace (u64)-1 in places where said
> value triggers a special-case ref search behavior.

> index 9c41fba..20915a6 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -23,6 +23,8 @@
>  #include "ulist.h"
>  #include "extent_io.h"
>  
> +#define SEQ_NONE	((u64)-1)

Can you please move the definition to ctree.h, near line 660, where
seq_list and SEQ_LIST_INIT are defined, so thay're all grouped together?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Qu Wenruo March 16, 2017, 1:59 a.m. UTC | #2

At 03/15/2017 10:38 PM, David Sterba wrote:
> On Mon, Mar 13, 2017 at 02:32:04PM -0600, [e-mail] wrote:
>> From: Edmund Nadolski <[e-mail]>
>>
>> Define the SEQ_NONE macro to replace (u64)-1 in places where said
>> value triggers a special-case ref search behavior.
>
>> index 9c41fba..20915a6 100644
>> --- a/fs/btrfs/backref.h
>> +++ b/fs/btrfs/backref.h
>> @@ -23,6 +23,8 @@
>>  #include "ulist.h"
>>  #include "extent_io.h"
>>
>> +#define SEQ_NONE	((u64)-1)

The naming of SEQ_NONE sounds not that good to me.

The (u64)-1 is to to info the backref walker to only search current 
root, and no need to worry about delayed_refs, since the caller (qgroup) 
will ensure that no delayed_ref will exist.

While the name SEQ_NONE seems a little like to 0, which is far from the 
original meaning.

What about SEQ_FINAL or SEQ_LAST?
Since the timing we use (u64)-1 is just before switching commit roots, 
it would be better for the naming to indicate that.

Thanks,
Qu



>
> Can you please move the definition to ctree.h, near line 660, where
> seq_list and SEQ_LIST_INIT are defined, so thay're all grouped together?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba March 27, 2017, 6:01 p.m. UTC | #3

On Thu, Mar 16, 2017 at 09:59:38AM +0800, Qu Wenruo wrote:
> 
> 
> At 03/15/2017 10:38 PM, David Sterba wrote:
> > On Mon, Mar 13, 2017 at 02:32:04PM -0600, [e-mail] wrote:
> >> From: Edmund Nadolski <[e-mail]>
> >>
> >> Define the SEQ_NONE macro to replace (u64)-1 in places where said
> >> value triggers a special-case ref search behavior.
> >
> >> index 9c41fba..20915a6 100644
> >> --- a/fs/btrfs/backref.h
> >> +++ b/fs/btrfs/backref.h
> >> @@ -23,6 +23,8 @@
> >>  #include "ulist.h"
> >>  #include "extent_io.h"
> >>
> >> +#define SEQ_NONE	((u64)-1)
> 
> The naming of SEQ_NONE sounds not that good to me.
> 
> The (u64)-1 is to to info the backref walker to only search current 
> root, and no need to worry about delayed_refs, since the caller (qgroup) 
> will ensure that no delayed_ref will exist.
> 
> While the name SEQ_NONE seems a little like to 0, which is far from the 
> original meaning.
> 
> What about SEQ_FINAL or SEQ_LAST?

SEQ_LAST sounds good to me.

> Since the timing we use (u64)-1 is just before switching commit roots, 
> it would be better for the naming to indicate that.

Yes, makes sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index e794b6e..297de5b 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -538,7 +538,7 @@  static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path,
 	 * slot==nritems. In that case, go to the next leaf before we continue.
 	 */
 	if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
-		if (time_seq == (u64)-1)
+		if (time_seq == SEQ_NONE)
 			ret = btrfs_next_leaf(root, path);
 		else
 			ret = btrfs_next_old_leaf(root, path, time_seq);
@@ -582,7 +582,7 @@  static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path,
 			eie = NULL;
 		}
 next:
-		if (time_seq == (u64)-1)
+		if (time_seq == SEQ_NONE)
 			ret = btrfs_next_item(root, path);
 		else
 			ret = btrfs_next_old_item(root, path, time_seq);
@@ -634,7 +634,7 @@  static int __resolve_indirect_ref(struct btrfs_fs_info *fs_info,
 
 	if (path->search_commit_root)
 		root_level = btrfs_header_level(root->commit_root);
-	else if (time_seq == (u64)-1)
+	else if (time_seq == SEQ_NONE)
 		root_level = btrfs_header_level(root->node);
 	else
 		root_level = btrfs_old_root_level(root, time_seq);
@@ -645,7 +645,7 @@  static int __resolve_indirect_ref(struct btrfs_fs_info *fs_info,
 	}
 
 	path->lowest_level = level;
-	if (time_seq == (u64)-1)
+	if (time_seq == SEQ_NONE)
 		ret = btrfs_search_slot(NULL, root, &ref->key_for_search, path,
 					0, 0);
 	else
@@ -1199,7 +1199,7 @@  static int __add_keyed_refs(struct btrfs_fs_info *fs_info,
  *
  * NOTE: This can return values > 0
  *
- * If time_seq is set to (u64)-1, it will not search delayed_refs, and behave
+ * If time_seq is set to SEQ_NONE, it will not search delayed_refs, and behave
  * much like trans == NULL case, the difference only lies in it will not
  * commit root.
  * The special case is for qgroup to search roots in commit_transaction().
@@ -1246,7 +1246,7 @@  static int find_parent_nodes(struct btrfs_trans_handle *trans,
 		path->skip_locking = 1;
 	}
 
-	if (time_seq == (u64)-1)
+	if (time_seq == SEQ_NONE)
 		path->skip_locking = 1;
 
 	/*
@@ -1276,9 +1276,9 @@  static int find_parent_nodes(struct btrfs_trans_handle *trans,
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 	if (trans && likely(trans->type != __TRANS_DUMMY) &&
-	    time_seq != (u64)-1) {
+	    time_seq != SEQ_NONE) {
 #else
-	if (trans && time_seq != (u64)-1) {
+	if (trans && time_seq != SEQ_NONE) {
 #endif
 		/*
 		 * look if there are updates for this ref queued and lock the
diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 9c41fba..20915a6 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -23,6 +23,8 @@ 
 #include "ulist.h"
 #include "extent_io.h"
 
+#define SEQ_NONE	((u64)-1)
+
 struct inode_fs_paths {
 	struct btrfs_path		*btrfs_path;
 	struct btrfs_root		*fs_root;
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index bb7e42f..76d84af 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2077,12 +2077,12 @@  int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,
 					goto cleanup;
 			}
 			/*
-			 * Use (u64)-1 as time_seq to do special search, which
+			 * Use SEQ_NONE as time_seq to do special search, which
 			 * doesn't lock tree or delayed_refs and search current
 			 * root. It's safe inside commit_transaction().
 			 */
 			ret = btrfs_find_all_roots(trans, fs_info,
-					record->bytenr, (u64)-1, &new_roots);
+					record->bytenr, SEQ_NONE, &new_roots);
 			if (ret < 0)
 				goto cleanup;
 			if (qgroup_to_skip) {