Re: [PATCH v2 06/10] ext4: update delalloc data reserve spcae in ext4_es_insert_extent()
From: Jan Kara <jack@suse.cz>
Date: 2024-08-07 17:41:12
Also in:
linux-fsdevel, lkml
On Fri 02-08-24 19:51:16, Zhang Yi wrote:
From: Zhang Yi <yi.zhang@huawei.com>
Now that we update data reserved space for delalloc after allocating
new blocks in ext4_{ind|ext}_map_blocks(), and if bigalloc feature is
enabled, we also need to query the extents_status tree to calculate the
exact reserved clusters. This is complicated now and it appears that
it's better to do this job in ext4_es_insert_extent(), because
__es_remove_extent() have already count delalloc blocks when removing
delalloc extents and __revise_pending() return new adding pending count,
we could update the reserved blocks easily in ext4_es_insert_extent().
Thers is one special case needs to concern is the quota claiming, when
bigalloc is enabled, if the delayed cluster allocation has been raced
by another no-delayed allocation(e.g. from fallocate) which doesn't
cover the delayed blocks:
|< one cluster >|
hhhhhhhhhhhhhhhhhhhdddddddddd
^ ^
|< >| < fallocate this range, don't claim quota again
We can't claim quota as usual because the fallocate has already claimed
it in ext4_mb_new_blocks(), we could notice this case through the
removed delalloc blocks count.
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>...
quoted hunk ↗ jump to hunk
@@ -926,9 +928,27 @@ void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk, __free_pending(pr); pr = NULL; } + pending = err3; } error: write_unlock(&EXT4_I(inode)->i_es_lock); + /* + * Reduce the reserved cluster count to reflect successful deferred + * allocation of delayed allocated clusters or direct allocation of + * clusters discovered to be delayed allocated. Once allocated, a + * cluster is not included in the reserved count. + * + * When bigalloc is enabled, allocating non-delayed allocated blocks + * which belong to delayed allocated clusters (from fallocate, filemap, + * DIO, or clusters allocated when delalloc has been disabled by + * ext4_nonda_switch()). Quota has been claimed by ext4_mb_new_blocks(), + * so release the quota reservations made for any previously delayed + * allocated clusters. + */ + resv_used = rinfo.delonly_cluster + pending; + if (resv_used) + ext4_da_update_reserve_space(inode, resv_used, + rinfo.delonly_block);
I'm not sure I understand here. We are inserting extent into extent status tree. We are replacing resv_used clusters worth of space with delayed allocation reservation with normally allocated clusters so we need to release the reservation (mballoc already reduced freeclusters counter). That I understand. In normal case we should also claim quota because we are converting from reserved into allocated state. Now if we allocated blocks under this range (e.g. from fallocate()) without EXT4_GET_BLOCKS_DELALLOC_RESERVE, we need to release quota reservation here instead of claiming it. But I fail to see how rinfo.delonly_block > 0 is related to whether EXT4_GET_BLOCKS_DELALLOC_RESERVE was set when allocating blocks for this extent or not. At this point it would seem much clearer if we passed flag to ext4_es_insert_extent() whether EXT4_GET_BLOCKS_DELALLOC_RESERVE was set when allocating extent or not instead of computing delonly_block and somehow infering from that. But maybe I miss some obvious reason why that is correct. Honza -- Jan Kara [off-list ref] SUSE Labs, CR