* [PATCH 0/2] Cleanup for some hardcoded constants
@ 2017-03-13 20:32 ednadolski
2017-03-13 20:32 ` [PATCH 1/2] btrfs: provide enumeration for __merge_refs mode argument ednadolski
2017-03-13 20:32 ` [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro ednadolski
0 siblings, 2 replies; 7+ messages in thread
From: ednadolski @ 2017-03-13 20:32 UTC (permalink / raw)
To: linux-btrfs; +Cc: Edmund Nadolski
From: Edmund Nadolski <enadolski@suse.com>
This series replaces several hard-coded values with descriptive
symbols.
Edmund Nadolski (2):
btrfs: provide enumeration for __merge_refs mode argument
btrfs: replace hardcoded value with SEQ_NONE macro
fs/btrfs/backref.c | 31 +++++++++++++++++--------------
fs/btrfs/backref.h | 2 ++
fs/btrfs/qgroup.c | 4 ++--
3 files changed, 21 insertions(+), 16 deletions(-)
--
2.10.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] btrfs: provide enumeration for __merge_refs mode argument
2017-03-13 20:32 [PATCH 0/2] Cleanup for some hardcoded constants ednadolski
@ 2017-03-13 20:32 ` ednadolski
2017-03-15 14:47 ` David Sterba
2017-03-13 20:32 ` [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro ednadolski
1 sibling, 1 reply; 7+ messages in thread
From: ednadolski @ 2017-03-13 20:32 UTC (permalink / raw)
To: linux-btrfs; +Cc: Edmund Nadolski
From: Edmund Nadolski <enadolski@suse.com>
Replace hardcoded numeric values for __merge_refs 'mode' argument
with descriptive constants.
Signed-off-by: Edmund Nadolski <enadolski@suse.com>
Reviewed-by: Jeff Mahoney <jeffm@suse.com>
---
fs/btrfs/backref.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 7699e16..e794b6e 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -26,6 +26,11 @@
#include "delayed-ref.h"
#include "locking.h"
+enum merge_mode {
+ MERGE_IDENTICAL_KEYS = 1,
+ MERGE_IDENTICAL_PARENTS,
+};
+
/* Just an arbitrary number so we can be sure this happened */
#define BACKREF_FOUND_SHARED 6
@@ -809,14 +814,12 @@ static int __add_missing_keys(struct btrfs_fs_info *fs_info,
/*
* merge backrefs and adjust counts accordingly
*
- * mode = 1: merge identical keys, if key is set
* FIXME: if we add more keys in __add_prelim_ref, we can merge more here.
* additionally, we could even add a key range for the blocks we
* looked into to merge even more (-> replace unresolved refs by those
* having a parent).
- * mode = 2: merge identical parents
*/
-static void __merge_refs(struct list_head *head, int mode)
+static void __merge_refs(struct list_head *head, enum merge_mode mode)
{
struct __prelim_ref *pos1;
@@ -829,7 +832,7 @@ static void __merge_refs(struct list_head *head, int mode)
if (!ref_for_same_block(ref1, ref2))
continue;
- if (mode == 1) {
+ if (mode == MERGE_IDENTICAL_KEYS) {
if (!ref1->parent && ref2->parent)
swap(ref1, ref2);
} else {
@@ -1374,7 +1377,7 @@ static int find_parent_nodes(struct btrfs_trans_handle *trans,
if (ret)
goto out;
- __merge_refs(&prefs, 1);
+ __merge_refs(&prefs, MERGE_IDENTICAL_KEYS);
ret = __resolve_indirect_refs(fs_info, path, time_seq, &prefs,
extent_item_pos, total_refs,
@@ -1382,7 +1385,7 @@ static int find_parent_nodes(struct btrfs_trans_handle *trans,
if (ret)
goto out;
- __merge_refs(&prefs, 2);
+ __merge_refs(&prefs, MERGE_IDENTICAL_PARENTS);
while (!list_empty(&prefs)) {
ref = list_first_entry(&prefs, struct __prelim_ref, list);
--
2.10.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro
2017-03-13 20:32 [PATCH 0/2] Cleanup for some hardcoded constants ednadolski
2017-03-13 20:32 ` [PATCH 1/2] btrfs: provide enumeration for __merge_refs mode argument ednadolski
@ 2017-03-13 20:32 ` ednadolski
2017-03-15 14:38 ` David Sterba
1 sibling, 1 reply; 7+ messages in thread
From: ednadolski @ 2017-03-13 20:32 UTC (permalink / raw)
To: linux-btrfs; +Cc: Edmund Nadolski
From: Edmund Nadolski <enadolski@suse.com>
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 <enadolski@suse.com>
Reviewed-by: Jeff Mahoney <jeffm@suse.com>
---
fs/btrfs/backref.c | 16 ++++++++--------
fs/btrfs/backref.h | 2 ++
fs/btrfs/qgroup.c | 4 ++--
3 files changed, 12 insertions(+), 10 deletions(-)
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) {
--
2.10.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro
2017-03-13 20:32 ` [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro ednadolski
@ 2017-03-15 14:38 ` David Sterba
2017-03-16 1:59 ` Qu Wenruo
0 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2017-03-15 14:38 UTC (permalink / raw)
To: ednadolski; +Cc: linux-btrfs, Edmund Nadolski
On Mon, Mar 13, 2017 at 02:32:04PM -0600, ednadolski@gmail.com wrote:
> From: Edmund Nadolski <enadolski@suse.com>
>
> 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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs: provide enumeration for __merge_refs mode argument
2017-03-13 20:32 ` [PATCH 1/2] btrfs: provide enumeration for __merge_refs mode argument ednadolski
@ 2017-03-15 14:47 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2017-03-15 14:47 UTC (permalink / raw)
To: ednadolski; +Cc: linux-btrfs, Edmund Nadolski
On Mon, Mar 13, 2017 at 02:32:03PM -0600, ednadolski@gmail.com wrote:
> @@ -809,14 +814,12 @@ static int __add_missing_keys(struct btrfs_fs_info *fs_info,
> /*
> * merge backrefs and adjust counts accordingly
> *
> - * mode = 1: merge identical keys, if key is set
> * FIXME: if we add more keys in __add_prelim_ref, we can merge more here.
> * additionally, we could even add a key range for the blocks we
> * looked into to merge even more (-> replace unresolved refs by those
> * having a parent).
The 'FIXME' seems to refer to mode = 1, but now that you remove it, it's
not clear what it's referring to. Mentioning MERGE_IDENTICAL_KEYS would
be good.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro
2017-03-15 14:38 ` David Sterba
@ 2017-03-16 1:59 ` Qu Wenruo
2017-03-27 18:01 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2017-03-16 1:59 UTC (permalink / raw)
To: dsterba, ednadolski, linux-btrfs, Edmund Nadolski
At 03/15/2017 10:38 PM, David Sterba wrote:
> On Mon, Mar 13, 2017 at 02:32:04PM -0600, ednadolski@gmail.com wrote:
>> From: Edmund Nadolski <enadolski@suse.com>
>>
>> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro
2017-03-16 1:59 ` Qu Wenruo
@ 2017-03-27 18:01 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2017-03-27 18:01 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, ednadolski, linux-btrfs, Edmund Nadolski
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, ednadolski@gmail.com wrote:
> >> From: Edmund Nadolski <enadolski@suse.com>
> >>
> >> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-27 18:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-13 20:32 [PATCH 0/2] Cleanup for some hardcoded constants ednadolski
2017-03-13 20:32 ` [PATCH 1/2] btrfs: provide enumeration for __merge_refs mode argument ednadolski
2017-03-15 14:47 ` David Sterba
2017-03-13 20:32 ` [PATCH 2/2] btrfs: replace hardcoded value with SEQ_NONE macro ednadolski
2017-03-15 14:38 ` David Sterba
2017-03-16 1:59 ` Qu Wenruo
2017-03-27 18:01 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.