2024-05-08 06:26:09

by Zhang Yi

Subject: [PATCH v3 08/10] ext4: factor out check for whether a cluster is allocated

From: Zhang Yi <[email protected]>

Factor out a common helper ext4_da_check_clu_allocated(), check whether
the cluster containing a delalloc block to be added has been delayed or
allocated, no logic changes.

Signed-off-by: Zhang Yi <[email protected]>
---
fs/ext4/inode.c | 52 +++++++++++++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 0c52969654ac..6e418d3f8e87 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1649,6 +1649,34 @@ static void ext4_print_free_blocks(struct inode *inode)
return;
}

+/*
+ * Check whether the cluster containing lblk has been delayed or allocated,
+ * if not, it means we should reserve a cluster when add delalloc, return 1,
+ * otherwise return 0 or error code.
+ */
+static int ext4_da_check_clu_allocated(struct inode *inode, ext4_lblk_t lblk,
+ bool *allocated)
+{
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+ int ret;
+
+ *allocated = false;
+ if (ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk))
+ return 0;
+
+ if (ext4_es_scan_clu(inode, &ext4_es_is_mapped, lblk))
+ goto allocated;
+
+ ret = ext4_clu_mapped(inode, EXT4_B2C(sbi, lblk));
+ if (ret < 0)
+ return ret;
+ if (ret == 0)
+ return 1;
+allocated:
+ *allocated = true;
+ return 0;
+}
+
/*
* ext4_insert_delayed_block - adds a delayed block to the extents status
* tree, incrementing the reserved cluster/block
@@ -1682,23 +1710,13 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
if (ret != 0) /* ENOSPC */
return ret;
} else { /* bigalloc */
- if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
- if (!ext4_es_scan_clu(inode,
- &ext4_es_is_mapped, lblk)) {
- ret = ext4_clu_mapped(inode,
- EXT4_B2C(sbi, lblk));
- if (ret < 0)
- return ret;
- if (ret == 0) {
- ret = ext4_da_reserve_space(inode, 1);
- if (ret != 0) /* ENOSPC */
- return ret;
- } else {
- allocated = true;
- }
- } else {
- allocated = true;
- }
+ ret = ext4_da_check_clu_allocated(inode, lblk, &allocated);
+ if (ret < 0)
+ return ret;
+ if (ret > 0) {
+ ret = ext4_da_reserve_space(inode, 1);
+ if (ret != 0) /* ENOSPC */
+ return ret;
}
}

--
2.39.2

2024-05-13 13:43:12

by Jan Kara

Subject: Re: [PATCH v3 08/10] ext4: factor out check for whether a cluster is allocated

On Wed 08-05-24 14:12:18, Zhang Yi wrote:
> From: Zhang Yi <[email protected]>
>
> Factor out a common helper ext4_da_check_clu_allocated(), check whether
> the cluster containing a delalloc block to be added has been delayed or
> allocated, no logic changes.
>
> Signed-off-by: Zhang Yi <[email protected]>

I have one suggestion for improvement here.

> +/*
> + * Check whether the cluster containing lblk has been delayed or allocated,
> + * if not, it means we should reserve a cluster when add delalloc, return 1,
> + * otherwise return 0 or error code.
> + */
> +static int ext4_da_check_clu_allocated(struct inode *inode, ext4_lblk_t lblk,
> + bool *allocated)

The name of the function does not quite match what it is returning and that
is confusing. Essentially we have three states here:

a) cluster allocated
b) cluster has delalloc reservation
c) cluster doesn't have either

So maybe we could call the function ext4_clu_alloc_state() and return 0 /
1 / 2 based on the state?

Honza

