Message ID | 20170313203204.11857-3-enadolski@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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? -- 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
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 > > -- 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
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. -- 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
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) {