diff mbox series

[v2,1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock()

Message ID 20230303082158.4012809-2-yebin@huaweicloud.com
State New
Headers
Series ext4: fix WARNING in ext4_update_inline_data |

Commit Message

Ye Bin March 3, 2023, 8:21 a.m. UTC 
  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>
---
 fs/ext4/ext4.h   | 2 +-
 fs/ext4/inline.c | 7 ++++---
 fs/ext4/inode.c  | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

Comments

Jan Kara March 3, 2023, 9:55 a.m. UTC | #1 
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
>
Ye Bin March 4, 2023, 1:16 a.m. UTC | #2 
On 2023/3/3 17:55, Jan Kara wrote:
> 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
Thank you for your suggestion. It really seems to be a good idea. I will 
send new patches.
>> ---
>>   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
>>
diff mbox series

Patch

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;