On Mon 29-04-24 20:09:46, Zhang Yi wrote:
> On 2024/4/29 17:16, Jan Kara wrote:
> > On Wed 10-04-24 11:41:59, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> >>
> >> Rename ext4_es_insert_delayed_block() to ext4_es_insert_delayed_extent()
> >> and pass length parameter to make it insert multi delalloc blocks once a
> >> time. For the case of bigalloc, expand the allocated parameter to
> >> lclu_allocated and end_allocated. lclu_allocated indicates the allocate
> >> state of the cluster which containing the lblk, end_allocated represents
> >> the end, and the middle clusters must be unallocated.
> >>
> >> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
...
> >> @@ -2112,13 +2124,22 @@ void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
> >> es2 = NULL;
> >> }
> >>
> >> - if (allocated) {
> >> - err3 = __insert_pending(inode, lblk, &pr);
> >> + if (lclu_allocated) {
> >> + err3 = __insert_pending(inode, lblk, &pr1);
> >> if (err3 != 0)
> >> goto error;
> >> - if (pr) {
> >> - __free_pending(pr);
> >> - pr = NULL;
> >> + if (pr1) {
> >> + __free_pending(pr1);
> >> + pr1 = NULL;
> >> + }
> >> + }
> >> + if (end_allocated) {
> >
> > So there's one unclear thing here: What if 'lblk' and 'end' are in the same
> > cluster? We don't want two pending reservation structures for the cluster.
> > __insert_pending() already handles this gracefully but perhaps we don't
> > need to allocate 'pr2' in that case and call __insert_pending() at all? I
> > think it could be easily handled by something like:
> >
> > if (EXT4_B2C(lblk) == EXT4_B2C(end))
> > end_allocated = false;
> >
> > at appropriate place in ext4_es_insert_delayed_extent().
> >
>
> I've done the check "EXT4_B2C(lblk) == EXT4_B2C(end)" in the caller
> ext4_insert_delayed_blocks() in patch 8, becasue there is no need to check
> the allocation state if they are in the same cluster, so it could make sure
> that end_allocated is always false when 'lblk' and 'end' are in the same
> cluster. So I suppose check and set it here again maybe redundant, how about
> add a wanging here in ext4_es_insert_delayed_extent(), like:
>
> WARN_ON_ONCE((EXT4_B2C(sbi, lblk) == EXT4_B2C(sbi, end)) &&
> end_allocated);
>
> and modify the 'lclu_allocated/end_allocated' parameter comments, note that
> end_allocated should always be set to false if the extent is in one cluster.
> Is it fine?
Yes, that is a good solution as well!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