> +{
> + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> + int ret;
> +
> + *allocated = false;
> + if (ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk))
> + return 0;
> +
> + if (ext4_es_scan_clu(inode, &ext4_es_is_mapped, lblk))
> + goto allocated;
> +
> + ret = ext4_clu_mapped(inode, EXT4_B2C(sbi, lblk));
> + if (ret < 0)
> + return ret;
> + if (ret == 0)
> + return 1;
> +allocated:
> + *allocated = true;
> + return 0;
> +}
> +
> /*
> * ext4_insert_delayed_block - adds a delayed block to the extents status
> * tree, incrementing the reserved cluster/block
> @@ -1682,23 +1710,13 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
> if (ret != 0) /* ENOSPC */
> return ret;
> } else { /* bigalloc */
> - if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
> - if (!ext4_es_scan_clu(inode,
> - &ext4_es_is_mapped, lblk)) {
> - ret = ext4_clu_mapped(inode,
> - EXT4_B2C(sbi, lblk));
> - if (ret < 0)
> - return ret;
> - if (ret == 0) {
> - ret = ext4_da_reserve_space(inode, 1);
> - if (ret != 0) /* ENOSPC */
> - return ret;
> - } else {
> - allocated = true;
> - }
> - } else {
> - allocated = true;
> - }
> + ret = ext4_da_check_clu_allocated(inode, lblk, &allocated);
> + if (ret < 0)
> + return ret;
> + if (ret > 0) {
> + ret = ext4_da_reserve_space(inode, 1);
> + if (ret != 0) /* ENOSPC */
> + return ret;
> }
> }
>
> --
> 2.39.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2024-05-14 02:38:22

by Zhang Yi

Subject: Re: [PATCH v3 08/10] ext4: factor out check for whether a cluster is allocated

On 2024/5/12 23:40, Jan Kara wrote:
> On Wed 08-05-24 14:12:18, Zhang Yi wrote:
>> From: Zhang Yi <[email protected]>
>>
>> Factor out a common helper ext4_da_check_clu_allocated(), check whether
>> the cluster containing a delalloc block to be added has been delayed or
>> allocated, no logic changes.
>>
>> Signed-off-by: Zhang Yi <[email protected]>
>
> I have one suggestion for improvement here.
>
>> +/*
>> + * Check whether the cluster containing lblk has been delayed or allocated,
>> + * if not, it means we should reserve a cluster when add delalloc, return 1,
>> + * otherwise return 0 or error code.
>> + */
>> +static int ext4_da_check_clu_allocated(struct inode *inode, ext4_lblk_t lblk,
>> + bool *allocated)
>
> The name of the function does not quite match what it is returning and that
> is confusing. Essentially we have three states here:
>
> a) cluster allocated
> b) cluster has delalloc reservation
> c) cluster doesn't have either
>
> So maybe we could call the function ext4_clu_alloc_state() and return 0 /
> 1 / 2 based on the state?
>
> Honza

Sure, thanks for the suggestion, it looks better.

Thanks,
Yi.

>
>> +{
>> + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>> + int ret;
>> +
>> + *allocated = false;
>> + if (ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk))
>> + return 0;
>> +
>> + if (ext4_es_scan_clu(inode, &ext4_es_is_mapped, lblk))
>> + goto allocated;
>> +
>> + ret = ext4_clu_mapped(inode, EXT4_B2C(sbi, lblk));
>> + if (ret < 0)
>> + return ret;
>> + if (ret == 0)
>> + return 1;
>> +allocated:
>> + *allocated = true;
>> + return 0;
>> +}
>> +
>> /*
>> * ext4_insert_delayed_block - adds a delayed block to the extents status
>> * tree, incrementing the reserved cluster/block
>> @@ -1682,23 +1710,13 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>> if (ret != 0) /* ENOSPC */
>> return ret;
>> } else { /* bigalloc */
>> - if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
>> - if (!ext4_es_scan_clu(inode,
>> - &ext4_es_is_mapped, lblk)) {
>> - ret = ext4_clu_mapped(inode,
>> - EXT4_B2C(sbi, lblk));
>> - if (ret < 0)
>> - return ret;
>> - if (ret == 0) {
>> - ret = ext4_da_reserve_space(inode, 1);
>> - if (ret != 0) /* ENOSPC */
>> - return ret;
>> - } else {
>> - allocated = true;
>> - }
>> - } else {
>> - allocated = true;
>> - }
>> + ret = ext4_da_check_clu_allocated(inode, lblk, &allocated);
>> + if (ret < 0)
>> + return ret;
>> + if (ret > 0) {
>> + ret = ext4_da_reserve_space(inode, 1);
>> + if (ret != 0) /* ENOSPC */
>> + return ret;
>> }
>> }
>>
>> --
>> 2.39.2
>>