Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves
On Fri, Apr 27, 2018 at 2:06 AM, David Sterba 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
>> ---
>> 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 majord...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves
On Fri, Apr 27, 2018 at 2:06 AM, David Sterba 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
>> ---
>> 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 majord...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves
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
> ---
> 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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves
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
> ---
> 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) {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Btrfs: set keep_lock when necessary in btrfs_defrag_leaves
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
