| Message ID | 20240410034203.2188357-9-yi.zhang@huaweicloud.com |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: support adding multi-delalloc blocks | expand |
On Wed 10-04-24 11:42:02, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > Rename ext4_insert_delayed_block() to ext4_insert_delayed_blocks(), > pass length parameter to make it insert multi delalloc blocks once a > time. For non-bigalloc case, just reserve len blocks and insert delalloc > extent. For bigalloc case, we can ensure the middle clusters are not > allocated, but need to check whether the start and end clusters are > delayed/allocated, if not, we should reserve more space for the start > and/or end block(s). > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> Thanks for the patch. Some comments below. > --- > fs/ext4/inode.c | 51 ++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 36 insertions(+), 15 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 46c34baa848a..08e2692b7286 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -1678,24 +1678,28 @@ static int ext4_da_check_clu_allocated(struct inode *inode, ext4_lblk_t lblk, > } > > /* > - * ext4_insert_delayed_block - adds a delayed block to the extents status > - * tree, incrementing the reserved cluster/block > - * count or making a pending reservation > - * where needed > + * ext4_insert_delayed_blocks - adds a multiple delayed blocks to the extents > + * status tree, incrementing the reserved > + * cluster/block count or making pending > + * reservations where needed > * > * @inode - file containing the newly added block > - * @lblk - logical block to be added > + * @lblk - start logical block to be added > + * @len - length of blocks to be added > * > * Returns 0 on success, negative error code on failure. > */ > -static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) > +static int ext4_insert_delayed_blocks(struct inode *inode, ext4_lblk_t lblk, > + ext4_lblk_t len) > { > struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); > - int ret; > - bool allocated = false; > + int resv_clu, ret; ^^^ this variable is in prinple the length of the extent. Thus it should be ext4_lblk_t type. > + bool lclu_allocated = false; > + bool end_allocated = false; > + ext4_lblk_t end = lblk + len - 1; > > /* > - * If the cluster containing lblk is shared with a delayed, > + * If the cluster containing lblk or end is shared with a delayed, > * written, or unwritten extent in a bigalloc file system, it's > * already been accounted for and does not need to be reserved. > * A pending reservation must be made for the cluster if it's > @@ -1706,21 +1710,38 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) > * extents status tree doesn't get a match. > */ > if (sbi->s_cluster_ratio == 1) { > - ret = ext4_da_reserve_space(inode, 1); > + ret = ext4_da_reserve_space(inode, len); > if (ret != 0) /* ENOSPC */ > return ret; > } else { /* bigalloc */ > - ret = ext4_da_check_clu_allocated(inode, lblk, &allocated); > + resv_clu = EXT4_B2C(sbi, end) - EXT4_B2C(sbi, lblk) - 1; > + if (resv_clu < 0) > + resv_clu = 0; Here resv_clu going negative is strange I'm not sure the math is 100% correct in all the cases. I think it would be more logical as: resv_clu = EXT4_B2C(sbi, end) - EXT4_B2C(sbi, lblk) + 1; and then update resv_clu below as: > + > + ret = ext4_da_check_clu_allocated(inode, lblk, &lclu_allocated); > if (ret < 0) > return ret; > - if (ret > 0) { > - ret = ext4_da_reserve_space(inode, 1); > + if (ret > 0) > + resv_clu++; Here we would do: if (ret == 0) resv_clu--; > + > + if (EXT4_B2C(sbi, lblk) != EXT4_B2C(sbi, end)) { > + ret = ext4_da_check_clu_allocated(inode, end, > + &end_allocated); > + if (ret < 0) > + return ret; > + if (ret > 0) > + resv_clu++; And similarly here: if (ret == 0) resv_clu--; Honza > + } > + > + if (resv_clu) { > + ret = ext4_da_reserve_space(inode, resv_clu); > if (ret != 0) /* ENOSPC */ > return ret; > } > } > > - ext4_es_insert_delayed_extent(inode, lblk, 1, allocated, false); > + ext4_es_insert_delayed_extent(inode, lblk, len, lclu_allocated, > + end_allocated); > return 0; > } > > @@ -1823,7 +1844,7 @@ static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map, > } > } > > - retval = ext4_insert_delayed_block(inode, map->m_lblk); > + retval = ext4_insert_delayed_blocks(inode, map->m_lblk, map->m_len); > up_write(&EXT4_I(inode)->i_data_sem); > if (retval) > return retval; > -- > 2.39.2 >
On 2024/4/29 18:06, Jan Kara wrote: > On Wed 10-04-24 11:42:02, Zhang Yi wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> Rename ext4_insert_delayed_block() to ext4_insert_delayed_blocks(), >> pass length parameter to make it insert multi delalloc blocks once a >> time. For non-bigalloc case, just reserve len blocks and insert delalloc >> extent. For bigalloc case, we can ensure the middle clusters are not >> allocated, but need to check whether the start and end clusters are >> delayed/allocated, if not, we should reserve more space for the start >> and/or end block(s). >> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > Thanks for the patch. Some comments below. > >> --- >> fs/ext4/inode.c | 51 ++++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 36 insertions(+), 15 deletions(-) >> >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index 46c34baa848a..08e2692b7286 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c >> @@ -1678,24 +1678,28 @@ static int ext4_da_check_clu_allocated(struct inode *inode, ext4_lblk_t lblk, >> } >> >> /* >> - * ext4_insert_delayed_block - adds a delayed block to the extents status >> - * tree, incrementing the reserved cluster/block >> - * count or making a pending reservation >> - * where needed >> + * ext4_insert_delayed_blocks - adds a multiple delayed blocks to the extents >> + * status tree, incrementing the reserved >> + * cluster/block count or making pending >> + * reservations where needed >> * >> * @inode - file containing the newly added block >> - * @lblk - logical block to be added >> + * @lblk - start logical block to be added >> + * @len - length of blocks to be added >> * >> * Returns 0 on success, negative error code on failure. >> */ >> -static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) >> +static int ext4_insert_delayed_blocks(struct inode *inode, ext4_lblk_t lblk, >> + ext4_lblk_t len) >> { >> struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); >> - int ret; >> - bool allocated = false; >> + int resv_clu, ret; > ^^^ this variable is in prinple the length of the extent. Thus > it should be ext4_lblk_t type. > >> + bool lclu_allocated = false; >> + bool end_allocated = false; >> + ext4_lblk_t end = lblk + len - 1; >> >> /* >> - * If the cluster containing lblk is shared with a delayed, >> + * If the cluster containing lblk or end is shared with a delayed, >> * written, or unwritten extent in a bigalloc file system, it's >> * already been accounted for and does not need to be reserved. >> * A pending reservation must be made for the cluster if it's >> @@ -1706,21 +1710,38 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) >> * extents status tree doesn't get a match. >> */ >> if (sbi->s_cluster_ratio == 1) { >> - ret = ext4_da_reserve_space(inode, 1); >> + ret = ext4_da_reserve_space(inode, len); >> if (ret != 0) /* ENOSPC */ >> return ret; >> } else { /* bigalloc */ >> - ret = ext4_da_check_clu_allocated(inode, lblk, &allocated); >> + resv_clu = EXT4_B2C(sbi, end) - EXT4_B2C(sbi, lblk) - 1; >> + if (resv_clu < 0) >> + resv_clu = 0; > > Here resv_clu going negative is strange I'm not sure the math is 100% > correct in all the cases. I think it would be more logical as: > > resv_clu = EXT4_B2C(sbi, end) - EXT4_B2C(sbi, lblk) + 1; >> and then update resv_clu below as: > >> + >> + ret = ext4_da_check_clu_allocated(inode, lblk, &lclu_allocated); >> if (ret < 0) >> return ret; >> - if (ret > 0) { >> - ret = ext4_da_reserve_space(inode, 1); >> + if (ret > 0) >> + resv_clu++; > > Here we would do: > if (ret == 0) > resv_clu--; > >> + >> + if (EXT4_B2C(sbi, lblk) != EXT4_B2C(sbi, end)) { >> + ret = ext4_da_check_clu_allocated(inode, end, >> + &end_allocated); >> + if (ret < 0) >> + return ret; >> + if (ret > 0) >> + resv_clu++; > > And similarly here: > if (ret == 0) > resv_clu--; Oh, yes, it is definitely more logical than mine. Thanks for taking time to review this series, other changelog and comments suggestions in this series are looks fine to me, I will use them. Besides, Ritesh improved my changelog in patch 2, I will keep your reviewed tag if you don't have different opinions. Thanks, Yi. > >> + } >> + >> + if (resv_clu) { >> + ret = ext4_da_reserve_space(inode, resv_clu); >> if (ret != 0) /* ENOSPC */ >> return ret; >> } >> } >> >> - ext4_es_insert_delayed_extent(inode, lblk, 1, allocated, false); >> + ext4_es_insert_delayed_extent(inode, lblk, len, lclu_allocated, >> + end_allocated); >> return 0; >> } >> >> @@ -1823,7 +1844,7 @@ static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map, >> } >> } >> >> - retval = ext4_insert_delayed_block(inode, map->m_lblk); >> + retval = ext4_insert_delayed_blocks(inode, map->m_lblk, map->m_len); >> up_write(&EXT4_I(inode)->i_data_sem); >> if (retval) >> return retval; >> -- >> 2.39.2 >>
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 46c34baa848a..08e2692b7286 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1678,24 +1678,28 @@ static int ext4_da_check_clu_allocated(struct inode *inode, ext4_lblk_t lblk, } /* - * ext4_insert_delayed_block - adds a delayed block to the extents status - * tree, incrementing the reserved cluster/block - * count or making a pending reservation - * where needed + * ext4_insert_delayed_blocks - adds a multiple delayed blocks to the extents + * status tree, incrementing the reserved + * cluster/block count or making pending + * reservations where needed * * @inode - file containing the newly added block - * @lblk - logical block to be added + * @lblk - start logical block to be added + * @len - length of blocks to be added * * Returns 0 on success, negative error code on failure. */ -static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) +static int ext4_insert_delayed_blocks(struct inode *inode, ext4_lblk_t lblk, + ext4_lblk_t len) { struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); - int ret; - bool allocated = false; + int resv_clu, ret; + bool lclu_allocated = false; + bool end_allocated = false; + ext4_lblk_t end = lblk + len - 1; /* - * If the cluster containing lblk is shared with a delayed, + * If the cluster containing lblk or end is shared with a delayed, * written, or unwritten extent in a bigalloc file system, it's * already been accounted for and does not need to be reserved. * A pending reservation must be made for the cluster if it's @@ -1706,21 +1710,38 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) * extents status tree doesn't get a match. */ if (sbi->s_cluster_ratio == 1) { - ret = ext4_da_reserve_space(inode, 1); + ret = ext4_da_reserve_space(inode, len); if (ret != 0) /* ENOSPC */ return ret; } else { /* bigalloc */ - ret = ext4_da_check_clu_allocated(inode, lblk, &allocated); + resv_clu = EXT4_B2C(sbi, end) - EXT4_B2C(sbi, lblk) - 1; + if (resv_clu < 0) + resv_clu = 0; + + ret = ext4_da_check_clu_allocated(inode, lblk, &lclu_allocated); if (ret < 0) return ret; - if (ret > 0) { - ret = ext4_da_reserve_space(inode, 1); + if (ret > 0) + resv_clu++; + + if (EXT4_B2C(sbi, lblk) != EXT4_B2C(sbi, end)) { + ret = ext4_da_check_clu_allocated(inode, end, + &end_allocated); + if (ret < 0) + return ret; + if (ret > 0) + resv_clu++; + } + + if (resv_clu) { + ret = ext4_da_reserve_space(inode, resv_clu); if (ret != 0) /* ENOSPC */ return ret; } } - ext4_es_insert_delayed_extent(inode, lblk, 1, allocated, false); + ext4_es_insert_delayed_extent(inode, lblk, len, lclu_allocated, + end_allocated); return 0; } @@ -1823,7 +1844,7 @@ static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map, } } - retval = ext4_insert_delayed_block(inode, map->m_lblk); + retval = ext4_insert_delayed_blocks(inode, map->m_lblk, map->m_len); up_write(&EXT4_I(inode)->i_data_sem); if (retval) return retval;