Received-SPF: pass (google.com: domain of linux-ext4+bounces-2237-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 29 Apr 2024 18:27:25 +0200
From: Jan Kara <jack@suse.cz>
To: Zhang Yi <yi.zhang@huaweicloud.com>
Cc: Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, tytso@mit.edu,
	adilger.kernel@dilger.ca, yi.zhang@huawei.com,
	chengzhihao1@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH v2 5/9] ext4: make ext4_es_insert_delayed_block() insert
 multi-blocks
Message-ID: <20240429162725.rzj43hscw6to7xed@quack3>
References: <20240410034203.2188357-1-yi.zhang@huaweicloud.com>
 <20240410034203.2188357-6-yi.zhang@huaweicloud.com>
 <20240429091638.bghtdkbufbmhlw3r@quack3>
 <cf125f2c-d2f0-57f8-ee6f-9a93b9f5828d@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cf125f2c-d2f0-57f8-ee6f-9a93b9f5828d@huaweicloud.com>

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@huawei.com>
> >>
> >> 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@huawei.com>
...
> >> @@ -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@suse.com>
SUSE Labs, CR