* [PATCH RESEND] btrfs: reduce return argument of btrfs_chunk_readonly [not found] <20210824052742.6E-Km1SWTTGkqvPnsnUECQI3TIDMt6LfxrwXZGZrNE8@z> @ 2021-08-24 5:27 ` Anand Jain 2021-08-11 7:15 ` [PATCH] " Nikolay Borisov 0 siblings, 1 reply; 4+ messages in thread From: Anand Jain @ 2021-08-24 5:27 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba btrfs_chunk_readonly() checks if the given chunk is writeable. It returns 1 for readonly, and 0 for writeable. So the return argument type bool shall suffice instead of the current type int. Also, rename btrfs_chunk_readonly() to btrfs_chunk_writeable() as we check if the bg is writeable, and helps to keep the logic at the parent function simpler to understand. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/block-group.c | 17 ++++++++++------- fs/btrfs/volumes.c | 12 ++++++------ fs/btrfs/volumes.h | 2 +- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c index a3b830b8410a..d7e06b4bae26 100644 --- a/fs/btrfs/block-group.c +++ b/fs/btrfs/block-group.c @@ -2062,15 +2062,18 @@ static int read_one_block_group(struct btrfs_fs_info *info, link_block_group(cache); set_avail_alloc_bits(info, cache->flags); - if (btrfs_chunk_readonly(info, cache->start)) { + if (btrfs_chunk_writeable(info, cache->start)) { + if (cache->used == 0) { + ASSERT(list_empty(&cache->bg_list)); + if (btrfs_test_opt(info, DISCARD_ASYNC)) + btrfs_discard_queue_work(&info->discard_ctl, cache); + else + btrfs_mark_bg_unused(cache); + } + } else { inc_block_group_ro(cache, 1); - } else if (cache->used == 0) { - ASSERT(list_empty(&cache->bg_list)); - if (btrfs_test_opt(info, DISCARD_ASYNC)) - btrfs_discard_queue_work(&info->discard_ctl, cache); - else - btrfs_mark_bg_unused(cache); } + return 0; error: btrfs_put_block_group(cache); diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 87d4c34ea35b..9cb4ed90888d 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -5568,17 +5568,17 @@ static inline int btrfs_chunk_max_errors(struct map_lookup *map) return btrfs_raid_array[index].tolerated_failures; } -int btrfs_chunk_readonly(struct btrfs_fs_info *fs_info, u64 chunk_offset) +bool btrfs_chunk_writeable(struct btrfs_fs_info *fs_info, u64 chunk_offset) { struct extent_map *em; struct map_lookup *map; - int readonly = 0; int miss_ndevs = 0; int i; + bool ret = true; em = btrfs_get_chunk_map(fs_info, chunk_offset, 1); if (IS_ERR(em)) - return 1; + return false; map = em->map_lookup; for (i = 0; i < map->num_stripes; i++) { @@ -5589,7 +5589,7 @@ int btrfs_chunk_readonly(struct btrfs_fs_info *fs_info, u64 chunk_offset) } if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &map->stripes[i].dev->dev_state)) { - readonly = 1; + ret = false; goto end; } } @@ -5600,10 +5600,10 @@ int btrfs_chunk_readonly(struct btrfs_fs_info *fs_info, u64 chunk_offset) * set it readonly. */ if (miss_ndevs > btrfs_chunk_max_errors(map)) - readonly = 1; + ret = false; end: free_extent_map(em); - return readonly; + return ret; } void btrfs_mapping_tree_free(struct extent_map_tree *tree) diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index b082250b42e0..4e1627b64a84 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -492,7 +492,7 @@ int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset); int btrfs_cancel_balance(struct btrfs_fs_info *fs_info); int btrfs_create_uuid_tree(struct btrfs_fs_info *fs_info); int btrfs_uuid_scan_kthread(void *data); -int btrfs_chunk_readonly(struct btrfs_fs_info *fs_info, u64 chunk_offset); +bool btrfs_chunk_writeable(struct btrfs_fs_info *fs_info, u64 chunk_offset); int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes, u64 *start, u64 *max_avail); void btrfs_dev_stat_inc_and_print(struct btrfs_device *dev, int index); -- 2.31.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: reduce return argument of btrfs_chunk_readonly 2021-08-24 5:27 ` [PATCH RESEND] btrfs: reduce return argument of btrfs_chunk_readonly Anand Jain @ 2021-08-11 7:15 ` Nikolay Borisov 2021-08-12 8:58 ` Anand Jain 0 siblings, 1 reply; 4+ messages in thread From: Nikolay Borisov @ 2021-08-11 7:15 UTC (permalink / raw) To: Anand Jain, linux-btrfs On 10.08.21 г. 18:48, Anand Jain wrote: > btrfs_chunk_readonly() checks if the given chunk is writeable. It returns > 1 for readonly, and 0 for writeable. So the return argument type bool > shall suffice instead of the current type int. > > Also, rename btrfs_chunk_readonly() to btrfs_chunk_writeable() as we check > if the bg is writeable, and helps to keep the logic at the parent function > simpler. I don't see how the logic is kept simpler, given that you just invert it. IMO changing the argument to bool without renaming the function is a sufficient change and it will result in a lot smaller diff. <snip> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: reduce return argument of btrfs_chunk_readonly 2021-08-11 7:15 ` [PATCH] " Nikolay Borisov @ 2021-08-12 8:58 ` Anand Jain 2021-08-26 18:10 ` David Sterba 0 siblings, 1 reply; 4+ messages in thread From: Anand Jain @ 2021-08-12 8:58 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs On 11/08/2021 15:15, Nikolay Borisov wrote: > > > On 10.08.21 г. 18:48, Anand Jain wrote: >> btrfs_chunk_readonly() checks if the given chunk is writeable. It returns >> 1 for readonly, and 0 for writeable. So the return argument type bool >> shall suffice instead of the current type int. >> >> Also, rename btrfs_chunk_readonly() to btrfs_chunk_writeable() as we check >> if the bg is writeable, and helps to keep the logic at the parent function >> simpler. > I don't see how the logic is kept simpler, given that you just invert > it. IMO it is simpler to read. No? In btrfs_chunk_readonly(), we consider a chunk is readonly when the device it is on has _no_ DEV_STATE_WRITEABLE flag set. So rename to btrfs_chunk_writeable() is correct. We also use readonly to the filesystem. I will wait to see if David also has the same opinion. I am ok to drop this part. > IMO changing the argument to bool without renaming the function is a > sufficient change and it will result in a lot smaller diff. OK. Thx, Anand ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: reduce return argument of btrfs_chunk_readonly 2021-08-12 8:58 ` Anand Jain @ 2021-08-26 18:10 ` David Sterba 0 siblings, 0 replies; 4+ messages in thread From: David Sterba @ 2021-08-26 18:10 UTC (permalink / raw) To: Anand Jain; +Cc: Nikolay Borisov, linux-btrfs On Thu, Aug 12, 2021 at 04:58:30PM +0800, Anand Jain wrote: > On 11/08/2021 15:15, Nikolay Borisov wrote: > > On 10.08.21 г. 18:48, Anand Jain wrote: > >> btrfs_chunk_readonly() checks if the given chunk is writeable. It returns > >> 1 for readonly, and 0 for writeable. So the return argument type bool > >> shall suffice instead of the current type int. > >> > >> Also, rename btrfs_chunk_readonly() to btrfs_chunk_writeable() as we check > >> if the bg is writeable, and helps to keep the logic at the parent function > >> simpler. > > > I don't see how the logic is kept simpler, given that you just invert > > it. > > IMO it is simpler to read. No? In btrfs_chunk_readonly(), we consider a > chunk is readonly when the device it is on has _no_ DEV_STATE_WRITEABLE > flag set. So rename to btrfs_chunk_writeable() is correct. We also use > readonly to the filesystem. The logic in the function is to check for each stripe if it has writeable flag and short cuircit if not, and this follows as we're indeed checking for the writeable status. Further there's a check for the missing devices that can drop previous witebale status. This all reads as the the main point is 'writeable' status, so I'm fine with the rename. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-26 18:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210824052742.6E-Km1SWTTGkqvPnsnUECQI3TIDMt6LfxrwXZGZrNE8@z>
2021-08-24 5:27 ` [PATCH RESEND] btrfs: reduce return argument of btrfs_chunk_readonly Anand Jain
2021-08-11 7:15 ` [PATCH] " Nikolay Borisov
2021-08-12 8:58 ` Anand Jain
2021-08-26 18:10 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).