Received-SPF: pass (google.com: domain of linux-ext4+bounces-651-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 3 Jan 2024 10:14:49 +0100
From: Jan Kara <jack@suse.cz>
To: Kemeng Shi <shikemeng@huaweicloud.com>
Cc: tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
	linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 8/9] ext4: remove unnecessary parameter "needed" in
 ext4_discard_preallocations
Message-ID: <20240103091449.djg4tepqfbizi75h@quack3>
References: <20240103102821.448134-1-shikemeng@huaweicloud.com>
 <20240103102821.448134-9-shikemeng@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240103102821.448134-9-shikemeng@huaweicloud.com>

On Wed 03-01-24 18:28:20, Kemeng Shi wrote:
> The "needed" controls the number of ext4_prealloc_space to discard in
> ext4_discard_preallocations. Function ext4_discard_preallocations is
> supposed to discard all non-used preallocated blocks when "needed"
> is 0 and now ext4_discard_preallocations is always called with "needed"
> = 0. Remove unnecessary parameter "needed" and remove all non-used
> preallocated spaces in ext4_discard_preallocations to simplify the
> code.
> 
> Note: If count of non-used preallocated spaces could be more than
> UINT_MAX, there was a memory leak as some non-used preallocated
> spaces are left ununsed and this commit will fix it. Otherwise,
> there is no behavior change.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ext4.h        |  2 +-
>  fs/ext4/extents.c     | 10 +++++-----
>  fs/ext4/file.c        |  2 +-
>  fs/ext4/indirect.c    |  2 +-
>  fs/ext4/inode.c       |  6 +++---
>  fs/ext4/ioctl.c       |  2 +-
>  fs/ext4/mballoc.c     | 10 +++-------
>  fs/ext4/move_extent.c |  4 ++--
>  fs/ext4/super.c       |  2 +-
>  9 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index fb35cae16..2fd444034 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2905,7 +2905,7 @@ extern int ext4_mb_init(struct super_block *);
>  extern void ext4_mb_release(struct super_block *);
>  extern ext4_fsblk_t ext4_mb_new_blocks(handle_t *,
>  				struct ext4_allocation_request *, int *);
> -extern void ext4_discard_preallocations(struct inode *, unsigned int);
> +extern void ext4_discard_preallocations(struct inode *);
>  extern int __init ext4_init_mballoc(void);
>  extern void ext4_exit_mballoc(void);
>  extern ext4_group_t ext4_mb_prefetch(struct super_block *sb,
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 880f383df..fd350889a 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -100,7 +100,7 @@ static int ext4_ext_trunc_restart_fn(struct inode *inode, int *dropped)
>  	 * i_rwsem. So we can safely drop the i_data_sem here.
>  	 */
>  	BUG_ON(EXT4_JOURNAL(inode) == NULL);
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  	up_write(&EXT4_I(inode)->i_data_sem);
>  	*dropped = 1;
>  	return 0;
> @@ -4313,7 +4313,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  			 * not a good idea to call discard here directly,
>  			 * but otherwise we'd need to call it every free().
>  			 */
> -			ext4_discard_preallocations(inode, 0);
> +			ext4_discard_preallocations(inode);
>  			if (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE)
>  				fb_flags = EXT4_FREE_BLOCKS_NO_QUOT_UPDATE;
>  			ext4_free_blocks(handle, inode, NULL, newblock,
> @@ -5354,7 +5354,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
>  	ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE, handle);
>  
>  	down_write(&EXT4_I(inode)->i_data_sem);
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  	ext4_es_remove_extent(inode, punch_start, EXT_MAX_BLOCKS - punch_start);
>  
>  	ret = ext4_ext_remove_space(inode, punch_start, punch_stop - 1);
> @@ -5362,7 +5362,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
>  		up_write(&EXT4_I(inode)->i_data_sem);
>  		goto out_stop;
>  	}
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  
>  	ret = ext4_ext_shift_extents(inode, handle, punch_stop,
>  				     punch_stop - punch_start, SHIFT_LEFT);
> @@ -5494,7 +5494,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
>  		goto out_stop;
>  
>  	down_write(&EXT4_I(inode)->i_data_sem);
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  
>  	path = ext4_find_extent(inode, offset_lblk, NULL, 0);
>  	if (IS_ERR(path)) {
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 0166bb9ca..89cb28da8 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -174,7 +174,7 @@ static int ext4_release_file(struct inode *inode, struct file *filp)
>  			(atomic_read(&inode->i_writecount) == 1) &&
>  			!EXT4_I(inode)->i_reserved_data_blocks) {
>  		down_write(&EXT4_I(inode)->i_data_sem);
> -		ext4_discard_preallocations(inode, 0);
> +		ext4_discard_preallocations(inode);
>  		up_write(&EXT4_I(inode)->i_data_sem);
>  	}
>  	if (is_dx(inode) && filp->private_data)
> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index a9f371611..d8ca7f64f 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -714,7 +714,7 @@ static int ext4_ind_trunc_restart_fn(handle_t *handle, struct inode *inode,
>  	 * i_rwsem. So we can safely drop the i_data_sem here.
>  	 */
>  	BUG_ON(EXT4_JOURNAL(inode) == NULL);
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  	up_write(&EXT4_I(inode)->i_data_sem);
>  	*dropped = 1;
>  	return 0;
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d77323204..53ed4a0ad 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -371,7 +371,7 @@ void ext4_da_update_reserve_space(struct inode *inode,
>  	 */
>  	if ((ei->i_reserved_data_blocks == 0) &&
>  	    !inode_is_open_for_write(inode))
> -		ext4_discard_preallocations(inode, 0);
> +		ext4_discard_preallocations(inode);
>  }
>  
>  static int __check_block_validity(struct inode *inode, const char *func,
> @@ -4014,7 +4014,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>  	if (stop_block > first_block) {
>  
>  		down_write(&EXT4_I(inode)->i_data_sem);
> -		ext4_discard_preallocations(inode, 0);
> +		ext4_discard_preallocations(inode);
>  
>  		ext4_es_remove_extent(inode, first_block,
>  				      stop_block - first_block);
> @@ -4167,7 +4167,7 @@ int ext4_truncate(struct inode *inode)
>  
>  	down_write(&EXT4_I(inode)->i_data_sem);
>  
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  
>  	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
>  		err = ext4_ext_truncate(handle, inode);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 0bfe2ce58..2e2bcd22b 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -458,7 +458,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
>  	ext4_reset_inode_seed(inode);
>  	ext4_reset_inode_seed(inode_bl);
>  
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  
>  	err = ext4_mark_inode_dirty(handle, inode);
>  	if (err < 0) {
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index e6561a09d..0e6beb3b4 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -5469,7 +5469,7 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
>   *
>   * FIXME!! Make sure it is valid at all the call sites
>   */
> -void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
> +void ext4_discard_preallocations(struct inode *inode)
>  {
>  	struct ext4_inode_info *ei = EXT4_I(inode);
>  	struct super_block *sb = inode->i_sb;
> @@ -5491,15 +5491,12 @@ void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
>  	mb_debug(sb, "discard preallocation for inode %lu\n",
>  		 inode->i_ino);
>  	trace_ext4_discard_preallocations(inode,
> -			atomic_read(&ei->i_prealloc_active), needed);
> -
> -	if (needed == 0)
> -		needed = UINT_MAX;
> +			atomic_read(&ei->i_prealloc_active), 0);
>  
>  repeat:
>  	/* first, collect all pa's in the inode */
>  	write_lock(&ei->i_prealloc_lock);
> -	for (iter = rb_first(&ei->i_prealloc_node); iter && needed;
> +	for (iter = rb_first(&ei->i_prealloc_node); iter;
>  	     iter = rb_next(iter)) {
>  		pa = rb_entry(iter, struct ext4_prealloc_space,
>  			      pa_node.inode_node);
> @@ -5523,7 +5520,6 @@ void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
>  			spin_unlock(&pa->pa_lock);
>  			rb_erase(&pa->pa_node.inode_node, &ei->i_prealloc_node);
>  			list_add(&pa->u.pa_tmp_list, &list);
> -			needed--;
>  			continue;
>  		}
>  
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 18a9e7c47..0abfc104a 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -689,8 +689,8 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  
>  out:
>  	if (*moved_len) {
> -		ext4_discard_preallocations(orig_inode, 0);
> -		ext4_discard_preallocations(donor_inode, 0);
> +		ext4_discard_preallocations(orig_inode);
> +		ext4_discard_preallocations(donor_inode);
>  	}
>  
>  	ext4_free_ext_path(path);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index d062383ea..c0cfc3e46 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1518,7 +1518,7 @@ void ext4_clear_inode(struct inode *inode)
>  	ext4_fc_del(inode);
>  	invalidate_inode_buffers(inode);
>  	clear_inode(inode);
> -	ext4_discard_preallocations(inode, 0);
> +	ext4_discard_preallocations(inode);
>  	ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
>  	dquot_drop(inode);
>  	if (EXT4_I(inode)->jinode) {
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR