From: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
To: Zhiguo Niu <niuzhiguo84@gmail.com>, wangzijie <wangzijie1@honor.com>
Cc: feng.han@honor.com, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, bo.wu@vivo.com,
jaegeuk@kernel.org
Subject: Re: [f2fs-dev] [PATCH] f2fs: continue to allocate pinned section when gc happens EAGAIN
Date: Fri, 13 Jun 2025 16:49:07 +0800 [thread overview]
Message-ID: <12dc51f7-a765-4995-bf6d-8abe973f4ea1@kernel.org> (raw)
In-Reply-To: <CAHJ8P3LSxzSbdz_ONUKy8dyDjLGsNJ08Aq724CUwYdOJ=m_2eA@mail.gmail.com>
On 2025/6/13 16:25, Zhiguo Niu wrote:
> wangzijie <wangzijie1@honor.com> 于2025年6月12日周四 20:38写道：
>>
>>> On 6/12/25 11:27, wangzijie wrote:
>>>> Wu Bo once mentioned a fallocate fail scenario in this link[1].
>>>> After commit 3fdd89b452c2("f2fs: prevent writing without fallocate()
>>>> for pinned files"), we cannot directly generate 4K size file and
>>>> pin it, but we can still generate non-segment aligned pinned file:
>>>>
>>>> touch test_file
>>>> ./f2fs_io pinfile set test_file
>>>> ./f2fs_io fallocate 0 0 8192 test_file
>>>> truncate -s 4096 test_file
>>>
>>> Well, shouldn't we avoid such case by adding check condition in setattr?
>>
>> Maybe like this?
>>
>> ---
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 6bd3de64f..2f6537d9c 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1027,6 +1027,8 @@ int f2fs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>> struct inode *inode = d_inode(dentry);
>> struct f2fs_inode_info *fi = F2FS_I(inode);
>> int err;
>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> + block_t sec_blks = CAP_BLKS_PER_SEC(sbi);
>>
>> if (unlikely(f2fs_cp_error(F2FS_I_SB(inode))))
>> return -EIO;
>> @@ -1047,6 +1049,11 @@ int f2fs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>> !IS_ALIGNED(attr->ia_size,
>> F2FS_BLK_TO_BYTES(fi->i_cluster_size)))
>> return -EINVAL;
>> + if (f2fs_is_pinned_file(inode) &&
>> + attr->ia_size < i_size_read(inode) &&
>> + !IS_ALIGNED(attr->ia_size,
>> + F2FS_BLK_TO_BYTES(sec_blks)));
>> + return -EINVAL;
> Hi Chao and zijie,
> Excuse me ,I'm a bit confused about this:
> when a pin file size is not alinged with size of "section", it can not
> be truncated?
IMO, the pinned file can only be truncated to section-aligned size.
Thanks
> thanks！
>> }
>>
>> err = setattr_prepare(idmap, dentry, attr);
>> ---
>>
>>>>
>>>> By doing this, pin+fallocate failure(gc happens EAGAIN but f2fs shows
>>>> enough spare space) may occurs.
>>>>
>>>> From message in commit 2e42b7f817ac("f2fs: stop allocating pinned sections
>>>> if EAGAIN happens"), gc EAGAIN doesn't guarantee a free section, so we stop
>>>> allocating. But after commit 48ea8b200414 ("f2fs: fix to avoid panic once
>>>> fallocation fails for pinfile"), we have a way to avoid panic caused by
>>>> concurrent pinfile allocation run out of free section, so I think that we
>>>> can continue to allocate pinned section when gc happens EAGAIN. Even if we
>>>> don't have free section, f2fs_allocate_pinning_section() can fail with ENOSPC.
>>>
>>> What do you think of introduce /sys/fs/f2fs/<dev>/reserved_pin_section to
>>> tune @needed parameter of has_not_enough_free_secs()? If we configure it
>>> w/ zero, it can avoid f2fs_gc() before preallocation.
>>>
>>> ---
>>> fs/f2fs/f2fs.h | 3 +++
>>> fs/f2fs/file.c | 5 ++---
>>> fs/f2fs/super.c | 3 +++
>>> fs/f2fs/sysfs.c | 9 +++++++++
>>> 4 files changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 785537576aa8..ffb15da570d7 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -1731,6 +1731,9 @@ struct f2fs_sb_info {
>>> /* for skip statistic */
>>> unsigned long long skipped_gc_rwsem; /* FG_GC only */
>>>
>>> + /* free sections reserved for pinned file */
>>> + unsigned int reserved_pin_section;
>>> +
>>> /* threshold for gc trials on pinned files */
>>> unsigned short gc_pin_file_threshold;
>>> struct f2fs_rwsem pin_sem;
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 696131e655ed..a909f79db178 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -1887,9 +1887,8 @@ static int f2fs_expand_inode_data(struct inode *inode, loff_t offset,
>>> }
>>> }
>>>
>>> - if (has_not_enough_free_secs(sbi, 0, f2fs_sb_has_blkzoned(sbi) ?
>>> - ZONED_PIN_SEC_REQUIRED_COUNT :
>>> - GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) {
>>> + if (has_not_enough_free_secs(sbi, 0,
>>> + sbi->reserved_pin_section)) {
>>> f2fs_down_write(&sbi->gc_lock);
>>> stat_inc_gc_call_count(sbi, FOREGROUND);
>>> err = f2fs_gc(sbi, &gc_control);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 57adeff5ef25..48b97a95fd63 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -4975,6 +4975,9 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
>>> sbi->last_valid_block_count = sbi->total_valid_block_count;
>>> sbi->reserved_blocks = 0;
>>> sbi->current_reserved_blocks = 0;
>>> + sbi->reserved_pin_section = f2fs_sb_has_blkzoned(sbi) ?
>>> + ZONED_PIN_SEC_REQUIRED_COUNT :
>>> + GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi));
>>> limit_reserve_root(sbi);
>>> adjust_unusable_cap_perc(sbi);
>>>
>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>> index 75134d69a0bd..51be7ffb38c5 100644
>>> --- a/fs/f2fs/sysfs.c
>>> +++ b/fs/f2fs/sysfs.c
>>> @@ -824,6 +824,13 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>>> return count;
>>> }
>>>
>>> + if (!strcmp(a->attr.name, "reserved_pin_section")) {
>>> + if (t > GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))
>>> + return -EINVAL;
>>> + *ui = (unsigned int)t;
>>> + return count;
>>> + }
>>> +
>>> *ui = (unsigned int)t;
>>>
>>> return count;
>>> @@ -1130,6 +1137,7 @@ F2FS_SBI_GENERAL_RO_ATTR(unusable_blocks_per_sec);
>>> F2FS_SBI_GENERAL_RW_ATTR(blkzone_alloc_policy);
>>> #endif
>>> F2FS_SBI_GENERAL_RW_ATTR(carve_out);
>>> +F2FS_SBI_GENERAL_RW_ATTR(reserved_pin_section);
>>>
>>> /* STAT_INFO ATTR */
>>> #ifdef CONFIG_F2FS_STAT_FS
>>> @@ -1323,6 +1331,7 @@ static struct attribute *f2fs_attrs[] = {
>>> ATTR_LIST(last_age_weight),
>>> ATTR_LIST(max_read_extent_count),
>>> ATTR_LIST(carve_out),
>>> + ATTR_LIST(reserved_pin_section),
>>> NULL,
>>> };
>>> ATTRIBUTE_GROUPS(f2fs);
>>> --
>>> 2.49.0
>>
>> I think it's a good way to solve this problem. Thank you!
>>
>>
>>>>
>>>> [1] https://lore.kernel.org/linux-f2fs-devel/20231030094024.263707-1-bo.wu@vivo.com/t/#u
>>>>
>>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>>> ---
>>>> fs/f2fs/file.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index 6bd3de64f..05c80d2b5 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -1859,7 +1859,7 @@ static int f2fs_expand_inode_data(struct inode *inode, loff_t offset,
>>>> f2fs_down_write(&sbi->gc_lock);
>>>> stat_inc_gc_call_count(sbi, FOREGROUND);
>>>> err = f2fs_gc(sbi, &gc_control);
>>>> - if (err && err != -ENODATA) {
>>>> + if (err && err != -ENODATA && err != -EAGAIN) {
>>>> f2fs_up_write(&sbi->pin_sem);
>>>> goto out_err;
>>>> }
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
WARNING: multiple messages have this Message-ID (diff)
From: Chao Yu <chao@kernel.org>
To: Zhiguo Niu <niuzhiguo84@gmail.com>, wangzijie <wangzijie1@honor.com>
Cc: chao@kernel.org, jaegeuk@kernel.org, bo.wu@vivo.com,
linux-kernel@vger.kernel.org, feng.han@honor.com,
linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: continue to allocate pinned section when gc happens EAGAIN
Date: Fri, 13 Jun 2025 16:49:07 +0800 [thread overview]
Message-ID: <12dc51f7-a765-4995-bf6d-8abe973f4ea1@kernel.org> (raw)
In-Reply-To: <CAHJ8P3LSxzSbdz_ONUKy8dyDjLGsNJ08Aq724CUwYdOJ=m_2eA@mail.gmail.com>
On 2025/6/13 16:25, Zhiguo Niu wrote:
> wangzijie <wangzijie1@honor.com> 于2025年6月12日周四 20:38写道：
>>
>>> On 6/12/25 11:27, wangzijie wrote:
>>>> Wu Bo once mentioned a fallocate fail scenario in this link[1].
>>>> After commit 3fdd89b452c2("f2fs: prevent writing without fallocate()
>>>> for pinned files"), we cannot directly generate 4K size file and
>>>> pin it, but we can still generate non-segment aligned pinned file:
>>>>
>>>> touch test_file
>>>> ./f2fs_io pinfile set test_file
>>>> ./f2fs_io fallocate 0 0 8192 test_file
>>>> truncate -s 4096 test_file
>>>
>>> Well, shouldn't we avoid such case by adding check condition in setattr?
>>
>> Maybe like this?
>>
>> ---
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 6bd3de64f..2f6537d9c 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1027,6 +1027,8 @@ int f2fs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>> struct inode *inode = d_inode(dentry);
>> struct f2fs_inode_info *fi = F2FS_I(inode);
>> int err;
>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> + block_t sec_blks = CAP_BLKS_PER_SEC(sbi);
>>
>> if (unlikely(f2fs_cp_error(F2FS_I_SB(inode))))
>> return -EIO;
>> @@ -1047,6 +1049,11 @@ int f2fs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>> !IS_ALIGNED(attr->ia_size,
>> F2FS_BLK_TO_BYTES(fi->i_cluster_size)))
>> return -EINVAL;
>> + if (f2fs_is_pinned_file(inode) &&
>> + attr->ia_size < i_size_read(inode) &&
>> + !IS_ALIGNED(attr->ia_size,
>> + F2FS_BLK_TO_BYTES(sec_blks)));
>> + return -EINVAL;
> Hi Chao and zijie,
> Excuse me ,I'm a bit confused about this:
> when a pin file size is not alinged with size of "section", it can not
> be truncated?
IMO, the pinned file can only be truncated to section-aligned size.
Thanks
> thanks！
>> }
>>
>> err = setattr_prepare(idmap, dentry, attr);
>> ---
>>
>>>>
>>>> By doing this, pin+fallocate failure(gc happens EAGAIN but f2fs shows
>>>> enough spare space) may occurs.
>>>>
>>>> From message in commit 2e42b7f817ac("f2fs: stop allocating pinned sections
>>>> if EAGAIN happens"), gc EAGAIN doesn't guarantee a free section, so we stop
>>>> allocating. But after commit 48ea8b200414 ("f2fs: fix to avoid panic once
>>>> fallocation fails for pinfile"), we have a way to avoid panic caused by
>>>> concurrent pinfile allocation run out of free section, so I think that we
>>>> can continue to allocate pinned section when gc happens EAGAIN. Even if we
>>>> don't have free section, f2fs_allocate_pinning_section() can fail with ENOSPC.
>>>
>>> What do you think of introduce /sys/fs/f2fs/<dev>/reserved_pin_section to
>>> tune @needed parameter of has_not_enough_free_secs()? If we configure it
>>> w/ zero, it can avoid f2fs_gc() before preallocation.
>>>
>>> ---
>>> fs/f2fs/f2fs.h | 3 +++
>>> fs/f2fs/file.c | 5 ++---
>>> fs/f2fs/super.c | 3 +++
>>> fs/f2fs/sysfs.c | 9 +++++++++
>>> 4 files changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 785537576aa8..ffb15da570d7 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -1731,6 +1731,9 @@ struct f2fs_sb_info {
>>> /* for skip statistic */
>>> unsigned long long skipped_gc_rwsem; /* FG_GC only */
>>>
>>> + /* free sections reserved for pinned file */
>>> + unsigned int reserved_pin_section;
>>> +
>>> /* threshold for gc trials on pinned files */
>>> unsigned short gc_pin_file_threshold;
>>> struct f2fs_rwsem pin_sem;
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 696131e655ed..a909f79db178 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -1887,9 +1887,8 @@ static int f2fs_expand_inode_data(struct inode *inode, loff_t offset,
>>> }
>>> }
>>>
>>> - if (has_not_enough_free_secs(sbi, 0, f2fs_sb_has_blkzoned(sbi) ?
>>> - ZONED_PIN_SEC_REQUIRED_COUNT :
>>> - GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) {
>>> + if (has_not_enough_free_secs(sbi, 0,
>>> + sbi->reserved_pin_section)) {
>>> f2fs_down_write(&sbi->gc_lock);
>>> stat_inc_gc_call_count(sbi, FOREGROUND);
>>> err = f2fs_gc(sbi, &gc_control);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 57adeff5ef25..48b97a95fd63 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -4975,6 +4975,9 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
>>> sbi->last_valid_block_count = sbi->total_valid_block_count;
>>> sbi->reserved_blocks = 0;
>>> sbi->current_reserved_blocks = 0;
>>> + sbi->reserved_pin_section = f2fs_sb_has_blkzoned(sbi) ?
>>> + ZONED_PIN_SEC_REQUIRED_COUNT :
>>> + GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi));
>>> limit_reserve_root(sbi);
>>> adjust_unusable_cap_perc(sbi);
>>>
>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>> index 75134d69a0bd..51be7ffb38c5 100644
>>> --- a/fs/f2fs/sysfs.c
>>> +++ b/fs/f2fs/sysfs.c
>>> @@ -824,6 +824,13 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>>> return count;
>>> }
>>>
>>> + if (!strcmp(a->attr.name, "reserved_pin_section")) {
>>> + if (t > GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))
>>> + return -EINVAL;
>>> + *ui = (unsigned int)t;
>>> + return count;
>>> + }
>>> +
>>> *ui = (unsigned int)t;
>>>
>>> return count;
>>> @@ -1130,6 +1137,7 @@ F2FS_SBI_GENERAL_RO_ATTR(unusable_blocks_per_sec);
>>> F2FS_SBI_GENERAL_RW_ATTR(blkzone_alloc_policy);
>>> #endif
>>> F2FS_SBI_GENERAL_RW_ATTR(carve_out);
>>> +F2FS_SBI_GENERAL_RW_ATTR(reserved_pin_section);
>>>
>>> /* STAT_INFO ATTR */
>>> #ifdef CONFIG_F2FS_STAT_FS
>>> @@ -1323,6 +1331,7 @@ static struct attribute *f2fs_attrs[] = {
>>> ATTR_LIST(last_age_weight),
>>> ATTR_LIST(max_read_extent_count),
>>> ATTR_LIST(carve_out),
>>> + ATTR_LIST(reserved_pin_section),
>>> NULL,
>>> };
>>> ATTRIBUTE_GROUPS(f2fs);
>>> --
>>> 2.49.0
>>
>> I think it's a good way to solve this problem. Thank you!
>>
>>
>>>>
>>>> [1] https://lore.kernel.org/linux-f2fs-devel/20231030094024.263707-1-bo.wu@vivo.com/t/#u
>>>>
>>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>>> ---
>>>> fs/f2fs/file.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index 6bd3de64f..05c80d2b5 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -1859,7 +1859,7 @@ static int f2fs_expand_inode_data(struct inode *inode, loff_t offset,
>>>> f2fs_down_write(&sbi->gc_lock);
>>>> stat_inc_gc_call_count(sbi, FOREGROUND);
>>>> err = f2fs_gc(sbi, &gc_control);
>>>> - if (err && err != -ENODATA) {
>>>> + if (err && err != -ENODATA && err != -EAGAIN) {
>>>> f2fs_up_write(&sbi->pin_sem);
>>>> goto out_err;
>>>> }
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2025-06-13 8:49 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-12 3:27 [f2fs-dev] [PATCH] f2fs: continue to allocate pinned section when gc happens EAGAIN wangzijie
2025-06-12 3:27 ` wangzijie
2025-06-12 8:31 ` Chao Yu via Linux-f2fs-devel
2025-06-12 8:31 ` Chao Yu
2025-06-12 12:34 ` wangzijie
2025-06-12 12:34 ` wangzijie
2025-06-12 14:07 ` Chao Yu via Linux-f2fs-devel
2025-06-12 14:07 ` Chao Yu
2025-06-13 2:03 ` wangzijie
2025-06-13 2:03 ` wangzijie
2025-06-13 8:25 ` Zhiguo Niu
2025-06-13 8:25 ` Zhiguo Niu
2025-06-13 8:49 ` Chao Yu via Linux-f2fs-devel [this message]
2025-06-13 8:49 ` Chao Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=12dc51f7-a765-4995-bf6d-8abe973f4ea1@kernel.org \
--to=linux-f2fs-devel@lists.sourceforge.net \
--cc=bo.wu@vivo.com \
--cc=chao@kernel.org \
--cc=feng.han@honor.com \
--cc=jaegeuk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=niuzhiguo84@gmail.com \
--cc=wangzijie1@honor.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.