2023-06-13 23:42:31

by Jaegeuk Kim

[permalink] [raw]
Subject: [PATCH] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

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: <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
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);
--
2.41.0.162.gfafddb0af9-goog



2023-06-15 18:06:51

by patchwork-bot+f2fs

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

Hello:

This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <[email protected]>:

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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



2023-06-25 07:33:34

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

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: <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> 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);

2023-06-25 10:54:37

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

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: <[email protected]>
>> Signed-off-by: Jaegeuk Kim <[email protected]>
>> ---
>>   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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2023-06-26 13:17:05

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

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: <[email protected]>
> > > Signed-off-by: Jaegeuk Kim <[email protected]>
> > > ---
> > > ? 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
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2023-06-27 14:10:50

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

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: <[email protected]>
>>>> Signed-off-by: Jaegeuk Kim <[email protected]>
>>>> ---
>>>>   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
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2023-06-28 08:43:49

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: fix deadlock in i_xattr_sem and inode page lock and fix the original issue

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: <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
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;

--
2.41.0.162.gfafddb0af9-goog


2023-06-28 09:18:44

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: fix deadlock in i_xattr_sem and inode page lock and fix the original issue

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: <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> 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;
>

2023-06-28 09:32:56

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH] f2fs: remove i_xattr_sem to avoid deadlock and fix the original issue

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: <[email protected]>
> > > > > Signed-off-by: Jaegeuk Kim <[email protected]>
> > > > > ---
> > > > > ? 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
> > > > [email protected]
> > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2023-06-28 18:50:20

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: fix deadlock in i_xattr_sem and inode page lock and fix the original issue

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: <[email protected]>
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > 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;

2023-06-30 22:16:53

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH v2] f2fs: fix deadlock in i_xattr_sem and inode page lock and fix the original issue

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: <[email protected]>
> > > Signed-off-by: Jaegeuk Kim <[email protected]>
> > > ---
> > > 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2023-06-30 23:56:17

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH v2] f2fs: fix deadlock in i_xattr_sem and inode page lock and fix the original issue

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 <[email protected]>

Thanks,

>
> Thanks,
>
>>
>>>
>>> Thanks,
>>>
>>>>
>>>> Fixes: 27161f13e3c3 "f2fs: avoid race in between read xattr & write xattr"
>>>> Cc: <[email protected]>
>>>> Signed-off-by: Jaegeuk Kim <[email protected]>
>>>> ---
>>>> 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
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel