Re: [PATCH] btrfs: reduce return argument of btrfs_chunk_readonly

Re: [PATCH] btrfs: reduce return argument of btrfs_chunk_readonly

[Nikolay Borisov]

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>

Re: [PATCH] btrfs: reduce return argument of btrfs_chunk_readonly

[Anand Jain]

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

[PATCH RESEND] btrfs: reduce return argument of btrfs_chunk_readonly

[Anand Jain]

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

Re: [PATCH] btrfs: reduce return argument of btrfs_chunk_readonly

[David Sterba]

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.