* [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
@ 2024-08-27 6:22 wangzijie
2024-09-02 7:32 ` Chao Yu via Linux-f2fs-devel
0 siblings, 1 reply; 8+ messages in thread
From: wangzijie @ 2024-08-27 6:22 UTC (permalink / raw)
To: chao, jaegeuk; +Cc: wangzijie, linux-kernel, linux-f2fs-devel
Thread A
-dquot_initialize
-dqget
-f2fs_dquot_acquire
-v2_read_dquot
-qtree_read_dquot
-find_tree_dqentry
-f2fs_quota_read
-read_cache_page_gfp
-do_read_cache_folio
-fiemap_read_folio
-folio_wait_locked_killable
-receive SIGKILL : return -EINTR
-set SBI_QUOTA_NEED_REPAIR
-set SBI_QUOTA_NEED_REPAIR
When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
Signed-off-by: wangzijie <wangzijie1@honor.com>
---
fs/f2fs/inode.c | 3 ++-
fs/f2fs/super.c | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index ed629dabb..2af98e2b7 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -837,8 +837,9 @@ void f2fs_evict_inode(struct inode *inode)
err = f2fs_dquot_initialize(inode);
if (err) {
+ if (err != -EINTR)
+ set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
err = 0;
- set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
}
f2fs_remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 1f1b3647a..f99a36ff3 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2650,8 +2650,8 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
if (PTR_ERR(page) == -ENOMEM) {
memalloc_retry_wait(GFP_NOFS);
goto repeat;
- }
- set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
+ } else if (PTR_ERR(page) != -EINTR)
+ set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
return PTR_ERR(page);
}
@@ -3070,7 +3070,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
f2fs_down_read(&sbi->quota_sem);
ret = dquot_acquire(dquot);
- if (ret < 0)
+ if (ret < 0 && ret != -EINTR)
set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
f2fs_up_read(&sbi->quota_sem);
return ret;
--
2.25.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
2024-08-27 6:22 [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL wangzijie
@ 2024-09-02 7:32 ` Chao Yu via Linux-f2fs-devel
[not found] ` <20240903060658.1780002-1-wangzijie1@honor.com>
0 siblings, 1 reply; 8+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2024-09-02 7:32 UTC (permalink / raw)
To: wangzijie, jaegeuk; +Cc: linux-kernel, linux-f2fs-devel
On 2024/8/27 14:22, wangzijie wrote:
> Thread A
> -dquot_initialize
> -dqget
> -f2fs_dquot_acquire
> -v2_read_dquot
> -qtree_read_dquot
> -find_tree_dqentry
> -f2fs_quota_read
> -read_cache_page_gfp
> -do_read_cache_folio
> -fiemap_read_folio
> -folio_wait_locked_killable
> -receive SIGKILL : return -EINTR
> -set SBI_QUOTA_NEED_REPAIR
> -set SBI_QUOTA_NEED_REPAIR
>
> When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
> set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
f2fs_quota_read() can be called in a lot of contexts, can we just ignore -EINTR
for f2fs_dquot_initialize() case?
Thanks,
>
> Signed-off-by: wangzijie <wangzijie1@honor.com>
> ---
> fs/f2fs/inode.c | 3 ++-
> fs/f2fs/super.c | 6 +++---
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index ed629dabb..2af98e2b7 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -837,8 +837,9 @@ void f2fs_evict_inode(struct inode *inode)
>
> err = f2fs_dquot_initialize(inode);
> if (err) {
> + if (err != -EINTR)
> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> err = 0;
> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> }
>
> f2fs_remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 1f1b3647a..f99a36ff3 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2650,8 +2650,8 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
> if (PTR_ERR(page) == -ENOMEM) {
> memalloc_retry_wait(GFP_NOFS);
> goto repeat;
> - }
> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
> + } else if (PTR_ERR(page) != -EINTR)
> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
> return PTR_ERR(page);
> }
>
> @@ -3070,7 +3070,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>
> f2fs_down_read(&sbi->quota_sem);
> ret = dquot_acquire(dquot);
> - if (ret < 0)
> + if (ret < 0 && ret != -EINTR)
> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> f2fs_up_read(&sbi->quota_sem);
> return ret;
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
[not found] ` <20240903060658.1780002-1-wangzijie1@honor.com>
@ 2024-09-04 3:49 ` Chao Yu via Linux-f2fs-devel
2024-09-08 4:12 ` wangzijie
2024-09-11 7:12 ` Chao Yu via Linux-f2fs-devel
0 siblings, 2 replies; 8+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2024-09-04 3:49 UTC (permalink / raw)
To: linux-f2fs-devel; +Cc: jaegeuk, wangzijie1, linux-kernel
On 2024/9/3 14:06, wangzijie wrote:
> From: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
>
>> On 2024/8/27 14:22, wangzijie wrote:
>>> Thread A
>>> -dquot_initialize
>>> -dqget
>>> -f2fs_dquot_acquire
>>> -v2_read_dquot
>>> -qtree_read_dquot
>>> -find_tree_dqentry
>>> -f2fs_quota_read
>>> -read_cache_page_gfp
>>> -do_read_cache_folio
>>> -fiemap_read_folio
>>> -folio_wait_locked_killable
>>> -receive SIGKILL : return -EINTR
>>> -set SBI_QUOTA_NEED_REPAIR
>>> -set SBI_QUOTA_NEED_REPAIR
>>>
>>> When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
>>> set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
>>
>> f2fs_quota_read() can be called in a lot of contexts, can we just ignore -EINTR
>> for f2fs_dquot_initialize() case?
>>
>> Thanks,
>
> Yes, in many contexts f2fs_quota_read() can be called and may return -EINTR, we need to ignore this errno for more cases. If we need to do so, I will check it and resend patch.
> Or do you have other suggestions to avoid unnecessary SBI_QUOTA_NEED_REPAIR flag set?
How about this?
---
fs/f2fs/f2fs.h | 1 +
fs/f2fs/inode.c | 3 +--
fs/f2fs/super.c | 17 +++++++++++++----
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index dfed1974eda5..a1704a19dfe9 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -810,6 +810,7 @@ enum {
FI_ATOMIC_DIRTIED, /* indicate atomic file is dirtied */
FI_ATOMIC_REPLACE, /* indicate atomic replace */
FI_OPENED_FILE, /* indicate file has been opened */
+ FI_INIT_DQUOT, /* indicate it's initializing dquot */
FI_MAX, /* max flag, never be used */
};
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 008f01348afa..b1dbaeda306f 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -827,8 +827,7 @@ void f2fs_evict_inode(struct inode *inode)
err = f2fs_dquot_initialize(inode);
if (err) {
- if (err != -EINTR)
- set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
+ set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
err = 0;
}
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8e29aba4b7a4..e774bdf875b2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2644,8 +2644,11 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
if (PTR_ERR(page) == -ENOMEM) {
memalloc_retry_wait(GFP_NOFS);
goto repeat;
- } else if (PTR_ERR(page) != -EINTR)
- set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
+ } else if (PTR_ERR(page) == -EINTR &&
+ is_inode_flag_set(inode, FI_INIT_DQUOT)) {
+ return PTR_ERR(page);
+ }
+ set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
return PTR_ERR(page);
}
@@ -2721,10 +2724,16 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
int f2fs_dquot_initialize(struct inode *inode)
{
+ int ret;
+
if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT))
return -ESRCH;
- return dquot_initialize(inode);
+ set_inode_flag(inode, FI_INIT_DQUOT);
+ ret = dquot_initialize(inode);
+ clear_inode_flag(inode, FI_INIT_DQUOT);
+
+ return ret;
}
static struct dquot __rcu **f2fs_get_dquots(struct inode *inode)
@@ -3064,7 +3073,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
f2fs_down_read(&sbi->quota_sem);
ret = dquot_acquire(dquot);
- if (ret < 0 && ret != -EINTR)
+ if (ret < 0)
set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
f2fs_up_read(&sbi->quota_sem);
return ret;
--
2.40.1
>
> Thank you for review.
>
>>>
>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>> ---
>>> fs/f2fs/inode.c | 3 ++-
>>> fs/f2fs/super.c | 6 +++---
>>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index ed629dabb..2af98e2b7 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -837,8 +837,9 @@ void f2fs_evict_inode(struct inode *inode)
>>> err = f2fs_dquot_initialize(inode);
>>> if (err) {
>>> + if (err != -EINTR)
>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>> err = 0;
>>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>> }
>>> f2fs_remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 1f1b3647a..f99a36ff3 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -2650,8 +2650,8 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>> if (PTR_ERR(page) == -ENOMEM) {
>>> memalloc_retry_wait(GFP_NOFS);
>>> goto repeat;
>>> - }
>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>> + } else if (PTR_ERR(page) != -EINTR)
>>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>> return PTR_ERR(page);
>>> }
>>> @@ -3070,7 +3070,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>> f2fs_down_read(&sbi->quota_sem);
>>> ret = dquot_acquire(dquot);
>>> - if (ret < 0)
>>> + if (ret < 0 && ret != -EINTR)
>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>> f2fs_up_read(&sbi->quota_sem);
>>> return ret;
>
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
2024-09-04 3:49 ` Chao Yu via Linux-f2fs-devel
@ 2024-09-08 4:12 ` wangzijie
2024-09-09 1:24 ` Chao Yu via Linux-f2fs-devel
2024-09-11 7:12 ` Chao Yu via Linux-f2fs-devel
1 sibling, 1 reply; 8+ messages in thread
From: wangzijie @ 2024-09-08 4:12 UTC (permalink / raw)
To: chao; +Cc: jaegeuk, wangzijie1, linux-kernel, linux-f2fs-devel
>> From: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
>>
>>> On 2024/8/27 14:22, wangzijie wrote:
>>>> Thread A
>>>> -dquot_initialize
>>>> -dqget
>>>> -f2fs_dquot_acquire
>>>> -v2_read_dquot
>>>> -qtree_read_dquot
>>>> -find_tree_dqentry
>>>> -f2fs_quota_read
>>>> -read_cache_page_gfp
>>>> -do_read_cache_folio
>>>> -fiemap_read_folio
>>>> -folio_wait_locked_killable
>>>> -receive SIGKILL : return -EINTR
>>>> -set SBI_QUOTA_NEED_REPAIR
>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>
>>>> When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
>>>> set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
>>>
>>> f2fs_quota_read() can be called in a lot of contexts, can we just ignore -EINTR
>>> for f2fs_dquot_initialize() case?
>>>
>>> Thanks,
>>
>> Yes, in many contexts f2fs_quota_read() can be called and may return -EINTR, we need to ignore this errno for more cases. If we need to do so, I will check it and resend patch.
>> Or do you have other suggestions to avoid unnecessary SBI_QUOTA_NEED_REPAIR flag set?
>
>How about this?
>
>---
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/inode.c | 3 +--
> fs/f2fs/super.c | 17 +++++++++++++----
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
>diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>index dfed1974eda5..a1704a19dfe9 100644
>--- a/fs/f2fs/f2fs.h
>+++ b/fs/f2fs/f2fs.h
>@@ -810,6 +810,7 @@ enum {
> FI_ATOMIC_DIRTIED, /* indicate atomic file is dirtied */
> FI_ATOMIC_REPLACE, /* indicate atomic replace */
> FI_OPENED_FILE, /* indicate file has been opened */
>+ FI_INIT_DQUOT, /* indicate it's initializing dquot */
> FI_MAX, /* max flag, never be used */
> };
>
>diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>index 008f01348afa..b1dbaeda306f 100644
>--- a/fs/f2fs/inode.c
>+++ b/fs/f2fs/inode.c
>@@ -827,8 +827,7 @@ void f2fs_evict_inode(struct inode *inode)
>
> err = f2fs_dquot_initialize(inode);
> if (err) {
>- if (err != -EINTR)
>- set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>+ set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> err = 0;
> }
>
>diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>index 8e29aba4b7a4..e774bdf875b2 100644
>--- a/fs/f2fs/super.c
>+++ b/fs/f2fs/super.c
>@@ -2644,8 +2644,11 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
> if (PTR_ERR(page) == -ENOMEM) {
> memalloc_retry_wait(GFP_NOFS);
> goto repeat;
>- } else if (PTR_ERR(page) != -EINTR)
>- set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>+ } else if (PTR_ERR(page) == -EINTR &&
>+ is_inode_flag_set(inode, FI_INIT_DQUOT)) {
>+ return PTR_ERR(page);
>+ }
>+ set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
> return PTR_ERR(page);
> }
>
>@@ -2721,10 +2724,16 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
>
> int f2fs_dquot_initialize(struct inode *inode)
> {
>+ int ret;
>+
> if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT))
> return -ESRCH;
>
>- return dquot_initialize(inode);
>+ set_inode_flag(inode, FI_INIT_DQUOT);
>+ ret = dquot_initialize(inode);
>+ clear_inode_flag(inode, FI_INIT_DQUOT);
>+
>+ return ret;
> }
>
> static struct dquot __rcu **f2fs_get_dquots(struct inode *inode)
>@@ -3064,7 +3073,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>
> f2fs_down_read(&sbi->quota_sem);
> ret = dquot_acquire(dquot);
>- if (ret < 0 && ret != -EINTR)
>+ if (ret < 0)
> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> f2fs_up_read(&sbi->quota_sem);
> return ret;
>--
>2.40.1
Hi, Chao
If we dont't ignore -EINTR in f2fs_dquot_acquire(), we will still set SBI_QUOTA_NEED_REPAIR flag
in f2fs_dquot_acquire() if f2fs_quota_read return -EINTR. I think we need more cases in addition to
dquot initializing and I will check it again.
Thank you for your suggestion!
>>
>> Thank you for review.
>>
>>>>
>>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>>> ---
>>>> fs/f2fs/inode.c | 3 ++-
>>>> fs/f2fs/super.c | 6 +++---
>>>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>> index ed629dabb..2af98e2b7 100644
>>>> --- a/fs/f2fs/inode.c
>>>> +++ b/fs/f2fs/inode.c
>>>> @@ -837,8 +837,9 @@ void f2fs_evict_inode(struct inode *inode)
>>>> err = f2fs_dquot_initialize(inode);
>>>> if (err) {
>>>> + if (err != -EINTR)
>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>> err = 0;
>>>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>> }
>>>> f2fs_remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index 1f1b3647a..f99a36ff3 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -2650,8 +2650,8 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>>> if (PTR_ERR(page) == -ENOMEM) {
>>>> memalloc_retry_wait(GFP_NOFS);
>>>> goto repeat;
>>>> - }
>>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>> + } else if (PTR_ERR(page) != -EINTR)
>>>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>> return PTR_ERR(page);
>>>> }
>>>> @@ -3070,7 +3070,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>>> f2fs_down_read(&sbi->quota_sem);
>>>> ret = dquot_acquire(dquot);
>>>> - if (ret < 0)
>>>> + if (ret < 0 && ret != -EINTR)
>>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>> f2fs_up_read(&sbi->quota_sem);
>>>> return ret;
>>
>>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
2024-09-08 4:12 ` wangzijie
@ 2024-09-09 1:24 ` Chao Yu via Linux-f2fs-devel
2024-09-09 4:06 ` wangzijie
0 siblings, 1 reply; 8+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2024-09-09 1:24 UTC (permalink / raw)
To: wangzijie; +Cc: jaegeuk, linux-kernel, linux-f2fs-devel
On 2024/9/8 12:12, wangzijie wrote:
>>> From: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
>>>
>>>> On 2024/8/27 14:22, wangzijie wrote:
>>>>> Thread A
>>>>> -dquot_initialize
>>>>> -dqget
>>>>> -f2fs_dquot_acquire
>>>>> -v2_read_dquot
>>>>> -qtree_read_dquot
>>>>> -find_tree_dqentry
>>>>> -f2fs_quota_read
>>>>> -read_cache_page_gfp
>>>>> -do_read_cache_folio
>>>>> -fiemap_read_folio
>>>>> -folio_wait_locked_killable
>>>>> -receive SIGKILL : return -EINTR
>>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>>
>>>>> When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
>>>>> set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
>>>>
>>>> f2fs_quota_read() can be called in a lot of contexts, can we just ignore -EINTR
>>>> for f2fs_dquot_initialize() case?
>>>>
>>>> Thanks,
>>>
>>> Yes, in many contexts f2fs_quota_read() can be called and may return -EINTR, we need to ignore this errno for more cases. If we need to do so, I will check it and resend patch.
>>> Or do you have other suggestions to avoid unnecessary SBI_QUOTA_NEED_REPAIR flag set?
>>
>> How about this?
>>
>> ---
>> fs/f2fs/f2fs.h | 1 +
>> fs/f2fs/inode.c | 3 +--
>> fs/f2fs/super.c | 17 +++++++++++++----
>> 3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index dfed1974eda5..a1704a19dfe9 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -810,6 +810,7 @@ enum {
>> FI_ATOMIC_DIRTIED, /* indicate atomic file is dirtied */
>> FI_ATOMIC_REPLACE, /* indicate atomic replace */
>> FI_OPENED_FILE, /* indicate file has been opened */
>> + FI_INIT_DQUOT, /* indicate it's initializing dquot */
>> FI_MAX, /* max flag, never be used */
>> };
>>
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index 008f01348afa..b1dbaeda306f 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -827,8 +827,7 @@ void f2fs_evict_inode(struct inode *inode)
>>
>> err = f2fs_dquot_initialize(inode);
>> if (err) {
>> - if (err != -EINTR)
>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>> err = 0;
>> }
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 8e29aba4b7a4..e774bdf875b2 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -2644,8 +2644,11 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>> if (PTR_ERR(page) == -ENOMEM) {
>> memalloc_retry_wait(GFP_NOFS);
>> goto repeat;
>> - } else if (PTR_ERR(page) != -EINTR)
>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>> + } else if (PTR_ERR(page) == -EINTR &&
>> + is_inode_flag_set(inode, FI_INIT_DQUOT)) {
>> + return PTR_ERR(page);
>> + }
>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>> return PTR_ERR(page);
>> }
>>
>> @@ -2721,10 +2724,16 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
>>
>> int f2fs_dquot_initialize(struct inode *inode)
>> {
>> + int ret;
>> +
>> if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT))
>> return -ESRCH;
>>
>> - return dquot_initialize(inode);
>> + set_inode_flag(inode, FI_INIT_DQUOT);
>> + ret = dquot_initialize(inode);
>> + clear_inode_flag(inode, FI_INIT_DQUOT);
>> +
>> + return ret;
>> }
>>
>> static struct dquot __rcu **f2fs_get_dquots(struct inode *inode)
>> @@ -3064,7 +3073,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>
>> f2fs_down_read(&sbi->quota_sem);
>> ret = dquot_acquire(dquot);
>> - if (ret < 0 && ret != -EINTR)
>> + if (ret < 0)
>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>> f2fs_up_read(&sbi->quota_sem);
>> return ret;
>> --
>> 2.40.1
>
> Hi, Chao
> If we dont't ignore -EINTR in f2fs_dquot_acquire(), we will still set SBI_QUOTA_NEED_REPAIR flag
> in f2fs_dquot_acquire() if f2fs_quota_read return -EINTR. I think we need more cases in addition to
> dquot initializing and I will check it again.
Maybe we can cover this case w/ below diff?
---
fs/f2fs/super.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index e774bdf875b2..7fc970121a3f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3073,7 +3073,8 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
f2fs_down_read(&sbi->quota_sem);
ret = dquot_acquire(dquot);
- if (ret < 0)
+ if (ret < 0 &&
+ (ret != -EINTR || !is_inode_flag_set(inode, FI_INIT_DQUOT)))
set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
f2fs_up_read(&sbi->quota_sem);
return ret;
--
2.40.1
Thanks,
> Thank you for your suggestion!
>
>>>
>>> Thank you for review.
>>>
>>>>>
>>>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>>>> ---
>>>>> fs/f2fs/inode.c | 3 ++-
>>>>> fs/f2fs/super.c | 6 +++---
>>>>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>> index ed629dabb..2af98e2b7 100644
>>>>> --- a/fs/f2fs/inode.c
>>>>> +++ b/fs/f2fs/inode.c
>>>>> @@ -837,8 +837,9 @@ void f2fs_evict_inode(struct inode *inode)
>>>>> err = f2fs_dquot_initialize(inode);
>>>>> if (err) {
>>>>> + if (err != -EINTR)
>>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>> err = 0;
>>>>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>> }
>>>>> f2fs_remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>> index 1f1b3647a..f99a36ff3 100644
>>>>> --- a/fs/f2fs/super.c
>>>>> +++ b/fs/f2fs/super.c
>>>>> @@ -2650,8 +2650,8 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>>>> if (PTR_ERR(page) == -ENOMEM) {
>>>>> memalloc_retry_wait(GFP_NOFS);
>>>>> goto repeat;
>>>>> - }
>>>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>>> + } else if (PTR_ERR(page) != -EINTR)
>>>>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>>> return PTR_ERR(page);
>>>>> }
>>>>> @@ -3070,7 +3070,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>>>> f2fs_down_read(&sbi->quota_sem);
>>>>> ret = dquot_acquire(dquot);
>>>>> - if (ret < 0)
>>>>> + if (ret < 0 && ret != -EINTR)
>>>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>> f2fs_up_read(&sbi->quota_sem);
>>>>> return ret;
>>>
>>>
>
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
2024-09-09 1:24 ` Chao Yu via Linux-f2fs-devel
@ 2024-09-09 4:06 ` wangzijie
2024-09-09 9:04 ` Chao Yu via Linux-f2fs-devel
0 siblings, 1 reply; 8+ messages in thread
From: wangzijie @ 2024-09-09 4:06 UTC (permalink / raw)
To: chao; +Cc: jaegeuk, wangzijie1, linux-kernel, linux-f2fs-devel
>On 2024/9/8 12:12, wangzijie wrote:
>>>> From: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
>>>>
>>>>> On 2024/8/27 14:22, wangzijie wrote:
>>>>>> Thread A
>>>>>> -dquot_initialize
>>>>>> -dqget
>>>>>> -f2fs_dquot_acquire
>>>>>> -v2_read_dquot
>>>>>> -qtree_read_dquot
>>>>>> -find_tree_dqentry
>>>>>> -f2fs_quota_read
>>>>>> -read_cache_page_gfp
>>>>>> -do_read_cache_folio
>>>>>> -fiemap_read_folio
>>>>>> -folio_wait_locked_killable
>>>>>> -receive SIGKILL : return -EINTR
>>>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>>>
>>>>>> When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
>>>>>> set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
>>>>>
>>>>> f2fs_quota_read() can be called in a lot of contexts, can we just ignore -EINTR
>>>>> for f2fs_dquot_initialize() case?
>>>>>
>>>>> Thanks,
>>>>
>>>> Yes, in many contexts f2fs_quota_read() can be called and may return -EINTR, we need to ignore this errno for more cases. If we need to do so, I will check it and resend patch.
>>>> Or do you have other suggestions to avoid unnecessary SBI_QUOTA_NEED_REPAIR flag set?
>>>
>>> How about this?
>>>
>>> ---
>>> fs/f2fs/f2fs.h | 1 +
>>> fs/f2fs/inode.c | 3 +--
>>> fs/f2fs/super.c | 17 +++++++++++++----
>>> 3 files changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index dfed1974eda5..a1704a19dfe9 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -810,6 +810,7 @@ enum {
>>> FI_ATOMIC_DIRTIED, /* indicate atomic file is dirtied */
>>> FI_ATOMIC_REPLACE, /* indicate atomic replace */
>>> FI_OPENED_FILE, /* indicate file has been opened */
>>> + FI_INIT_DQUOT, /* indicate it's initializing dquot */
>>> FI_MAX, /* max flag, never be used */
>>> };
>>>
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 008f01348afa..b1dbaeda306f 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -827,8 +827,7 @@ void f2fs_evict_inode(struct inode *inode)
>>>
>>> err = f2fs_dquot_initialize(inode);
>>> if (err) {
>>> - if (err != -EINTR)
>>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>> err = 0;
>>> }
>>>
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 8e29aba4b7a4..e774bdf875b2 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -2644,8 +2644,11 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>> if (PTR_ERR(page) == -ENOMEM) {
>>> memalloc_retry_wait(GFP_NOFS);
>>> goto repeat;
>>> - } else if (PTR_ERR(page) != -EINTR)
>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>> + } else if (PTR_ERR(page) == -EINTR &&
>>> + is_inode_flag_set(inode, FI_INIT_DQUOT)) {
>>> + return PTR_ERR(page);
>>> + }
>>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>> return PTR_ERR(page);
>>> }
>>>
>>> @@ -2721,10 +2724,16 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
>>>
>>> int f2fs_dquot_initialize(struct inode *inode)
>>> {
>>> + int ret;
>>> +
>>> if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT))
>>> return -ESRCH;
>>>
>>> - return dquot_initialize(inode);
>>> + set_inode_flag(inode, FI_INIT_DQUOT);
>>> + ret = dquot_initialize(inode);
>>> + clear_inode_flag(inode, FI_INIT_DQUOT);
>>> +
>>> + return ret;
>>> }
>>>
>>> static struct dquot __rcu **f2fs_get_dquots(struct inode *inode)
>>> @@ -3064,7 +3073,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>>
>>> f2fs_down_read(&sbi->quota_sem);
>>> ret = dquot_acquire(dquot);
>>> - if (ret < 0 && ret != -EINTR)
>>> + if (ret < 0)
>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>> f2fs_up_read(&sbi->quota_sem);
>>> return ret;
>>> --
>>> 2.40.1
>>
>> Hi, Chao
>> If we dont't ignore -EINTR in f2fs_dquot_acquire(), we will still set SBI_QUOTA_NEED_REPAIR flag
>> in f2fs_dquot_acquire() if f2fs_quota_read return -EINTR. I think we need more cases in addition to
>> dquot initializing and I will check it again.
>
>Maybe we can cover this case w/ below diff?
>
>---
> fs/f2fs/super.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>index e774bdf875b2..7fc970121a3f 100644
>--- a/fs/f2fs/super.c
>+++ b/fs/f2fs/super.c
>@@ -3073,7 +3073,8 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>
> f2fs_down_read(&sbi->quota_sem);
> ret = dquot_acquire(dquot);
>- if (ret < 0)
>+ if (ret < 0 &&
>+ (ret != -EINTR || !is_inode_flag_set(inode, FI_INIT_DQUOT)))
> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> f2fs_up_read(&sbi->quota_sem);
> return ret;
>--
>2.40.1
>
>Thanks,
Yes, I think it can cover initializing dquot case.
>
>> Thank you for your suggestion!
>>
>>>>
>>>> Thank you for review.
>>>>
>>>>>>
>>>>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>>>>> ---
>>>>>> fs/f2fs/inode.c | 3 ++-
>>>>>> fs/f2fs/super.c | 6 +++---
>>>>>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>> index ed629dabb..2af98e2b7 100644
>>>>>> --- a/fs/f2fs/inode.c
>>>>>> +++ b/fs/f2fs/inode.c
>>>>>> @@ -837,8 +837,9 @@ void f2fs_evict_inode(struct inode *inode)
>>>>>> err = f2fs_dquot_initialize(inode);
>>>>>> if (err) {
>>>>>> + if (err != -EINTR)
>>>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>>> err = 0;
>>>>>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>>> }
>>>>>> f2fs_remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>> index 1f1b3647a..f99a36ff3 100644
>>>>>> --- a/fs/f2fs/super.c
>>>>>> +++ b/fs/f2fs/super.c
>>>>>> @@ -2650,8 +2650,8 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>>>>> if (PTR_ERR(page) == -ENOMEM) {
>>>>>> memalloc_retry_wait(GFP_NOFS);
>>>>>> goto repeat;
>>>>>> - }
>>>>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>>>> + } else if (PTR_ERR(page) != -EINTR)
>>>>>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>>>> return PTR_ERR(page);
>>>>>> }
>>>>>> @@ -3070,7 +3070,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>>>>> f2fs_down_read(&sbi->quota_sem);
>>>>>> ret = dquot_acquire(dquot);
>>>>>> - if (ret < 0)
>>>>>> + if (ret < 0 && ret != -EINTR)
>>>>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>>> f2fs_up_read(&sbi->quota_sem);
>>>>>> return ret;
>>>>
>>>>
>>
>>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
2024-09-09 4:06 ` wangzijie
@ 2024-09-09 9:04 ` Chao Yu via Linux-f2fs-devel
0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2024-09-09 9:04 UTC (permalink / raw)
To: wangzijie; +Cc: jaegeuk, linux-kernel, linux-f2fs-devel
On 2024/9/9 12:06, wangzijie wrote:
>> On 2024/9/8 12:12, wangzijie wrote:
>>>>> From: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
>>>>>
>>>>>> On 2024/8/27 14:22, wangzijie wrote:
>>>>>>> Thread A
>>>>>>> -dquot_initialize
>>>>>>> -dqget
>>>>>>> -f2fs_dquot_acquire
>>>>>>> -v2_read_dquot
>>>>>>> -qtree_read_dquot
>>>>>>> -find_tree_dqentry
>>>>>>> -f2fs_quota_read
>>>>>>> -read_cache_page_gfp
>>>>>>> -do_read_cache_folio
>>>>>>> -fiemap_read_folio
>>>>>>> -folio_wait_locked_killable
>>>>>>> -receive SIGKILL : return -EINTR
>>>>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>>>>
>>>>>>> When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
>>>>>>> set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
>>>>>>
>>>>>> f2fs_quota_read() can be called in a lot of contexts, can we just ignore -EINTR
>>>>>> for f2fs_dquot_initialize() case?
>>>>>>
>>>>>> Thanks,
>>>>>
>>>>> Yes, in many contexts f2fs_quota_read() can be called and may return -EINTR, we need to ignore this errno for more cases. If we need to do so, I will check it and resend patch.
>>>>> Or do you have other suggestions to avoid unnecessary SBI_QUOTA_NEED_REPAIR flag set?
>>>>
>>>> How about this?
>>>>
>>>> ---
>>>> fs/f2fs/f2fs.h | 1 +
>>>> fs/f2fs/inode.c | 3 +--
>>>> fs/f2fs/super.c | 17 +++++++++++++----
>>>> 3 files changed, 15 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index dfed1974eda5..a1704a19dfe9 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -810,6 +810,7 @@ enum {
>>>> FI_ATOMIC_DIRTIED, /* indicate atomic file is dirtied */
>>>> FI_ATOMIC_REPLACE, /* indicate atomic replace */
>>>> FI_OPENED_FILE, /* indicate file has been opened */
>>>> + FI_INIT_DQUOT, /* indicate it's initializing dquot */
>>>> FI_MAX, /* max flag, never be used */
>>>> };
>>>>
>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>> index 008f01348afa..b1dbaeda306f 100644
>>>> --- a/fs/f2fs/inode.c
>>>> +++ b/fs/f2fs/inode.c
>>>> @@ -827,8 +827,7 @@ void f2fs_evict_inode(struct inode *inode)
>>>>
>>>> err = f2fs_dquot_initialize(inode);
>>>> if (err) {
>>>> - if (err != -EINTR)
>>>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>> err = 0;
>>>> }
>>>>
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index 8e29aba4b7a4..e774bdf875b2 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -2644,8 +2644,11 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>>> if (PTR_ERR(page) == -ENOMEM) {
>>>> memalloc_retry_wait(GFP_NOFS);
>>>> goto repeat;
>>>> - } else if (PTR_ERR(page) != -EINTR)
>>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>> + } else if (PTR_ERR(page) == -EINTR &&
>>>> + is_inode_flag_set(inode, FI_INIT_DQUOT)) {
>>>> + return PTR_ERR(page);
>>>> + }
>>>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>> return PTR_ERR(page);
>>>> }
>>>>
>>>> @@ -2721,10 +2724,16 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
>>>>
>>>> int f2fs_dquot_initialize(struct inode *inode)
>>>> {
>>>> + int ret;
>>>> +
>>>> if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT))
>>>> return -ESRCH;
>>>>
>>>> - return dquot_initialize(inode);
>>>> + set_inode_flag(inode, FI_INIT_DQUOT);
>>>> + ret = dquot_initialize(inode);
>>>> + clear_inode_flag(inode, FI_INIT_DQUOT);
>>>> +
>>>> + return ret;
>>>> }
>>>>
>>>> static struct dquot __rcu **f2fs_get_dquots(struct inode *inode)
>>>> @@ -3064,7 +3073,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>>>
>>>> f2fs_down_read(&sbi->quota_sem);
>>>> ret = dquot_acquire(dquot);
>>>> - if (ret < 0 && ret != -EINTR)
>>>> + if (ret < 0)
>>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>> f2fs_up_read(&sbi->quota_sem);
>>>> return ret;
>>>> --
>>>> 2.40.1
>>>
>>> Hi, Chao
>>> If we dont't ignore -EINTR in f2fs_dquot_acquire(), we will still set SBI_QUOTA_NEED_REPAIR flag
>>> in f2fs_dquot_acquire() if f2fs_quota_read return -EINTR. I think we need more cases in addition to
>>> dquot initializing and I will check it again.
>>
>> Maybe we can cover this case w/ below diff?
>>
>> ---
>> fs/f2fs/super.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index e774bdf875b2..7fc970121a3f 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -3073,7 +3073,8 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>
>> f2fs_down_read(&sbi->quota_sem);
>> ret = dquot_acquire(dquot);
>> - if (ret < 0)
>> + if (ret < 0 &&
>> + (ret != -EINTR || !is_inode_flag_set(inode, FI_INIT_DQUOT)))
>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>> f2fs_up_read(&sbi->quota_sem);
>> return ret;
>> --
>> 2.40.1
>>
>> Thanks,
>
> Yes, I think it can cover initializing dquot case.
Oops, please ignore this, as we can not get inode pointer in
f2fs_dquot_acquire() context...
Thanks,
>
>>
>>> Thank you for your suggestion!
>>>
>>>>>
>>>>> Thank you for review.
>>>>>
>>>>>>>
>>>>>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>>>>>> ---
>>>>>>> fs/f2fs/inode.c | 3 ++-
>>>>>>> fs/f2fs/super.c | 6 +++---
>>>>>>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>>> index ed629dabb..2af98e2b7 100644
>>>>>>> --- a/fs/f2fs/inode.c
>>>>>>> +++ b/fs/f2fs/inode.c
>>>>>>> @@ -837,8 +837,9 @@ void f2fs_evict_inode(struct inode *inode)
>>>>>>> err = f2fs_dquot_initialize(inode);
>>>>>>> if (err) {
>>>>>>> + if (err != -EINTR)
>>>>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>>>> err = 0;
>>>>>>> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>>>> }
>>>>>>> f2fs_remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>>> index 1f1b3647a..f99a36ff3 100644
>>>>>>> --- a/fs/f2fs/super.c
>>>>>>> +++ b/fs/f2fs/super.c
>>>>>>> @@ -2650,8 +2650,8 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>>>>>> if (PTR_ERR(page) == -ENOMEM) {
>>>>>>> memalloc_retry_wait(GFP_NOFS);
>>>>>>> goto repeat;
>>>>>>> - }
>>>>>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>>>>> + } else if (PTR_ERR(page) != -EINTR)
>>>>>>> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>>>>> return PTR_ERR(page);
>>>>>>> }
>>>>>>> @@ -3070,7 +3070,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>>>>>>> f2fs_down_read(&sbi->quota_sem);
>>>>>>> ret = dquot_acquire(dquot);
>>>>>>> - if (ret < 0)
>>>>>>> + if (ret < 0 && ret != -EINTR)
>>>>>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>>>>> f2fs_up_read(&sbi->quota_sem);
>>>>>>> return ret;
>>>>>
>>>>>
>>>
>>>
>
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL
2024-09-04 3:49 ` Chao Yu via Linux-f2fs-devel
2024-09-08 4:12 ` wangzijie
@ 2024-09-11 7:12 ` Chao Yu via Linux-f2fs-devel
1 sibling, 0 replies; 8+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2024-09-11 7:12 UTC (permalink / raw)
To: wangzijie1; +Cc: jaegeuk, linux-kernel, linux-f2fs-devel
On 2024/9/4 11:49, Chao Yu via Linux-f2fs-devel wrote:
> On 2024/9/3 14:06, wangzijie wrote:
>> From: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
>>
>>> On 2024/8/27 14:22, wangzijie wrote:
>>>> Thread A
>>>> -dquot_initialize
>>>> -dqget
>>>> -f2fs_dquot_acquire
>>>> -v2_read_dquot
>>>> -qtree_read_dquot
>>>> -find_tree_dqentry
>>>> -f2fs_quota_read
>>>> -read_cache_page_gfp
>>>> -do_read_cache_folio
>>>> -fiemap_read_folio
>>>> -folio_wait_locked_killable
>>>> -receive SIGKILL : return -EINTR
>>>> -set SBI_QUOTA_NEED_REPAIR
>>>> -set SBI_QUOTA_NEED_REPAIR
>>>>
>>>> When calling read_cache_page_gfp in quota read, thread may receive SIGKILL and
>>>> set SBI_QUOTA_NEED_REPAIR, should we set SBI_QUOTA_NEED_REPAIR in this error path?
>>>
>>> f2fs_quota_read() can be called in a lot of contexts, can we just ignore -EINTR
>>> for f2fs_dquot_initialize() case?
>>>
>>> Thanks,
>>
>> Yes, in many contexts f2fs_quota_read() can be called and may return -EINTR, we need to ignore this errno for more cases. If we need to do so, I will check it and resend patch.
>> Or do you have other suggestions to avoid unnecessary SBI_QUOTA_NEED_REPAIR flag set?
>
> How about this?
>
> ---
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/inode.c | 3 +--
> fs/f2fs/super.c | 17 +++++++++++++----
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index dfed1974eda5..a1704a19dfe9 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -810,6 +810,7 @@ enum {
> FI_ATOMIC_DIRTIED, /* indicate atomic file is dirtied */
> FI_ATOMIC_REPLACE, /* indicate atomic replace */
> FI_OPENED_FILE, /* indicate file has been opened */
> + FI_INIT_DQUOT, /* indicate it's initializing dquot */
> FI_MAX, /* max flag, never be used */
> };
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 008f01348afa..b1dbaeda306f 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -827,8 +827,7 @@ void f2fs_evict_inode(struct inode *inode)
>
> err = f2fs_dquot_initialize(inode);
> if (err) {
> - if (err != -EINTR)
> - set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> err = 0;
> }
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 8e29aba4b7a4..e774bdf875b2 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2644,8 +2644,11 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
> if (PTR_ERR(page) == -ENOMEM) {
> memalloc_retry_wait(GFP_NOFS);
> goto repeat;
> - } else if (PTR_ERR(page) != -EINTR)
> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
> + } else if (PTR_ERR(page) == -EINTR &&
> + is_inode_flag_set(inode, FI_INIT_DQUOT)) {
Current inode is quota inode, it's not the same inode we tagged
w/ FI_INIT_DQUOT, so please ignore this diff...
Thanks,
> + return PTR_ERR(page);
> + }
> + set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
> return PTR_ERR(page);
> }
>
> @@ -2721,10 +2724,16 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
>
> int f2fs_dquot_initialize(struct inode *inode)
> {
> + int ret;
> +
> if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT))
> return -ESRCH;
>
> - return dquot_initialize(inode);
> + set_inode_flag(inode, FI_INIT_DQUOT);
> + ret = dquot_initialize(inode);
> + clear_inode_flag(inode, FI_INIT_DQUOT);
> +
> + return ret;
> }
>
> static struct dquot __rcu **f2fs_get_dquots(struct inode *inode)
> @@ -3064,7 +3073,7 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
>
> f2fs_down_read(&sbi->quota_sem);
> ret = dquot_acquire(dquot);
> - if (ret < 0 && ret != -EINTR)
> + if (ret < 0)
> set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
> f2fs_up_read(&sbi->quota_sem);
> return ret;
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-09-11 7:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-27 6:22 [f2fs-dev] [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL wangzijie
2024-09-02 7:32 ` Chao Yu via Linux-f2fs-devel
[not found] ` <20240903060658.1780002-1-wangzijie1@honor.com>
2024-09-04 3:49 ` Chao Yu via Linux-f2fs-devel
2024-09-08 4:12 ` wangzijie
2024-09-09 1:24 ` Chao Yu via Linux-f2fs-devel
2024-09-09 4:06 ` wangzijie
2024-09-09 9:04 ` Chao Yu via Linux-f2fs-devel
2024-09-11 7:12 ` Chao Yu via Linux-f2fs-devel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).