diff mbox series

f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

Message ID 20230613233940.3643362-1-jaegeuk@kernel.org
State New
Headers
Series f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue |

Commit Message

Jaegeuk Kim June 13, 2023, 11:39 p.m. UTC 
  This reverts commit 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr".

That introduced a deadlock case:

Thread #1:

[122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
    -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);

[122554.641927][   T92]  __f2fs_get_acl+0x50/0x284
[122554.641948][   T92]  f2fs_init_acl+0x84/0x54c
[122554.641969][   T92]  f2fs_init_inode_metadata+0x460/0x5f0
[122554.641990][   T92]  f2fs_add_inline_entry+0x11c/0x350
    -> Locked dir->inode_page by f2fs_get_node_page()

[122554.642009][   T92]  f2fs_do_add_link+0x100/0x1e4
[122554.642025][   T92]  f2fs_create+0xf4/0x22c
[122554.642047][   T92]  vfs_create+0x130/0x1f4

Thread #2:

[123996.386358][   T92]  __get_node_page+0x8c/0x504
    -> waiting for dir->inode_page lock

[123996.386383][   T92]  read_all_xattrs+0x11c/0x1f4
[123996.386405][   T92]  __f2fs_setxattr+0xcc/0x528
[123996.386424][   T92]  f2fs_setxattr+0x158/0x1f4
    -> f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);

[123996.386443][   T92]  __f2fs_set_acl+0x328/0x430
[123996.386618][   T92]  f2fs_set_acl+0x38/0x50
[123996.386642][   T92]  posix_acl_chmod+0xc8/0x1c8
[123996.386669][   T92]  f2fs_setattr+0x5e0/0x6bc
[123996.386689][   T92]  notify_change+0x4d8/0x580
[123996.386717][   T92]  chmod_common+0xd8/0x184
[123996.386748][   T92]  do_fchmodat+0x60/0x124
[123996.386766][   T92]  __arm64_sys_fchmodat+0x28/0x3c

Let's take a look at the original issue back.

Thread A:                                       Thread B:
-f2fs_getxattr
   -lookup_all_xattrs
      -xnid = F2FS_I(inode)->i_xattr_nid;
                                                -f2fs_setxattr
                                                    -__f2fs_setxattr
                                                        -write_all_xattrs
                                                            -truncate_xattr_node
                                                                  ...  ...
                                                -write_checkpoint
                                                                  ...  ...
                                                -alloc_nid   <- nid reuse
          -get_node_page
              -f2fs_bug_on  <- nid != node_footer->nid

I think we don't need to truncate xattr pages eagerly which introduces lots of
data races without big benefits.

Cc: <stable@vger.kernel.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h  |  1 -
 fs/f2fs/super.c |  1 -
 fs/f2fs/xattr.c | 31 ++++++++-----------------------
 3 files changed, 8 insertions(+), 25 deletions(-)

Comments

patchwork-bot+f2fs@kernel.org June 15, 2023, 6 p.m. UTC | #1 
Hello:

This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Tue, 13 Jun 2023 16:39:40 -0700 you wrote:
> This reverts commit 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr".
> 
> That introduced a deadlock case:
> 
> Thread #1:
> 
> [122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
>     -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> 
> [...]

Here is the summary with links:
  - [f2fs-dev] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue
    https://git.kernel.org/jaegeuk/f2fs/c/c1ac6e02b5fc

You are awesome, thank you!
Chao Yu June 25, 2023, 7:26 a.m. UTC | #2 
On 2023/6/14 7:39, Jaegeuk Kim wrote:
> This reverts commit 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr".
> 
> That introduced a deadlock case:
> 
> Thread #1:
> 
> [122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
>      -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> 
> [122554.641927][   T92]  __f2fs_get_acl+0x50/0x284
> [122554.641948][   T92]  f2fs_init_acl+0x84/0x54c
> [122554.641969][   T92]  f2fs_init_inode_metadata+0x460/0x5f0
> [122554.641990][   T92]  f2fs_add_inline_entry+0x11c/0x350
>      -> Locked dir->inode_page by f2fs_get_node_page()
> 
> [122554.642009][   T92]  f2fs_do_add_link+0x100/0x1e4
> [122554.642025][   T92]  f2fs_create+0xf4/0x22c
> [122554.642047][   T92]  vfs_create+0x130/0x1f4
> 
> Thread #2:
> 
> [123996.386358][   T92]  __get_node_page+0x8c/0x504
>      -> waiting for dir->inode_page lock
> 
> [123996.386383][   T92]  read_all_xattrs+0x11c/0x1f4
> [123996.386405][   T92]  __f2fs_setxattr+0xcc/0x528
> [123996.386424][   T92]  f2fs_setxattr+0x158/0x1f4
>      -> f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
> 
> [123996.386443][   T92]  __f2fs_set_acl+0x328/0x430
> [123996.386618][   T92]  f2fs_set_acl+0x38/0x50
> [123996.386642][   T92]  posix_acl_chmod+0xc8/0x1c8
> [123996.386669][   T92]  f2fs_setattr+0x5e0/0x6bc
> [123996.386689][   T92]  notify_change+0x4d8/0x580
> [123996.386717][   T92]  chmod_common+0xd8/0x184
> [123996.386748][   T92]  do_fchmodat+0x60/0x124
> [123996.386766][   T92]  __arm64_sys_fchmodat+0x28/0x3c
> 
> Let's take a look at the original issue back.
> 
> Thread A:                                       Thread B:
> -f2fs_getxattr
>     -lookup_all_xattrs
>        -xnid = F2FS_I(inode)->i_xattr_nid;
>                                                  -f2fs_setxattr
>                                                      -__f2fs_setxattr
>                                                          -write_all_xattrs
>                                                              -truncate_xattr_node
>                                                                    ...  ...
>                                                  -write_checkpoint
>                                                                    ...  ...
>                                                  -alloc_nid   <- nid reuse
>            -get_node_page
>                -f2fs_bug_on  <- nid != node_footer->nid

One concern below:

Thread A:					Thread B:
- f2fs_getxattr
  - lookup_all_xattrs
   - read_inline_xattr
    - f2fs_get_node_page(ino)
    - memcpy inline xattr
    - f2fs_put_page
						- f2fs_setxattr
						 - __f2fs_setxattr
						  - __f2fs_setxattr
						   - write_all_xattrs
						    - write xnode and inode
   ---> inline xattr may out of update here.
   - read_xattr_block
    - f2fs_get_node_page(xnid)
    - memcpy xnode xattr
    - f2fs_put_page

Do we need to keep xattr_{get,set} being atomical operation?

Thanks,

> 
> I think we don't need to truncate xattr pages eagerly which introduces lots of
> data races without big benefits.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>   fs/f2fs/f2fs.h  |  1 -
>   fs/f2fs/super.c |  1 -
>   fs/f2fs/xattr.c | 31 ++++++++-----------------------
>   3 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 3f5b161dd743..7b9af2d51656 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -838,7 +838,6 @@ struct f2fs_inode_info {
>   
>   	/* avoid racing between foreground op and gc */
>   	struct f2fs_rwsem i_gc_rwsem[2];
> -	struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */
>   
>   	int i_extra_isize;		/* size of extra space located in i_addr */
>   	kprojid_t i_projid;		/* id for project quota */
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 1b2c788ed80d..c917fa771f0e 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>   	INIT_LIST_HEAD(&fi->gdirty_list);
>   	init_f2fs_rwsem(&fi->i_gc_rwsem[READ]);
>   	init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
> -	init_f2fs_rwsem(&fi->i_xattr_sem);
>   
>   	/* Will be used by directory only */
>   	fi->i_dir_level = F2FS_SB(sb)->dir_level;
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 213805d3592c..bdc8a55085a2 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -433,7 +433,7 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>   {
>   	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   	size_t inline_size = inline_xattr_size(inode);
> -	struct page *in_page = NULL;
> +	struct page *in_page = ipage;
>   	void *xattr_addr;
>   	void *inline_addr = NULL;
>   	struct page *xpage;
> @@ -446,29 +446,19 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>   
>   	/* write to inline xattr */
>   	if (inline_size) {
> -		if (ipage) {
> -			inline_addr = inline_xattr_addr(inode, ipage);
> -		} else {
> +		if (!in_page) {
>   			in_page = f2fs_get_node_page(sbi, inode->i_ino);
>   			if (IS_ERR(in_page)) {
>   				f2fs_alloc_nid_failed(sbi, new_nid);
>   				return PTR_ERR(in_page);
>   			}
> -			inline_addr = inline_xattr_addr(inode, in_page);
>   		}
> +		inline_addr = inline_xattr_addr(inode, in_page);
>   
> -		f2fs_wait_on_page_writeback(ipage ? ipage : in_page,
> -							NODE, true, true);
> -		/* no need to use xattr node block */
> +		f2fs_wait_on_page_writeback(in_page, NODE, true, true);
>   		if (hsize <= inline_size) {
> -			err = f2fs_truncate_xattr_node(inode);
> -			f2fs_alloc_nid_failed(sbi, new_nid);
> -			if (err) {
> -				f2fs_put_page(in_page, 1);
> -				return err;
> -			}
>   			memcpy(inline_addr, txattr_addr, inline_size);
> -			set_page_dirty(ipage ? ipage : in_page);
> +			set_page_dirty(in_page);
>   			goto in_page_out;
>   		}
>   	}
> @@ -502,12 +492,13 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>   	memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE);
>   
>   	if (inline_size)
> -		set_page_dirty(ipage ? ipage : in_page);
> +		set_page_dirty(in_page);
>   	set_page_dirty(xpage);
>   
>   	f2fs_put_page(xpage, 1);
>   in_page_out:
> -	f2fs_put_page(in_page, 1);
> +	if (in_page != ipage)
> +		f2fs_put_page(in_page, 1);
>   	return err;
>   }
>   
> @@ -528,10 +519,8 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>   	if (len > F2FS_NAME_LEN)
>   		return -ERANGE;
>   
> -	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>   	error = lookup_all_xattrs(inode, ipage, index, len, name,
>   				&entry, &base_addr, &base_size, &is_inline);
> -	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>   	if (error)
>   		return error;
>   
> @@ -565,9 +554,7 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>   	int error;
>   	size_t rest = buffer_size;
>   
> -	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>   	error = read_all_xattrs(inode, NULL, &base_addr);
> -	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>   	if (error)
>   		return error;
>   
> @@ -794,9 +781,7 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name,
>   	f2fs_balance_fs(sbi, true);
>   
>   	f2fs_lock_op(sbi);
> -	f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
>   	err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags);
> -	f2fs_up_write(&F2FS_I(inode)->i_xattr_sem);
>   	f2fs_unlock_op(sbi);
>   
>   	f2fs_update_time(sbi, REQ_TIME);
Chao Yu June 25, 2023, 10:27 a.m. UTC | #3 
On 2023/6/25 15:26, Chao Yu wrote:
> One concern below:
> 
> Thread A:                    Thread B:
> - f2fs_getxattr
>   - lookup_all_xattrs
>    - read_inline_xattr
>     - f2fs_get_node_page(ino)
>     - memcpy inline xattr
>     - f2fs_put_page
>                          - f2fs_setxattr
>                           - __f2fs_setxattr
>                            - __f2fs_setxattr
>                             - write_all_xattrs
>                              - write xnode and inode
>    ---> inline xattr may out of update here.
>    - read_xattr_block
>     - f2fs_get_node_page(xnid)
>     - memcpy xnode xattr
>     - f2fs_put_page
> 
> Do we need to keep xattr_{get,set} being atomical operation?

It seems xfstest starts to complain w/ below message...

[ 3400.856443] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
[ 3400.864042] F2FS-fs (vdc): inode (1595) has invalid last xattr entry, entry_size: 26580
[ 3400.865764] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
[ 3400.880067] F2FS-fs (vdc): inode (9839) has corrupted xattr
[ 3400.880714] F2FS-fs (vdc): inode (10855) has corrupted xattr

Thanks,

> 
> Thanks,
> 
>>
>> I think we don't need to truncate xattr pages eagerly which introduces lots of
>> data races without big benefits.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>> ---
>>   fs/f2fs/f2fs.h  |  1 -
>>   fs/f2fs/super.c |  1 -
>>   fs/f2fs/xattr.c | 31 ++++++++-----------------------
>>   3 files changed, 8 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 3f5b161dd743..7b9af2d51656 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -838,7 +838,6 @@ struct f2fs_inode_info {
>>       /* avoid racing between foreground op and gc */
>>       struct f2fs_rwsem i_gc_rwsem[2];
>> -    struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */
>>       int i_extra_isize;        /* size of extra space located in i_addr */
>>       kprojid_t i_projid;        /* id for project quota */
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 1b2c788ed80d..c917fa771f0e 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>>       INIT_LIST_HEAD(&fi->gdirty_list);
>>       init_f2fs_rwsem(&fi->i_gc_rwsem[READ]);
>>       init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
>> -    init_f2fs_rwsem(&fi->i_xattr_sem);
>>       /* Will be used by directory only */
>>       fi->i_dir_level = F2FS_SB(sb)->dir_level;
>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>> index 213805d3592c..bdc8a55085a2 100644
>> --- a/fs/f2fs/xattr.c
>> +++ b/fs/f2fs/xattr.c
>> @@ -433,7 +433,7 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>>   {
>>       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>       size_t inline_size = inline_xattr_size(inode);
>> -    struct page *in_page = NULL;
>> +    struct page *in_page = ipage;
>>       void *xattr_addr;
>>       void *inline_addr = NULL;
>>       struct page *xpage;
>> @@ -446,29 +446,19 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>>       /* write to inline xattr */
>>       if (inline_size) {
>> -        if (ipage) {
>> -            inline_addr = inline_xattr_addr(inode, ipage);
>> -        } else {
>> +        if (!in_page) {
>>               in_page = f2fs_get_node_page(sbi, inode->i_ino);
>>               if (IS_ERR(in_page)) {
>>                   f2fs_alloc_nid_failed(sbi, new_nid);
>>                   return PTR_ERR(in_page);
>>               }
>> -            inline_addr = inline_xattr_addr(inode, in_page);
>>           }
>> +        inline_addr = inline_xattr_addr(inode, in_page);
>> -        f2fs_wait_on_page_writeback(ipage ? ipage : in_page,
>> -                            NODE, true, true);
>> -        /* no need to use xattr node block */
>> +        f2fs_wait_on_page_writeback(in_page, NODE, true, true);
>>           if (hsize <= inline_size) {
>> -            err = f2fs_truncate_xattr_node(inode);
>> -            f2fs_alloc_nid_failed(sbi, new_nid);
>> -            if (err) {
>> -                f2fs_put_page(in_page, 1);
>> -                return err;
>> -            }
>>               memcpy(inline_addr, txattr_addr, inline_size);
>> -            set_page_dirty(ipage ? ipage : in_page);
>> +            set_page_dirty(in_page);
>>               goto in_page_out;
>>           }
>>       }
>> @@ -502,12 +492,13 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>>       memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE);
>>       if (inline_size)
>> -        set_page_dirty(ipage ? ipage : in_page);
>> +        set_page_dirty(in_page);
>>       set_page_dirty(xpage);
>>       f2fs_put_page(xpage, 1);
>>   in_page_out:
>> -    f2fs_put_page(in_page, 1);
>> +    if (in_page != ipage)
>> +        f2fs_put_page(in_page, 1);
>>       return err;
>>   }
>> @@ -528,10 +519,8 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>>       if (len > F2FS_NAME_LEN)
>>           return -ERANGE;
>> -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>>       error = lookup_all_xattrs(inode, ipage, index, len, name,
>>                   &entry, &base_addr, &base_size, &is_inline);
>> -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>>       if (error)
>>           return error;
>> @@ -565,9 +554,7 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>>       int error;
>>       size_t rest = buffer_size;
>> -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>>       error = read_all_xattrs(inode, NULL, &base_addr);
>> -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>>       if (error)
>>           return error;
>> @@ -794,9 +781,7 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name,
>>       f2fs_balance_fs(sbi, true);
>>       f2fs_lock_op(sbi);
>> -    f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
>>       err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags);
>> -    f2fs_up_write(&F2FS_I(inode)->i_xattr_sem);
>>       f2fs_unlock_op(sbi);
>>       f2fs_update_time(sbi, REQ_TIME);
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Jaegeuk Kim June 26, 2023, 1:11 p.m. UTC | #4 
On 06/25, Chao Yu wrote:
> On 2023/6/25 15:26, Chao Yu wrote:
> > One concern below:
> > 
> > Thread A:                    Thread B:
> > - f2fs_getxattr
> >   - lookup_all_xattrs
> >    - read_inline_xattr
> >     - f2fs_get_node_page(ino)
> >     - memcpy inline xattr
> >     - f2fs_put_page
> >                          - f2fs_setxattr
> >                           - __f2fs_setxattr
> >                            - __f2fs_setxattr
> >                             - write_all_xattrs
> >                              - write xnode and inode
> >    ---> inline xattr may out of update here.
> >    - read_xattr_block
> >     - f2fs_get_node_page(xnid)
> >     - memcpy xnode xattr
> >     - f2fs_put_page
> > 
> > Do we need to keep xattr_{get,set} being atomical operation?
> 
> It seems xfstest starts to complain w/ below message...

I don't see any failure. Which test do you see?

> 
> [ 3400.856443] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
> [ 3400.864042] F2FS-fs (vdc): inode (1595) has invalid last xattr entry, entry_size: 26580
> [ 3400.865764] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
> [ 3400.880067] F2FS-fs (vdc): inode (9839) has corrupted xattr
> [ 3400.880714] F2FS-fs (vdc): inode (10855) has corrupted xattr
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > > 
> > > I think we don't need to truncate xattr pages eagerly which introduces lots of
> > > data races without big benefits.
> > > 
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >   fs/f2fs/f2fs.h  |  1 -
> > >   fs/f2fs/super.c |  1 -
> > >   fs/f2fs/xattr.c | 31 ++++++++-----------------------
> > >   3 files changed, 8 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 3f5b161dd743..7b9af2d51656 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -838,7 +838,6 @@ struct f2fs_inode_info {
> > >       /* avoid racing between foreground op and gc */
> > >       struct f2fs_rwsem i_gc_rwsem[2];
> > > -    struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */
> > >       int i_extra_isize;        /* size of extra space located in i_addr */
> > >       kprojid_t i_projid;        /* id for project quota */
> > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > index 1b2c788ed80d..c917fa771f0e 100644
> > > --- a/fs/f2fs/super.c
> > > +++ b/fs/f2fs/super.c
> > > @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
> > >       INIT_LIST_HEAD(&fi->gdirty_list);
> > >       init_f2fs_rwsem(&fi->i_gc_rwsem[READ]);
> > >       init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
> > > -    init_f2fs_rwsem(&fi->i_xattr_sem);
> > >       /* Will be used by directory only */
> > >       fi->i_dir_level = F2FS_SB(sb)->dir_level;
> > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > > index 213805d3592c..bdc8a55085a2 100644
> > > --- a/fs/f2fs/xattr.c
> > > +++ b/fs/f2fs/xattr.c
> > > @@ -433,7 +433,7 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
> > >   {
> > >       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > >       size_t inline_size = inline_xattr_size(inode);
> > > -    struct page *in_page = NULL;
> > > +    struct page *in_page = ipage;
> > >       void *xattr_addr;
> > >       void *inline_addr = NULL;
> > >       struct page *xpage;
> > > @@ -446,29 +446,19 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
> > >       /* write to inline xattr */
> > >       if (inline_size) {
> > > -        if (ipage) {
> > > -            inline_addr = inline_xattr_addr(inode, ipage);
> > > -        } else {
> > > +        if (!in_page) {
> > >               in_page = f2fs_get_node_page(sbi, inode->i_ino);
> > >               if (IS_ERR(in_page)) {
> > >                   f2fs_alloc_nid_failed(sbi, new_nid);
> > >                   return PTR_ERR(in_page);
> > >               }
> > > -            inline_addr = inline_xattr_addr(inode, in_page);
> > >           }
> > > +        inline_addr = inline_xattr_addr(inode, in_page);
> > > -        f2fs_wait_on_page_writeback(ipage ? ipage : in_page,
> > > -                            NODE, true, true);
> > > -        /* no need to use xattr node block */
> > > +        f2fs_wait_on_page_writeback(in_page, NODE, true, true);
> > >           if (hsize <= inline_size) {
> > > -            err = f2fs_truncate_xattr_node(inode);
> > > -            f2fs_alloc_nid_failed(sbi, new_nid);
> > > -            if (err) {
> > > -                f2fs_put_page(in_page, 1);
> > > -                return err;
> > > -            }
> > >               memcpy(inline_addr, txattr_addr, inline_size);
> > > -            set_page_dirty(ipage ? ipage : in_page);
> > > +            set_page_dirty(in_page);
> > >               goto in_page_out;
> > >           }
> > >       }
> > > @@ -502,12 +492,13 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
> > >       memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE);
> > >       if (inline_size)
> > > -        set_page_dirty(ipage ? ipage : in_page);
> > > +        set_page_dirty(in_page);
> > >       set_page_dirty(xpage);
> > >       f2fs_put_page(xpage, 1);
> > >   in_page_out:
> > > -    f2fs_put_page(in_page, 1);
> > > +    if (in_page != ipage)
> > > +        f2fs_put_page(in_page, 1);
> > >       return err;
> > >   }
> > > @@ -528,10 +519,8 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
> > >       if (len > F2FS_NAME_LEN)
> > >           return -ERANGE;
> > > -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > >       error = lookup_all_xattrs(inode, ipage, index, len, name,
> > >                   &entry, &base_addr, &base_size, &is_inline);
> > > -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> > >       if (error)
> > >           return error;
> > > @@ -565,9 +554,7 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
> > >       int error;
> > >       size_t rest = buffer_size;
> > > -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > >       error = read_all_xattrs(inode, NULL, &base_addr);
> > > -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> > >       if (error)
> > >           return error;
> > > @@ -794,9 +781,7 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name,
> > >       f2fs_balance_fs(sbi, true);
> > >       f2fs_lock_op(sbi);
> > > -    f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
> > >       err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags);
> > > -    f2fs_up_write(&F2FS_I(inode)->i_xattr_sem);
> > >       f2fs_unlock_op(sbi);
> > >       f2fs_update_time(sbi, REQ_TIME);
> > 
> > 
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Chao Yu June 27, 2023, 1:37 p.m. UTC | #5 
On 2023/6/26 21:11, Jaegeuk Kim wrote:
> On 06/25, Chao Yu wrote:
>> On 2023/6/25 15:26, Chao Yu wrote:
>>> One concern below:
>>>
>>> Thread A:                    Thread B:
>>> - f2fs_getxattr
>>>    - lookup_all_xattrs
>>>     - read_inline_xattr
>>>      - f2fs_get_node_page(ino)
>>>      - memcpy inline xattr
>>>      - f2fs_put_page
>>>                           - f2fs_setxattr
>>>                            - __f2fs_setxattr
>>>                             - __f2fs_setxattr
>>>                              - write_all_xattrs
>>>                               - write xnode and inode
>>>     ---> inline xattr may out of update here.
>>>     - read_xattr_block
>>>      - f2fs_get_node_page(xnid)
>>>      - memcpy xnode xattr
>>>      - f2fs_put_page
>>>
>>> Do we need to keep xattr_{get,set} being atomical operation?
>>
>> It seems xfstest starts to complain w/ below message...
> 
> I don't see any failure. Which test do you see?

051, 083, ... 467, 642

Testcase doesn't fail, but kernel log shows inode has corrupted xattr.

> 
>>
>> [ 3400.856443] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
>> [ 3400.864042] F2FS-fs (vdc): inode (1595) has invalid last xattr entry, entry_size: 26580
>> [ 3400.865764] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
>> [ 3400.880067] F2FS-fs (vdc): inode (9839) has corrupted xattr
>> [ 3400.880714] F2FS-fs (vdc): inode (10855) has corrupted xattr
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>> I think we don't need to truncate xattr pages eagerly which introduces lots of
>>>> data races without big benefits.
>>>>
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>> ---
>>>>    fs/f2fs/f2fs.h  |  1 -
>>>>    fs/f2fs/super.c |  1 -
>>>>    fs/f2fs/xattr.c | 31 ++++++++-----------------------
>>>>    3 files changed, 8 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index 3f5b161dd743..7b9af2d51656 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -838,7 +838,6 @@ struct f2fs_inode_info {
>>>>        /* avoid racing between foreground op and gc */
>>>>        struct f2fs_rwsem i_gc_rwsem[2];
>>>> -    struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */
>>>>        int i_extra_isize;        /* size of extra space located in i_addr */
>>>>        kprojid_t i_projid;        /* id for project quota */
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index 1b2c788ed80d..c917fa771f0e 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>>>>        INIT_LIST_HEAD(&fi->gdirty_list);
>>>>        init_f2fs_rwsem(&fi->i_gc_rwsem[READ]);
>>>>        init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
>>>> -    init_f2fs_rwsem(&fi->i_xattr_sem);
>>>>        /* Will be used by directory only */
>>>>        fi->i_dir_level = F2FS_SB(sb)->dir_level;
>>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>>>> index 213805d3592c..bdc8a55085a2 100644
>>>> --- a/fs/f2fs/xattr.c
>>>> +++ b/fs/f2fs/xattr.c
>>>> @@ -433,7 +433,7 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>>>>    {
>>>>        struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>        size_t inline_size = inline_xattr_size(inode);
>>>> -    struct page *in_page = NULL;
>>>> +    struct page *in_page = ipage;
>>>>        void *xattr_addr;
>>>>        void *inline_addr = NULL;
>>>>        struct page *xpage;
>>>> @@ -446,29 +446,19 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>>>>        /* write to inline xattr */
>>>>        if (inline_size) {
>>>> -        if (ipage) {
>>>> -            inline_addr = inline_xattr_addr(inode, ipage);
>>>> -        } else {
>>>> +        if (!in_page) {
>>>>                in_page = f2fs_get_node_page(sbi, inode->i_ino);
>>>>                if (IS_ERR(in_page)) {
>>>>                    f2fs_alloc_nid_failed(sbi, new_nid);
>>>>                    return PTR_ERR(in_page);
>>>>                }
>>>> -            inline_addr = inline_xattr_addr(inode, in_page);
>>>>            }
>>>> +        inline_addr = inline_xattr_addr(inode, in_page);
>>>> -        f2fs_wait_on_page_writeback(ipage ? ipage : in_page,
>>>> -                            NODE, true, true);
>>>> -        /* no need to use xattr node block */
>>>> +        f2fs_wait_on_page_writeback(in_page, NODE, true, true);
>>>>            if (hsize <= inline_size) {
>>>> -            err = f2fs_truncate_xattr_node(inode);
>>>> -            f2fs_alloc_nid_failed(sbi, new_nid);
>>>> -            if (err) {
>>>> -                f2fs_put_page(in_page, 1);
>>>> -                return err;
>>>> -            }
>>>>                memcpy(inline_addr, txattr_addr, inline_size);
>>>> -            set_page_dirty(ipage ? ipage : in_page);
>>>> +            set_page_dirty(in_page);
>>>>                goto in_page_out;
>>>>            }
>>>>        }
>>>> @@ -502,12 +492,13 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>>>>        memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE);
>>>>        if (inline_size)
>>>> -        set_page_dirty(ipage ? ipage : in_page);
>>>> +        set_page_dirty(in_page);
>>>>        set_page_dirty(xpage);
>>>>        f2fs_put_page(xpage, 1);
>>>>    in_page_out:
>>>> -    f2fs_put_page(in_page, 1);
>>>> +    if (in_page != ipage)
>>>> +        f2fs_put_page(in_page, 1);
>>>>        return err;
>>>>    }
>>>> @@ -528,10 +519,8 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>>>>        if (len > F2FS_NAME_LEN)
>>>>            return -ERANGE;
>>>> -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>>>>        error = lookup_all_xattrs(inode, ipage, index, len, name,
>>>>                    &entry, &base_addr, &base_size, &is_inline);
>>>> -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>>>>        if (error)
>>>>            return error;
>>>> @@ -565,9 +554,7 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>>>>        int error;
>>>>        size_t rest = buffer_size;
>>>> -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>>>>        error = read_all_xattrs(inode, NULL, &base_addr);
>>>> -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>>>>        if (error)
>>>>            return error;
>>>> @@ -794,9 +781,7 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name,
>>>>        f2fs_balance_fs(sbi, true);
>>>>        f2fs_lock_op(sbi);
>>>> -    f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
>>>>        err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags);
>>>> -    f2fs_up_write(&F2FS_I(inode)->i_xattr_sem);
>>>>        f2fs_unlock_op(sbi);
>>>>        f2fs_update_time(sbi, REQ_TIME);
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Jaegeuk Kim June 28, 2023, 8:07 a.m. UTC | #6 
On 06/27, Chao Yu wrote:
> On 2023/6/26 21:11, Jaegeuk Kim wrote:
> > On 06/25, Chao Yu wrote:
> > > On 2023/6/25 15:26, Chao Yu wrote:
> > > > One concern below:
> > > > 
> > > > Thread A:                    Thread B:
> > > > - f2fs_getxattr
> > > >    - lookup_all_xattrs
> > > >     - read_inline_xattr
> > > >      - f2fs_get_node_page(ino)
> > > >      - memcpy inline xattr
> > > >      - f2fs_put_page
> > > >                           - f2fs_setxattr
> > > >                            - __f2fs_setxattr
> > > >                             - __f2fs_setxattr
> > > >                              - write_all_xattrs
> > > >                               - write xnode and inode
> > > >     ---> inline xattr may out of update here.
> > > >     - read_xattr_block
> > > >      - f2fs_get_node_page(xnid)
> > > >      - memcpy xnode xattr
> > > >      - f2fs_put_page
> > > > 
> > > > Do we need to keep xattr_{get,set} being atomical operation?
> > > 
> > > It seems xfstest starts to complain w/ below message...
> > 
> > I don't see any failure. Which test do you see?
> 
> 051, 083, ... 467, 642
> 
> Testcase doesn't fail, but kernel log shows inode has corrupted xattr.

I got it. It seems I had to fix the above issue only while keeping the sem. :(

> 
> > 
> > > 
> > > [ 3400.856443] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
> > > [ 3400.864042] F2FS-fs (vdc): inode (1595) has invalid last xattr entry, entry_size: 26580
> > > [ 3400.865764] F2FS-fs (vdc): inode (2187) has invalid last xattr entry, entry_size: 21468
> > > [ 3400.880067] F2FS-fs (vdc): inode (9839) has corrupted xattr
> > > [ 3400.880714] F2FS-fs (vdc): inode (10855) has corrupted xattr
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > I think we don't need to truncate xattr pages eagerly which introduces lots of
> > > > > data races without big benefits.
> > > > > 
> > > > > Cc: <stable@vger.kernel.org>
> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > > ---
> > > > >    fs/f2fs/f2fs.h  |  1 -
> > > > >    fs/f2fs/super.c |  1 -
> > > > >    fs/f2fs/xattr.c | 31 ++++++++-----------------------
> > > > >    3 files changed, 8 insertions(+), 25 deletions(-)
> > > > > 
> > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > index 3f5b161dd743..7b9af2d51656 100644
> > > > > --- a/fs/f2fs/f2fs.h
> > > > > +++ b/fs/f2fs/f2fs.h
> > > > > @@ -838,7 +838,6 @@ struct f2fs_inode_info {
> > > > >        /* avoid racing between foreground op and gc */
> > > > >        struct f2fs_rwsem i_gc_rwsem[2];
> > > > > -    struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */
> > > > >        int i_extra_isize;        /* size of extra space located in i_addr */
> > > > >        kprojid_t i_projid;        /* id for project quota */
> > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > > index 1b2c788ed80d..c917fa771f0e 100644
> > > > > --- a/fs/f2fs/super.c
> > > > > +++ b/fs/f2fs/super.c
> > > > > @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
> > > > >        INIT_LIST_HEAD(&fi->gdirty_list);
> > > > >        init_f2fs_rwsem(&fi->i_gc_rwsem[READ]);
> > > > >        init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
> > > > > -    init_f2fs_rwsem(&fi->i_xattr_sem);
> > > > >        /* Will be used by directory only */
> > > > >        fi->i_dir_level = F2FS_SB(sb)->dir_level;
> > > > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > > > > index 213805d3592c..bdc8a55085a2 100644
> > > > > --- a/fs/f2fs/xattr.c
> > > > > +++ b/fs/f2fs/xattr.c
> > > > > @@ -433,7 +433,7 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
> > > > >    {
> > > > >        struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > > > >        size_t inline_size = inline_xattr_size(inode);
> > > > > -    struct page *in_page = NULL;
> > > > > +    struct page *in_page = ipage;
> > > > >        void *xattr_addr;
> > > > >        void *inline_addr = NULL;
> > > > >        struct page *xpage;
> > > > > @@ -446,29 +446,19 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
> > > > >        /* write to inline xattr */
> > > > >        if (inline_size) {
> > > > > -        if (ipage) {
> > > > > -            inline_addr = inline_xattr_addr(inode, ipage);
> > > > > -        } else {
> > > > > +        if (!in_page) {
> > > > >                in_page = f2fs_get_node_page(sbi, inode->i_ino);
> > > > >                if (IS_ERR(in_page)) {
> > > > >                    f2fs_alloc_nid_failed(sbi, new_nid);
> > > > >                    return PTR_ERR(in_page);
> > > > >                }
> > > > > -            inline_addr = inline_xattr_addr(inode, in_page);
> > > > >            }
> > > > > +        inline_addr = inline_xattr_addr(inode, in_page);
> > > > > -        f2fs_wait_on_page_writeback(ipage ? ipage : in_page,
> > > > > -                            NODE, true, true);
> > > > > -        /* no need to use xattr node block */
> > > > > +        f2fs_wait_on_page_writeback(in_page, NODE, true, true);
> > > > >            if (hsize <= inline_size) {
> > > > > -            err = f2fs_truncate_xattr_node(inode);
> > > > > -            f2fs_alloc_nid_failed(sbi, new_nid);
> > > > > -            if (err) {
> > > > > -                f2fs_put_page(in_page, 1);
> > > > > -                return err;
> > > > > -            }
> > > > >                memcpy(inline_addr, txattr_addr, inline_size);
> > > > > -            set_page_dirty(ipage ? ipage : in_page);
> > > > > +            set_page_dirty(in_page);
> > > > >                goto in_page_out;
> > > > >            }
> > > > >        }
> > > > > @@ -502,12 +492,13 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
> > > > >        memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE);
> > > > >        if (inline_size)
> > > > > -        set_page_dirty(ipage ? ipage : in_page);
> > > > > +        set_page_dirty(in_page);
> > > > >        set_page_dirty(xpage);
> > > > >        f2fs_put_page(xpage, 1);
> > > > >    in_page_out:
> > > > > -    f2fs_put_page(in_page, 1);
> > > > > +    if (in_page != ipage)
> > > > > +        f2fs_put_page(in_page, 1);
> > > > >        return err;
> > > > >    }
> > > > > @@ -528,10 +519,8 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
> > > > >        if (len > F2FS_NAME_LEN)
> > > > >            return -ERANGE;
> > > > > -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > > > >        error = lookup_all_xattrs(inode, ipage, index, len, name,
> > > > >                    &entry, &base_addr, &base_size, &is_inline);
> > > > > -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> > > > >        if (error)
> > > > >            return error;
> > > > > @@ -565,9 +554,7 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
> > > > >        int error;
> > > > >        size_t rest = buffer_size;
> > > > > -    f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > > > >        error = read_all_xattrs(inode, NULL, &base_addr);
> > > > > -    f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> > > > >        if (error)
> > > > >            return error;
> > > > > @@ -794,9 +781,7 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name,
> > > > >        f2fs_balance_fs(sbi, true);
> > > > >        f2fs_lock_op(sbi);
> > > > > -    f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
> > > > >        err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags);
> > > > > -    f2fs_up_write(&F2FS_I(inode)->i_xattr_sem);
> > > > >        f2fs_unlock_op(sbi);
> > > > >        f2fs_update_time(sbi, REQ_TIME);
> > > > 
> > > > 
> > > > _______________________________________________
> > > > Linux-f2fs-devel mailing list
> > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Jaegeuk Kim June 28, 2023, 8:08 a.m. UTC | #7 
Thread #1:

[122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
    -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);

[122554.641927][   T92]  __f2fs_get_acl+0x50/0x284
[122554.641948][   T92]  f2fs_init_acl+0x84/0x54c
[122554.641969][   T92]  f2fs_init_inode_metadata+0x460/0x5f0
[122554.641990][   T92]  f2fs_add_inline_entry+0x11c/0x350
    -> Locked dir->inode_page by f2fs_get_node_page()

[122554.642009][   T92]  f2fs_do_add_link+0x100/0x1e4
[122554.642025][   T92]  f2fs_create+0xf4/0x22c
[122554.642047][   T92]  vfs_create+0x130/0x1f4

Thread #2:

[123996.386358][   T92]  __get_node_page+0x8c/0x504
    -> waiting for dir->inode_page lock

[123996.386383][   T92]  read_all_xattrs+0x11c/0x1f4
[123996.386405][   T92]  __f2fs_setxattr+0xcc/0x528
[123996.386424][   T92]  f2fs_setxattr+0x158/0x1f4
    -> f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);

[123996.386443][   T92]  __f2fs_set_acl+0x328/0x430
[123996.386618][   T92]  f2fs_set_acl+0x38/0x50
[123996.386642][   T92]  posix_acl_chmod+0xc8/0x1c8
[123996.386669][   T92]  f2fs_setattr+0x5e0/0x6bc
[123996.386689][   T92]  notify_change+0x4d8/0x580
[123996.386717][   T92]  chmod_common+0xd8/0x184
[123996.386748][   T92]  do_fchmodat+0x60/0x124
[123996.386766][   T92]  __arm64_sys_fchmodat+0x28/0x3c

Fixes: 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr"
Cc: <stable@vger.kernel.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c   | 9 ++++++++-
 fs/f2fs/xattr.c | 6 ++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 887e55988450..d635c58cf5a3 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -775,8 +775,15 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname,
 {
 	int err = -EAGAIN;
 
-	if (f2fs_has_inline_dentry(dir))
+	if (f2fs_has_inline_dentry(dir)) {
+		/*
+		 * Should get i_xattr_sem to keep the lock order:
+		 * i_xattr_sem -> inode_page lock used by f2fs_setxattr.
+		 */
+		f2fs_down_read(&F2FS_I(dir)->i_xattr_sem);
 		err = f2fs_add_inline_entry(dir, fname, inode, ino, mode);
+		f2fs_up_read(&F2FS_I(dir)->i_xattr_sem);
+	}
 	if (err == -EAGAIN)
 		err = f2fs_add_regular_entry(dir, fname, inode, ino, mode);
 
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 213805d3592c..476b186b90a6 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -528,10 +528,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
 	if (len > F2FS_NAME_LEN)
 		return -ERANGE;
 
-	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
+	if (!ipage)
+		f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
 	error = lookup_all_xattrs(inode, ipage, index, len, name,
 				&entry, &base_addr, &base_size, &is_inline);
-	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
+	if (!ipage)
+		f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
 	if (error)
 		return error;
Chao Yu June 28, 2023, 8:36 a.m. UTC | #8 
On 2023/6/28 16:08, Jaegeuk Kim wrote:
> Thread #1:
> 
> [122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
>      -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> 
> [122554.641927][   T92]  __f2fs_get_acl+0x50/0x284
> [122554.641948][   T92]  f2fs_init_acl+0x84/0x54c
> [122554.641969][   T92]  f2fs_init_inode_metadata+0x460/0x5f0
> [122554.641990][   T92]  f2fs_add_inline_entry+0x11c/0x350
>      -> Locked dir->inode_page by f2fs_get_node_page()
> 
> [122554.642009][   T92]  f2fs_do_add_link+0x100/0x1e4
> [122554.642025][   T92]  f2fs_create+0xf4/0x22c
> [122554.642047][   T92]  vfs_create+0x130/0x1f4
> 
> Thread #2:
> 
> [123996.386358][   T92]  __get_node_page+0x8c/0x504
>      -> waiting for dir->inode_page lock
> 
> [123996.386383][   T92]  read_all_xattrs+0x11c/0x1f4
> [123996.386405][   T92]  __f2fs_setxattr+0xcc/0x528
> [123996.386424][   T92]  f2fs_setxattr+0x158/0x1f4
>      -> f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
> 
> [123996.386443][   T92]  __f2fs_set_acl+0x328/0x430
> [123996.386618][   T92]  f2fs_set_acl+0x38/0x50
> [123996.386642][   T92]  posix_acl_chmod+0xc8/0x1c8
> [123996.386669][   T92]  f2fs_setattr+0x5e0/0x6bc
> [123996.386689][   T92]  notify_change+0x4d8/0x580
> [123996.386717][   T92]  chmod_common+0xd8/0x184
> [123996.386748][   T92]  do_fchmodat+0x60/0x124
> [123996.386766][   T92]  __arm64_sys_fchmodat+0x28/0x3c

Back to the race condition, my question is why we can chmod on inode before
it has been created?

Thanks,

> 
> Fixes: 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr"
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>   fs/f2fs/dir.c   | 9 ++++++++-
>   fs/f2fs/xattr.c | 6 ++++--
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 887e55988450..d635c58cf5a3 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -775,8 +775,15 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname,
>   {
>   	int err = -EAGAIN;
>   
> -	if (f2fs_has_inline_dentry(dir))
> +	if (f2fs_has_inline_dentry(dir)) {
> +		/*
> +		 * Should get i_xattr_sem to keep the lock order:
> +		 * i_xattr_sem -> inode_page lock used by f2fs_setxattr.
> +		 */
> +		f2fs_down_read(&F2FS_I(dir)->i_xattr_sem);
>   		err = f2fs_add_inline_entry(dir, fname, inode, ino, mode);
> +		f2fs_up_read(&F2FS_I(dir)->i_xattr_sem);
> +	}
>   	if (err == -EAGAIN)
>   		err = f2fs_add_regular_entry(dir, fname, inode, ino, mode);
>   
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 213805d3592c..476b186b90a6 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -528,10 +528,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>   	if (len > F2FS_NAME_LEN)
>   		return -ERANGE;
>   
> -	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> +	if (!ipage)
> +		f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>   	error = lookup_all_xattrs(inode, ipage, index, len, name,
>   				&entry, &base_addr, &base_size, &is_inline);
> -	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> +	if (!ipage)
> +		f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>   	if (error)
>   		return error;
>
Jaegeuk Kim June 28, 2023, 6:33 p.m. UTC | #9 
On 06/28, Chao Yu wrote:
> On 2023/6/28 16:08, Jaegeuk Kim wrote:
> > Thread #1:
> > 
> > [122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
> >      -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > 
> > [122554.641927][   T92]  __f2fs_get_acl+0x50/0x284
> > [122554.641948][   T92]  f2fs_init_acl+0x84/0x54c
> > [122554.641969][   T92]  f2fs_init_inode_metadata+0x460/0x5f0
> > [122554.641990][   T92]  f2fs_add_inline_entry+0x11c/0x350
> >      -> Locked dir->inode_page by f2fs_get_node_page()
> > 
> > [122554.642009][   T92]  f2fs_do_add_link+0x100/0x1e4
> > [122554.642025][   T92]  f2fs_create+0xf4/0x22c
> > [122554.642047][   T92]  vfs_create+0x130/0x1f4
> > 
> > Thread #2:
> > 
> > [123996.386358][   T92]  __get_node_page+0x8c/0x504
> >      -> waiting for dir->inode_page lock
> > 
> > [123996.386383][   T92]  read_all_xattrs+0x11c/0x1f4
> > [123996.386405][   T92]  __f2fs_setxattr+0xcc/0x528
> > [123996.386424][   T92]  f2fs_setxattr+0x158/0x1f4
> >      -> f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
> > 
> > [123996.386443][   T92]  __f2fs_set_acl+0x328/0x430
> > [123996.386618][   T92]  f2fs_set_acl+0x38/0x50
> > [123996.386642][   T92]  posix_acl_chmod+0xc8/0x1c8
> > [123996.386669][   T92]  f2fs_setattr+0x5e0/0x6bc
> > [123996.386689][   T92]  notify_change+0x4d8/0x580
> > [123996.386717][   T92]  chmod_common+0xd8/0x184
> > [123996.386748][   T92]  do_fchmodat+0x60/0x124
> > [123996.386766][   T92]  __arm64_sys_fchmodat+0x28/0x3c
> 
> Back to the race condition, my question is why we can chmod on inode before
> it has been created?

This is touching the directory.

> 
> Thanks,
> 
> > 
> > Fixes: 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr"
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >   fs/f2fs/dir.c   | 9 ++++++++-
> >   fs/f2fs/xattr.c | 6 ++++--
> >   2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > index 887e55988450..d635c58cf5a3 100644
> > --- a/fs/f2fs/dir.c
> > +++ b/fs/f2fs/dir.c
> > @@ -775,8 +775,15 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname,
> >   {
> >   	int err = -EAGAIN;
> > -	if (f2fs_has_inline_dentry(dir))
> > +	if (f2fs_has_inline_dentry(dir)) {
> > +		/*
> > +		 * Should get i_xattr_sem to keep the lock order:
> > +		 * i_xattr_sem -> inode_page lock used by f2fs_setxattr.
> > +		 */
> > +		f2fs_down_read(&F2FS_I(dir)->i_xattr_sem);
> >   		err = f2fs_add_inline_entry(dir, fname, inode, ino, mode);
> > +		f2fs_up_read(&F2FS_I(dir)->i_xattr_sem);
> > +	}
> >   	if (err == -EAGAIN)
> >   		err = f2fs_add_regular_entry(dir, fname, inode, ino, mode);
> > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > index 213805d3592c..476b186b90a6 100644
> > --- a/fs/f2fs/xattr.c
> > +++ b/fs/f2fs/xattr.c
> > @@ -528,10 +528,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
> >   	if (len > F2FS_NAME_LEN)
> >   		return -ERANGE;
> > -	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > +	if (!ipage)
> > +		f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> >   	error = lookup_all_xattrs(inode, ipage, index, len, name,
> >   				&entry, &base_addr, &base_size, &is_inline);
> > -	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> > +	if (!ipage)
> > +		f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> >   	if (error)
> >   		return error;
Jaegeuk Kim June 30, 2023, 9:59 p.m. UTC | #10 
On 06/28, Jaegeuk Kim wrote:
> On 06/28, Chao Yu wrote:
> > On 2023/6/28 16:08, Jaegeuk Kim wrote:
> > > Thread #1:
> > > 
> > > [122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
> > >      -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > > 
> > > [122554.641927][   T92]  __f2fs_get_acl+0x50/0x284
> > > [122554.641948][   T92]  f2fs_init_acl+0x84/0x54c
> > > [122554.641969][   T92]  f2fs_init_inode_metadata+0x460/0x5f0
> > > [122554.641990][   T92]  f2fs_add_inline_entry+0x11c/0x350
> > >      -> Locked dir->inode_page by f2fs_get_node_page()
> > > 
> > > [122554.642009][   T92]  f2fs_do_add_link+0x100/0x1e4
> > > [122554.642025][   T92]  f2fs_create+0xf4/0x22c
> > > [122554.642047][   T92]  vfs_create+0x130/0x1f4
> > > 
> > > Thread #2:
> > > 
> > > [123996.386358][   T92]  __get_node_page+0x8c/0x504
> > >      -> waiting for dir->inode_page lock
> > > 
> > > [123996.386383][   T92]  read_all_xattrs+0x11c/0x1f4
> > > [123996.386405][   T92]  __f2fs_setxattr+0xcc/0x528
> > > [123996.386424][   T92]  f2fs_setxattr+0x158/0x1f4
> > >      -> f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
> > > 
> > > [123996.386443][   T92]  __f2fs_set_acl+0x328/0x430
> > > [123996.386618][   T92]  f2fs_set_acl+0x38/0x50
> > > [123996.386642][   T92]  posix_acl_chmod+0xc8/0x1c8
> > > [123996.386669][   T92]  f2fs_setattr+0x5e0/0x6bc
> > > [123996.386689][   T92]  notify_change+0x4d8/0x580
> > > [123996.386717][   T92]  chmod_common+0xd8/0x184
> > > [123996.386748][   T92]  do_fchmodat+0x60/0x124
> > > [123996.386766][   T92]  __arm64_sys_fchmodat+0x28/0x3c
> > 
> > Back to the race condition, my question is why we can chmod on inode before
> > it has been created?
> 
> This is touching the directory.

Chao,

Do you have other concern? Otherwise, I'll include this into the next pull
request.

Thanks,

> 
> > 
> > Thanks,
> > 
> > > 
> > > Fixes: 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr"
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >   fs/f2fs/dir.c   | 9 ++++++++-
> > >   fs/f2fs/xattr.c | 6 ++++--
> > >   2 files changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > index 887e55988450..d635c58cf5a3 100644
> > > --- a/fs/f2fs/dir.c
> > > +++ b/fs/f2fs/dir.c
> > > @@ -775,8 +775,15 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname,
> > >   {
> > >   	int err = -EAGAIN;
> > > -	if (f2fs_has_inline_dentry(dir))
> > > +	if (f2fs_has_inline_dentry(dir)) {
> > > +		/*
> > > +		 * Should get i_xattr_sem to keep the lock order:
> > > +		 * i_xattr_sem -> inode_page lock used by f2fs_setxattr.
> > > +		 */
> > > +		f2fs_down_read(&F2FS_I(dir)->i_xattr_sem);
> > >   		err = f2fs_add_inline_entry(dir, fname, inode, ino, mode);
> > > +		f2fs_up_read(&F2FS_I(dir)->i_xattr_sem);
> > > +	}
> > >   	if (err == -EAGAIN)
> > >   		err = f2fs_add_regular_entry(dir, fname, inode, ino, mode);
> > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > > index 213805d3592c..476b186b90a6 100644
> > > --- a/fs/f2fs/xattr.c
> > > +++ b/fs/f2fs/xattr.c
> > > @@ -528,10 +528,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
> > >   	if (len > F2FS_NAME_LEN)
> > >   		return -ERANGE;
> > > -	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > > +	if (!ipage)
> > > +		f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
> > >   	error = lookup_all_xattrs(inode, ipage, index, len, name,
> > >   				&entry, &base_addr, &base_size, &is_inline);
> > > -	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> > > +	if (!ipage)
> > > +		f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
> > >   	if (error)
> > >   		return error;
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Chao Yu June 30, 2023, 11:36 p.m. UTC | #11 
On 2023/7/1 5:59, Jaegeuk Kim wrote:
> On 06/28, Jaegeuk Kim wrote:
>> On 06/28, Chao Yu wrote:
>>> On 2023/6/28 16:08, Jaegeuk Kim wrote:
>>>> Thread #1:
>>>>
>>>> [122554.641906][   T92]  f2fs_getxattr+0xd4/0x5fc
>>>>       -> waiting for f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>>>>
>>>> [122554.641927][   T92]  __f2fs_get_acl+0x50/0x284
>>>> [122554.641948][   T92]  f2fs_init_acl+0x84/0x54c
>>>> [122554.641969][   T92]  f2fs_init_inode_metadata+0x460/0x5f0
>>>> [122554.641990][   T92]  f2fs_add_inline_entry+0x11c/0x350
>>>>       -> Locked dir->inode_page by f2fs_get_node_page()
>>>>
>>>> [122554.642009][   T92]  f2fs_do_add_link+0x100/0x1e4
>>>> [122554.642025][   T92]  f2fs_create+0xf4/0x22c
>>>> [122554.642047][   T92]  vfs_create+0x130/0x1f4
>>>>
>>>> Thread #2:
>>>>
>>>> [123996.386358][   T92]  __get_node_page+0x8c/0x504
>>>>       -> waiting for dir->inode_page lock
>>>>
>>>> [123996.386383][   T92]  read_all_xattrs+0x11c/0x1f4
>>>> [123996.386405][   T92]  __f2fs_setxattr+0xcc/0x528
>>>> [123996.386424][   T92]  f2fs_setxattr+0x158/0x1f4
>>>>       -> f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
>>>>
>>>> [123996.386443][   T92]  __f2fs_set_acl+0x328/0x430
>>>> [123996.386618][   T92]  f2fs_set_acl+0x38/0x50
>>>> [123996.386642][   T92]  posix_acl_chmod+0xc8/0x1c8
>>>> [123996.386669][   T92]  f2fs_setattr+0x5e0/0x6bc
>>>> [123996.386689][   T92]  notify_change+0x4d8/0x580
>>>> [123996.386717][   T92]  chmod_common+0xd8/0x184
>>>> [123996.386748][   T92]  do_fchmodat+0x60/0x124
>>>> [123996.386766][   T92]  __arm64_sys_fchmodat+0x28/0x3c
>>>
>>> Back to the race condition, my question is why we can chmod on inode before
>>> it has been created?
>>
>> This is touching the directory.
> 
> Chao,
> 
> Do you have other concern? Otherwise, I'll include this into the next pull
> request.

Jaegeuk,

Sorry for late reply, I was misled by "dir->inode_page" and "inode" words in commit
message, it should be "dir->inode_page" and "dir_inode".

Anyway, the patch looks good to me.

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

> 
> Thanks,
> 
>>
>>>
>>> Thanks,
>>>
>>>>
>>>> Fixes: 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr"
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>> ---
>>>>    fs/f2fs/dir.c   | 9 ++++++++-
>>>>    fs/f2fs/xattr.c | 6 ++++--
>>>>    2 files changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>>>> index 887e55988450..d635c58cf5a3 100644
>>>> --- a/fs/f2fs/dir.c
>>>> +++ b/fs/f2fs/dir.c
>>>> @@ -775,8 +775,15 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname,
>>>>    {
>>>>    	int err = -EAGAIN;
>>>> -	if (f2fs_has_inline_dentry(dir))
>>>> +	if (f2fs_has_inline_dentry(dir)) {
>>>> +		/*
>>>> +		 * Should get i_xattr_sem to keep the lock order:
>>>> +		 * i_xattr_sem -> inode_page lock used by f2fs_setxattr.
>>>> +		 */
>>>> +		f2fs_down_read(&F2FS_I(dir)->i_xattr_sem);
>>>>    		err = f2fs_add_inline_entry(dir, fname, inode, ino, mode);
>>>> +		f2fs_up_read(&F2FS_I(dir)->i_xattr_sem);
>>>> +	}
>>>>    	if (err == -EAGAIN)
>>>>    		err = f2fs_add_regular_entry(dir, fname, inode, ino, mode);
>>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>>>> index 213805d3592c..476b186b90a6 100644
>>>> --- a/fs/f2fs/xattr.c
>>>> +++ b/fs/f2fs/xattr.c
>>>> @@ -528,10 +528,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>>>>    	if (len > F2FS_NAME_LEN)
>>>>    		return -ERANGE;
>>>> -	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>>>> +	if (!ipage)
>>>> +		f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
>>>>    	error = lookup_all_xattrs(inode, ipage, index, len, name,
>>>>    				&entry, &base_addr, &base_size, &is_inline);
>>>> -	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>>>> +	if (!ipage)
>>>> +		f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
>>>>    	if (error)
>>>>    		return error;
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
diff mbox series

Patch

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 3f5b161dd743..7b9af2d51656 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -838,7 +838,6 @@  struct f2fs_inode_info {
 
 	/* avoid racing between foreground op and gc */
 	struct f2fs_rwsem i_gc_rwsem[2];
-	struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */
 
 	int i_extra_isize;		/* size of extra space located in i_addr */
 	kprojid_t i_projid;		/* id for project quota */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 1b2c788ed80d..c917fa771f0e 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1418,7 +1418,6 @@  static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	INIT_LIST_HEAD(&fi->gdirty_list);
 	init_f2fs_rwsem(&fi->i_gc_rwsem[READ]);
 	init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
-	init_f2fs_rwsem(&fi->i_xattr_sem);
 
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 213805d3592c..bdc8a55085a2 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -433,7 +433,7 @@  static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	size_t inline_size = inline_xattr_size(inode);
-	struct page *in_page = NULL;
+	struct page *in_page = ipage;
 	void *xattr_addr;
 	void *inline_addr = NULL;
 	struct page *xpage;
@@ -446,29 +446,19 @@  static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
 
 	/* write to inline xattr */
 	if (inline_size) {
-		if (ipage) {
-			inline_addr = inline_xattr_addr(inode, ipage);
-		} else {
+		if (!in_page) {
 			in_page = f2fs_get_node_page(sbi, inode->i_ino);
 			if (IS_ERR(in_page)) {
 				f2fs_alloc_nid_failed(sbi, new_nid);
 				return PTR_ERR(in_page);
 			}
-			inline_addr = inline_xattr_addr(inode, in_page);
 		}
+		inline_addr = inline_xattr_addr(inode, in_page);
 
-		f2fs_wait_on_page_writeback(ipage ? ipage : in_page,
-							NODE, true, true);
-		/* no need to use xattr node block */
+		f2fs_wait_on_page_writeback(in_page, NODE, true, true);
 		if (hsize <= inline_size) {
-			err = f2fs_truncate_xattr_node(inode);
-			f2fs_alloc_nid_failed(sbi, new_nid);
-			if (err) {
-				f2fs_put_page(in_page, 1);
-				return err;
-			}
 			memcpy(inline_addr, txattr_addr, inline_size);
-			set_page_dirty(ipage ? ipage : in_page);
+			set_page_dirty(in_page);
 			goto in_page_out;
 		}
 	}
@@ -502,12 +492,13 @@  static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
 	memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE);
 
 	if (inline_size)
-		set_page_dirty(ipage ? ipage : in_page);
+		set_page_dirty(in_page);
 	set_page_dirty(xpage);
 
 	f2fs_put_page(xpage, 1);
 in_page_out:
-	f2fs_put_page(in_page, 1);
+	if (in_page != ipage)
+		f2fs_put_page(in_page, 1);
 	return err;
 }
 
@@ -528,10 +519,8 @@  int f2fs_getxattr(struct inode *inode, int index, const char *name,
 	if (len > F2FS_NAME_LEN)
 		return -ERANGE;
 
-	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
 	error = lookup_all_xattrs(inode, ipage, index, len, name,
 				&entry, &base_addr, &base_size, &is_inline);
-	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
 	if (error)
 		return error;
 
@@ -565,9 +554,7 @@  ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	int error;
 	size_t rest = buffer_size;
 
-	f2fs_down_read(&F2FS_I(inode)->i_xattr_sem);
 	error = read_all_xattrs(inode, NULL, &base_addr);
-	f2fs_up_read(&F2FS_I(inode)->i_xattr_sem);
 	if (error)
 		return error;
 
@@ -794,9 +781,7 @@  int f2fs_setxattr(struct inode *inode, int index, const char *name,
 	f2fs_balance_fs(sbi, true);
 
 	f2fs_lock_op(sbi);
-	f2fs_down_write(&F2FS_I(inode)->i_xattr_sem);
 	err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags);
-	f2fs_up_write(&F2FS_I(inode)->i_xattr_sem);
 	f2fs_unlock_op(sbi);
 
 	f2fs_update_time(sbi, REQ_TIME);