Date:   Fri, 3 Mar 2023 10:55:36 +0100
From:   Jan Kara <jack@suse.cz>
To:     Ye Bin <yebin@huaweicloud.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        jack@suse.cz, Ye Bin <yebin10@huawei.com>
Subject: Re: [PATCH v2 1/2] ext4: introduce 'update_only' parameter for
 ext4_find_inline_data_nolock()
Message-ID: <20230303095536.7h7hlpf5v54tjpbj@quack3>
References: <20230303082158.4012809-1-yebin@huaweicloud.com>
 <20230303082158.4012809-2-yebin@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230303082158.4012809-2-yebin@huaweicloud.com>
Precedence: bulk

On Fri 03-03-23 16:21:57, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Introduce 'update_only' parameter for ext4_find_inline_data_nolock() to
> use this function to update 'inline_off' only.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Now looking at the patch maybe we could do better? The call in
ext4_write_inline_data_end() is there also only to update i_inline_off and
setting EXT4_STATE_MAY_INLINE_DATA from that call is not strictly
problematic (we are just writing new inline data so we cannot be converting
them) but not necessary either. So maybe we should instead move setting of
EXT4_STATE_MAY_INLINE_DATA into ext4_iget_extra_inode() and remove it from 
ext4_find_inline_data_nolock()? Then we won't need the 'update_only'
argument...

								Honza

> ---
>  fs/ext4/ext4.h   | 2 +-
>  fs/ext4/inline.c | 7 ++++---
>  fs/ext4/inode.c  | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 4eeb02d456a9..b073976f4bf2 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3545,7 +3545,7 @@ extern loff_t ext4_llseek(struct file *file, loff_t offset, int origin);
>  
>  /* inline.c */
>  extern int ext4_get_max_inline_size(struct inode *inode);
> -extern int ext4_find_inline_data_nolock(struct inode *inode);
> +extern int ext4_find_inline_data_nolock(struct inode *inode, bool update_only);
>  extern int ext4_init_inline_data(handle_t *handle, struct inode *inode,
>  				 unsigned int len);
>  extern int ext4_destroy_inline_data(handle_t *handle, struct inode *inode);
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 2b42ececa46d..0fa8f41de3de 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -126,7 +126,7 @@ int ext4_get_max_inline_size(struct inode *inode)
>   * currently only used in a code path coming form ext4_iget, before
>   * the new inode has been unlocked
>   */
> -int ext4_find_inline_data_nolock(struct inode *inode)
> +int ext4_find_inline_data_nolock(struct inode *inode, bool update_only)
>  {
>  	struct ext4_xattr_ibody_find is = {
>  		.s = { .not_found = -ENODATA, },
> @@ -159,7 +159,8 @@ int ext4_find_inline_data_nolock(struct inode *inode)
>  					(void *)ext4_raw_inode(&is.iloc));
>  		EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE +
>  				le32_to_cpu(is.s.here->e_value_size);
> -		ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
> +		if (!update_only)
> +			ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
>  	}
>  out:
>  	brelse(is.iloc.bh);
> @@ -761,7 +762,7 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
>  		 * ext4_write_begin() called
>  		 * ext4_try_to_write_inline_data()
>  		 */
> -		(void) ext4_find_inline_data_nolock(inode);
> +		(void) ext4_find_inline_data_nolock(inode, false);
>  
>  		kaddr = kmap_atomic(page);
>  		ext4_write_inline_data(inode, &iloc, kaddr, pos, copied);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d251d705c276..6ecaa1bef9bb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4798,7 +4798,7 @@ static inline int ext4_iget_extra_inode(struct inode *inode,
>  	if (EXT4_INODE_HAS_XATTR_SPACE(inode)  &&
>  	    *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
>  		ext4_set_inode_state(inode, EXT4_STATE_XATTR);
> -		return ext4_find_inline_data_nolock(inode);
> +		return ext4_find_inline_data_nolock(inode, false);
>  	} else
>  		EXT4_I(inode)->i_inline_off = 0;
>  	return 0;
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR