Re: [PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[David Sterba]

On Sun, May 27, 2018 at 09:25:25AM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年05月22日 20:14, David Sterba wrote:
> > On Tue, May 22, 2018 at 04:43:47PM +0800, Qu Wenruo wrote:
> >> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
> >> locks and add a block group to unused_bgs list.
> > 
> > The helper is nice but hides that there's a reference taken on the 'bg'.
> > This would be good to add at least to the function comment or somehow
> > squeeze it into the function name itself, like
> > btrfs_get_and_mark_bg_unused.
> 
> That btrfs_get_block_group() call is only for later removal.
> The reference will be removed in btrfs_delete_unused_bgs().
> 
> So I don't think the function name needs the "_get" part.

Agreed, it's inside the helpers. Patch added to misc-next, thanks.
--
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: Use btrfs_mark_bg_unused() to replace open code

[Qu Wenruo]

On 2018年05月22日 20:14, David Sterba wrote:
> On Tue, May 22, 2018 at 04:43:47PM +0800, Qu Wenruo wrote:
>> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
>> locks and add a block group to unused_bgs list.
> 
> The helper is nice but hides that there's a reference taken on the 'bg'.
> This would be good to add at least to the function comment or somehow
> squeeze it into the function name itself, like
> btrfs_get_and_mark_bg_unused.

That btrfs_get_block_group() call is only for later removal.
The reference will be removed in btrfs_delete_unused_bgs().

So I don't think the function name needs the "_get" part.

Although I could add more comment about the btrfs_get_block_group() call.

How do you think about this?

Thanks,
Qu

> --
> 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
> 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[David Sterba]

On Tue, May 22, 2018 at 04:43:47PM +0800, Qu Wenruo wrote:
> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
> locks and add a block group to unused_bgs list.

The helper is nice but hides that there's a reference taken on the 'bg'.
This would be good to add at least to the function comment or somehow
squeeze it into the function name itself, like
btrfs_get_and_mark_bg_unused.
--
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: Use btrfs_mark_bg_unused() to replace open code

[Nikolay Borisov]

On 22.05.2018 11:43, Qu Wenruo wrote:
> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
> locks and add a block group to unused_bgs list.
> 
> No functional modification, and only 3 callers are involved.>
> Signed-off-by: Qu Wenruo 

Reviewed-by: Nikolay Borisov 

> ---
> This patch should provide the basis for later block group auto-removal
> to get more info (mostly transid) to determine should one block group
> being removed in current trans.
> 
> changelog:
> v2:
>   Add ASSERT() for btrfs_read_block_groups(), as in that call site a
>   block group should not be added to other list (new_bgs or
>   deleted_bgs).
>   Rename the function to btrfs_mark_bg_unused().
>   Both suggested by Nikolay.
> ---
>  fs/btrfs/ctree.h   |  1 +
>  fs/btrfs/extent-tree.c | 36 +---
>  fs/btrfs/scrub.c   |  9 +
>  3 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index bbb358143ded..4a34bc443fe4 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2827,6 +2827,7 @@ void check_system_chunk(struct btrfs_trans_handle 
> *trans,
>   struct btrfs_fs_info *fs_info, const u64 type);
>  u64 add_new_free_space(struct btrfs_block_group_cache *block_group,
>  u64 start, u64 end);
> +void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg);
>  
>  /* ctree.c */
>  int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index ccf2690f7ca1..33f046103073 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6312,16 +6312,8 @@ static int update_block_group(struct 
> btrfs_trans_handle *trans,
>* dirty list to avoid races between cleaner kthread and space
>* cache writeout.
>*/
> - if (!alloc && old_val == 0) {
> - spin_lock(>unused_bgs_lock);
> - if (list_empty(>bg_list)) {
> - btrfs_get_block_group(cache);
> - trace_btrfs_add_unused_block_group(cache);
> - list_add_tail(>bg_list,
> -   >unused_bgs);
> - }
> - spin_unlock(>unused_bgs_lock);
> - }
> + if (!alloc && old_val == 0)
> + btrfs_mark_bg_unused(cache);
>  
>   btrfs_put_block_group(cache);
>   total -= num_bytes;
> @@ -10144,15 +10136,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info 
> *info)
>   if (btrfs_chunk_readonly(info, cache->key.objectid)) {
>   inc_block_group_ro(cache, 1);
>   } else if (btrfs_block_group_used(>item) == 0) {
> - spin_lock(>unused_bgs_lock);
> - /* Should always be true but just in case. */
> - if (list_empty(>bg_list)) {
> - btrfs_get_block_group(cache);
> - trace_btrfs_add_unused_block_group(cache);
> - list_add_tail(>bg_list,
> -   >unused_bgs);
> - }
> - spin_unlock(>unused_bgs_lock);
> + ASSERT(list_empty(>bg_list));
> + btrfs_mark_bg_unused(cache);
>   }
>   }
>  
> @@ -11071,3 +11056,16 @@ void btrfs_wait_for_snapshot_creation(struct 
> btrfs_root *root)
>  !atomic_read(>will_be_snapshotted));
>   }
>  }
> +
> +void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg)
> +{
> + struct btrfs_fs_info *fs_info = bg->fs_info;
> +
> + spin_lock(_info->unused_bgs_lock);
> + if (list_empty(>bg_list)) {
> + btrfs_get_block_group(bg);
> + trace_btrfs_add_unused_block_group(bg);
> + list_add_tail(>bg_list, _info->unused_bgs);
> + }
> + spin_unlock(_info->unused_bgs_lock);
> +}
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index a59005862010..40086b47a65f 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3981,14 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
>   if (!cache->removed && !cache->ro && cache->reserved == 0 &&
>   btrfs_block_group_used(>item) == 0) {
>   spin_unlock(>lock);
> - spin_lock(_info->unused_bgs_lock);
> - if (list_empty(>bg_list)) {
> - btrfs_get_block_group(cache);
> - trace_btrfs_add_unused_block_group(cache);
> - list_add_tail(>bg_list,
> -   _info->unused_bgs);
> - }
> - 

[PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[Qu Wenruo]

Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
locks and add a block group to unused_bgs list.

No functional modification, and only 3 callers are involved.

Signed-off-by: Qu Wenruo 
---
This patch should provide the basis for later block group auto-removal
to get more info (mostly transid) to determine should one block group
being removed in current trans.

changelog:
v2:
  Add ASSERT() for btrfs_read_block_groups(), as in that call site a
  block group should not be added to other list (new_bgs or
  deleted_bgs).
  Rename the function to btrfs_mark_bg_unused().
  Both suggested by Nikolay.
---
 fs/btrfs/ctree.h   |  1 +
 fs/btrfs/extent-tree.c | 36 +---
 fs/btrfs/scrub.c   |  9 +
 3 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index bbb358143ded..4a34bc443fe4 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2827,6 +2827,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, const u64 type);
 u64 add_new_free_space(struct btrfs_block_group_cache *block_group,
   u64 start, u64 end);
+void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg);
 
 /* ctree.c */
 int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ccf2690f7ca1..33f046103073 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6312,16 +6312,8 @@ static int update_block_group(struct btrfs_trans_handle 
*trans,
 * dirty list to avoid races between cleaner kthread and space
 * cache writeout.
 */
-   if (!alloc && old_val == 0) {
-   spin_lock(>unused_bgs_lock);
-   if (list_empty(>bg_list)) {
-   btrfs_get_block_group(cache);
-   trace_btrfs_add_unused_block_group(cache);
-   list_add_tail(>bg_list,
- >unused_bgs);
-   }
-   spin_unlock(>unused_bgs_lock);
-   }
+   if (!alloc && old_val == 0)
+   btrfs_mark_bg_unused(cache);
 
btrfs_put_block_group(cache);
total -= num_bytes;
@@ -10144,15 +10136,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
if (btrfs_chunk_readonly(info, cache->key.objectid)) {
inc_block_group_ro(cache, 1);
} else if (btrfs_block_group_used(>item) == 0) {
-   spin_lock(>unused_bgs_lock);
-   /* Should always be true but just in case. */
-   if (list_empty(>bg_list)) {
-   btrfs_get_block_group(cache);
-   trace_btrfs_add_unused_block_group(cache);
-   list_add_tail(>bg_list,
- >unused_bgs);
-   }
-   spin_unlock(>unused_bgs_lock);
+   ASSERT(list_empty(>bg_list));
+   btrfs_mark_bg_unused(cache);
}
}
 
@@ -11071,3 +11056,16 @@ void btrfs_wait_for_snapshot_creation(struct 
btrfs_root *root)
   !atomic_read(>will_be_snapshotted));
}
 }
+
+void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg)
+{
+   struct btrfs_fs_info *fs_info = bg->fs_info;
+
+   spin_lock(_info->unused_bgs_lock);
+   if (list_empty(>bg_list)) {
+   btrfs_get_block_group(bg);
+   trace_btrfs_add_unused_block_group(bg);
+   list_add_tail(>bg_list, _info->unused_bgs);
+   }
+   spin_unlock(_info->unused_bgs_lock);
+}
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index a59005862010..40086b47a65f 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3981,14 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
if (!cache->removed && !cache->ro && cache->reserved == 0 &&
btrfs_block_group_used(>item) == 0) {
spin_unlock(>lock);
-   spin_lock(_info->unused_bgs_lock);
-   if (list_empty(>bg_list)) {
-   btrfs_get_block_group(cache);
-   trace_btrfs_add_unused_block_group(cache);
-   list_add_tail(>bg_list,
- _info->unused_bgs);
-   }
-   spin_unlock(_info->unused_bgs_lock);
+   btrfs_mark_bg_unused(cache);
} else {
spin_unlock(>lock);
}
-- 
2.17.0

--
To unsubscribe from this list: send the