[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240807174108.l2bbbhlnpznztp34@quack3>
Date: Wed, 7 Aug 2024 19:41:08 +0200
From: Jan Kara <jack@...e.cz>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, tytso@....edu,
adilger.kernel@...ger.ca, jack@...e.cz, ritesh.list@...il.com,
yi.zhang@...wei.com, chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH v2 06/10] ext4: update delalloc data reserve spcae in
ext4_es_insert_extent()
On Fri 02-08-24 19:51:16, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@...wei.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@...wei.com>
...
> @@ -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 <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists