* [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves @ 2018-04-26 1:27 Liu Bo 2018-04-26 4:57 ` Nikolay Borisov 2018-04-26 18:06 ` David Sterba 0 siblings, 2 replies; 7+ messages in thread From: Liu Bo @ 2018-04-26 1:27 UTC (permalink / raw) To: linux-btrfs path->keep_lock may force to lock tree root and higher nodes, and make lock contention worse, thus it needs to be avoided as much as possible. In btrfs_degrag_leaves, path->keep_lock is set but @path immediatley gets released, which is not necessary at all. Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> --- v2: update commit log with more details. fs/btrfs/tree-defrag.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/btrfs/tree-defrag.c b/fs/btrfs/tree-defrag.c index 3c0987ab587d..c12747904d4c 100644 --- a/fs/btrfs/tree-defrag.c +++ b/fs/btrfs/tree-defrag.c @@ -65,8 +65,6 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, memcpy(&key, &root->defrag_progress, sizeof(key)); } - path->keep_locks = 1; - ret = btrfs_search_forward(root, &key, path, BTRFS_OLDEST_GENERATION); if (ret < 0) goto out; @@ -81,6 +79,7 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, * a deadlock (attempting to write lock an already write locked leaf). */ path->lowest_level = 1; + path->keep_locks = 1; wret = btrfs_search_slot(trans, root, &key, path, 0, 1); if (wret < 0) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves 2018-04-26 1:27 [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves Liu Bo @ 2018-04-26 4:57 ` Nikolay Borisov 2018-04-26 18:06 ` David Sterba 1 sibling, 0 replies; 7+ messages in thread From: Nikolay Borisov @ 2018-04-26 4:57 UTC (permalink / raw) To: Liu Bo, linux-btrfs On 26.04.2018 04:27, Liu Bo wrote: > path->keep_lock may force to lock tree root and higher nodes, and make > lock contention worse, thus it needs to be avoided as much as > possible. > > In btrfs_degrag_leaves, path->keep_lock is set but @path immediatley > gets released, which is not necessary at all. > This is still very terse and still doesn't explain why keep_locks should be set right before btrfs_search_slot and not the beginning of the function. "when necessary" is perhaps the second most uninformative reason of doing something, right after "because of reasons" > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > --- > v2: update commit log with more details. > > fs/btrfs/tree-defrag.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/tree-defrag.c b/fs/btrfs/tree-defrag.c > index 3c0987ab587d..c12747904d4c 100644 > --- a/fs/btrfs/tree-defrag.c > +++ b/fs/btrfs/tree-defrag.c > @@ -65,8 +65,6 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, > memcpy(&key, &root->defrag_progress, sizeof(key)); > } > > - path->keep_locks = 1; > - > ret = btrfs_search_forward(root, &key, path, BTRFS_OLDEST_GENERATION); > if (ret < 0) > goto out; > @@ -81,6 +79,7 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, > * a deadlock (attempting to write lock an already write locked leaf). > */ > path->lowest_level = 1; > + path->keep_locks = 1; > wret = btrfs_search_slot(trans, root, &key, path, 0, 1); > > if (wret < 0) { > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves 2018-04-26 1:27 [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves Liu Bo 2018-04-26 4:57 ` Nikolay Borisov @ 2018-04-26 18:06 ` David Sterba 2018-04-27 3:23 ` Liu Bo 2018-04-27 5:36 ` Liu Bo 1 sibling, 2 replies; 7+ messages in thread From: David Sterba @ 2018-04-26 18:06 UTC (permalink / raw) To: Liu Bo; +Cc: linux-btrfs On Thu, Apr 26, 2018 at 09:27:43AM +0800, Liu Bo wrote: > path->keep_lock may force to lock tree root and higher nodes, and make > lock contention worse, thus it needs to be avoided as much as > possible. > > In btrfs_degrag_leaves, path->keep_lock is set but @path immediatley > gets released, which is not necessary at all. So what is the effect of this? The locks are held throughout btrfs_search_forward and released after that. Thi > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > --- > v2: update commit log with more details. > > fs/btrfs/tree-defrag.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/tree-defrag.c b/fs/btrfs/tree-defrag.c > index 3c0987ab587d..c12747904d4c 100644 > --- a/fs/btrfs/tree-defrag.c > +++ b/fs/btrfs/tree-defrag.c > @@ -65,8 +65,6 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, > memcpy(&key, &root->defrag_progress, sizeof(key)); > } > > - path->keep_locks = 1; > - > ret = btrfs_search_forward(root, &key, path, BTRFS_OLDEST_GENERATION); What does btrfs_search_forward do as the first statement: 5115 int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key, 5116 struct btrfs_path *path, 5117 u64 min_trans) 5118 { .... declarations 5128 5129 path->keep_locks = 1; So even if removed from above, there will be no change. The value of keep_locks is preserved after btrfs_path_release. > if (ret < 0) > goto out; > @@ -81,6 +79,7 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, > * a deadlock (attempting to write lock an already write locked leaf). > */ > path->lowest_level = 1; > + path->keep_locks = 1; > wret = btrfs_search_slot(trans, root, &key, path, 0, 1); > > if (wret < 0) { > -- > 1.8.3.1 > > -- > 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 v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves 2018-04-26 18:06 ` David Sterba @ 2018-04-27 3:23 ` Liu Bo 2018-04-27 10:55 ` David Sterba 2018-04-27 5:36 ` Liu Bo 1 sibling, 1 reply; 7+ messages in thread From: Liu Bo @ 2018-04-27 3:23 UTC (permalink / raw) To: David Sterba, Liu Bo, linux-btrfs On Fri, Apr 27, 2018 at 2:06 AM, David Sterba <dsterba@suse.cz> wrote: > On Thu, Apr 26, 2018 at 09:27:43AM +0800, Liu Bo wrote: >> path->keep_lock may force to lock tree root and higher nodes, and make >> lock contention worse, thus it needs to be avoided as much as >> possible. >> >> In btrfs_degrag_leaves, path->keep_lock is set but @path immediatley >> gets released, which is not necessary at all. > > So what is the effect of this? The locks are held throughout > btrfs_search_forward and released after that. Thi > >> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> >> --- >> v2: update commit log with more details. >> >> fs/btrfs/tree-defrag.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/tree-defrag.c b/fs/btrfs/tree-defrag.c >> index 3c0987ab587d..c12747904d4c 100644 >> --- a/fs/btrfs/tree-defrag.c >> +++ b/fs/btrfs/tree-defrag.c >> @@ -65,8 +65,6 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, >> memcpy(&key, &root->defrag_progress, sizeof(key)); >> } >> >> - path->keep_locks = 1; >> - >> ret = btrfs_search_forward(root, &key, path, BTRFS_OLDEST_GENERATION); > > What does btrfs_search_forward do as the first statement: > > 5115 int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key, > 5116 struct btrfs_path *path, > 5117 u64 min_trans) > 5118 { > .... declarations > 5128 > 5129 path->keep_locks = 1; > > So even if removed from above, there will be no change. The value of > keep_locks is preserved after btrfs_path_release. > It will set it back, out: path->keep_locks = keep_locks; thanks, liubo >> if (ret < 0) >> goto out; >> @@ -81,6 +79,7 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, >> * a deadlock (attempting to write lock an already write locked leaf). >> */ >> path->lowest_level = 1; >> + path->keep_locks = 1; >> wret = btrfs_search_slot(trans, root, &key, path, 0, 1); >> >> if (wret < 0) { >> -- >> 1.8.3.1 >> >> -- >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves 2018-04-27 3:23 ` Liu Bo @ 2018-04-27 10:55 ` David Sterba 0 siblings, 0 replies; 7+ messages in thread From: David Sterba @ 2018-04-27 10:55 UTC (permalink / raw) To: Liu Bo; +Cc: David Sterba, Liu Bo, linux-btrfs On Fri, Apr 27, 2018 at 11:23:23AM +0800, Liu Bo wrote: > >> --- > >> v2: update commit log with more details. > >> > >> fs/btrfs/tree-defrag.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/fs/btrfs/tree-defrag.c b/fs/btrfs/tree-defrag.c > >> index 3c0987ab587d..c12747904d4c 100644 > >> --- a/fs/btrfs/tree-defrag.c > >> +++ b/fs/btrfs/tree-defrag.c > >> @@ -65,8 +65,6 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, > >> memcpy(&key, &root->defrag_progress, sizeof(key)); > >> } > >> > >> - path->keep_locks = 1; > >> - > >> ret = btrfs_search_forward(root, &key, path, BTRFS_OLDEST_GENERATION); > > > > What does btrfs_search_forward do as the first statement: > > > > 5115 int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key, > > 5116 struct btrfs_path *path, > > 5117 u64 min_trans) > > 5118 { > > .... declarations > > 5128 > > 5129 path->keep_locks = 1; > > > > So even if removed from above, there will be no change. The value of > > keep_locks is preserved after btrfs_path_release. > > > > It will set it back, > > out: > path->keep_locks = keep_locks; Oh, right. So much for reading the whole function. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves 2018-04-26 18:06 ` David Sterba 2018-04-27 3:23 ` Liu Bo @ 2018-04-27 5:36 ` Liu Bo 2018-05-01 13:39 ` David Sterba 1 sibling, 1 reply; 7+ messages in thread From: Liu Bo @ 2018-04-27 5:36 UTC (permalink / raw) To: David Sterba, Liu Bo, linux-btrfs On Fri, Apr 27, 2018 at 2:06 AM, David Sterba <dsterba@suse.cz> wrote: > On Thu, Apr 26, 2018 at 09:27:43AM +0800, Liu Bo wrote: >> path->keep_lock may force to lock tree root and higher nodes, and make >> lock contention worse, thus it needs to be avoided as much as >> possible. >> >> In btrfs_degrag_leaves, path->keep_lock is set but @path immediatley >> gets released, which is not necessary at all. > > So what is the effect of this? The locks are held throughout > btrfs_search_forward and released after that. Thi > >> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> >> --- >> v2: update commit log with more details. >> >> fs/btrfs/tree-defrag.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/tree-defrag.c b/fs/btrfs/tree-defrag.c >> index 3c0987ab587d..c12747904d4c 100644 >> --- a/fs/btrfs/tree-defrag.c >> +++ b/fs/btrfs/tree-defrag.c >> @@ -65,8 +65,6 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, >> memcpy(&key, &root->defrag_progress, sizeof(key)); >> } >> >> - path->keep_locks = 1; >> - >> ret = btrfs_search_forward(root, &key, path, BTRFS_OLDEST_GENERATION); > > What does btrfs_search_forward do as the first statement: > > 5115 int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key, > 5116 struct btrfs_path *path, > 5117 u64 min_trans) > 5118 { > .... declarations > 5128 > 5129 path->keep_locks = 1; > > So even if removed from above, there will be no change. The value of > keep_locks is preserved after btrfs_path_release. > FYI, btrfs_search_forward() doesn't need keep_locks's semantics as all of its callers only access path->nodes[0], thus I'm planning to remove keep_locks setting inside it, too. thanks, liubo >> if (ret < 0) >> goto out; >> @@ -81,6 +79,7 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans, >> * a deadlock (attempting to write lock an already write locked leaf). >> */ >> path->lowest_level = 1; >> + path->keep_locks = 1; >> wret = btrfs_search_slot(trans, root, &key, path, 0, 1); >> >> if (wret < 0) { >> -- >> 1.8.3.1 >> >> -- >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves 2018-04-27 5:36 ` Liu Bo @ 2018-05-01 13:39 ` David Sterba 0 siblings, 0 replies; 7+ messages in thread From: David Sterba @ 2018-05-01 13:39 UTC (permalink / raw) To: Liu Bo; +Cc: David Sterba, Liu Bo, linux-btrfs On Fri, Apr 27, 2018 at 01:36:35PM +0800, Liu Bo wrote: > > What does btrfs_search_forward do as the first statement: > > > > 5115 int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key, > > 5116 struct btrfs_path *path, > > 5117 u64 min_trans) > > 5118 { > > .... declarations > > 5128 > > 5129 path->keep_locks = 1; > > > > So even if removed from above, there will be no change. The value of > > keep_locks is preserved after btrfs_path_release. > > > > FYI, btrfs_search_forward() doesn't need keep_locks's semantics as all > of its callers only access path->nodes[0], thus I'm planning to remove > keep_locks setting inside it, too. Ok. Please update the changelog of this patch and note something about btrfs_search_forward that sets the path lock on it's own and that there's no change in defrag leaves. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-01 13:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-26 1:27 [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves Liu Bo 2018-04-26 4:57 ` Nikolay Borisov 2018-04-26 18:06 ` David Sterba 2018-04-27 3:23 ` Liu Bo 2018-04-27 10:55 ` David Sterba 2018-04-27 5:36 ` Liu Bo 2018-05-01 13:39 ` 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.