lkml.org 
[lkml]   [2017]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3] f2fs: add cur_reserved_blocks to support soft block reservation
From
Date
Agree

On 2017/10/26 11:26, Chao Yu wrote:
> On 2017/10/26 11:07, Yunlong Song wrote:
>> Yes, I agree with the soft semantic you introduce, it is too slow to
>> increase cur_reserved_blocks only in
>> dec_valid_block(,node)_count, e.g. if users want to set
>> cur_reserved_blocks to 10G.
> Yup,
>
>> Then how about fix the initialization of cur_reserved_blocks in
>> fs/f2fs/super.c as following:
>> sbi->current_reserved_blocks = 0
>> change to
>> sbi->current_reserved_blocks = min(sbi->reserved_blocks,
>> sbi->user_block_count - valid_user_blocks(sbi));
> It will be necessary only if reserved_blocks is initialized with a non-zero value,
> but now it has value of zero, so it's redundant...
>
> What about adding this check if we initialize reserved_blocks with a non-zero default
> value or value which may be configured with mount_option?
>
> Thanks,
>
>> On 2017/10/25 23:46, Chao Yu wrote:
>>> On 2017/10/25 22:06, Yunlong Song wrote:
>>>> Hi, Chao,
>>>> Please see my comments below.
>>>>
>>>> On 2017/10/25 20:26, Chao Yu wrote:
>>>>> On 2017/10/25 18:02, Yunlong Song wrote:
>>>>>> ping...
>>>>> I've replied in this thread, check your email list please, or you can check the
>>>>> comments in below link:
>>>>>
>>>>> https://patchwork.kernel.org/patch/9909407/
>>>>>
>>>>> Anyway, see comments below.
>>>>>
>>>>>> On 2017/8/18 23:09, Yunlong Song wrote:
>>>>>>> This patch adds cur_reserved_blocks to extend reserved_blocks sysfs
>>>>>>> interface to be soft threshold, which allows user configure it exceeding
>>>>>>> current available user space. To ensure there is enough space for
>>>>>>> supporting system's activation, this patch does not set the reserved space
>>>>>>> to the configured reserved_blocks value at once, instead, it safely
>>>>>>> increase cur_reserved_blocks in dev_valid_block(,node)_count to only take
>>>>>>> up the blocks which are just obsoleted.
>>>>>>>
>>>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>> ---
>>>>>>> Documentation/ABI/testing/sysfs-fs-f2fs | 3 ++-
>>>>>>> fs/f2fs/f2fs.h | 13 +++++++++++--
>>>>>>> fs/f2fs/super.c | 3 ++-
>>>>>>> fs/f2fs/sysfs.c | 15 +++++++++++++--
>>>>>>> 4 files changed, 28 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>>>> index 11b7f4e..ba282ca 100644
>>>>>>> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>>>> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>>>> @@ -138,7 +138,8 @@ What: /sys/fs/f2fs/<disk>/reserved_blocks
>>>>>>> Date: June 2017
>>>>>>> Contact: "Chao Yu" <yuchao0@huawei.com>
>>>>>>> Description:
>>>>>>> - Controls current reserved blocks in system.
>>>>>>> + Controls current reserved blocks in system, the threshold
>>>>>>> + is soft, it could exceed current available user space.
>>>>>>> What: /sys/fs/f2fs/<disk>/gc_urgent
>>>>>>> Date: August 2017
>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>> index 2f20b6b..84ccbdc 100644
>>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>>> @@ -1041,6 +1041,7 @@ struct f2fs_sb_info {
>>>>>>> block_t discard_blks; /* discard command candidats */
>>>>>>> block_t last_valid_block_count; /* for recovery */
>>>>>>> block_t reserved_blocks; /* configurable reserved blocks */
>>>>>>> + block_t cur_reserved_blocks; /* current reserved blocks */
>>>>>>> u32 s_next_generation; /* for NFS support */
>>>>>>> @@ -1515,7 +1516,8 @@ static inline int inc_valid_block_count(struct f2fs_sb_info *sbi,
>>>>>>> spin_lock(&sbi->stat_lock);
>>>>>>> sbi->total_valid_block_count += (block_t)(*count);
>>>>>>> - avail_user_block_count = sbi->user_block_count - sbi->reserved_blocks;
>>>>>>> + avail_user_block_count = sbi->user_block_count -
>>>>>>> + sbi->cur_reserved_blocks;
>>>>>>> if (unlikely(sbi->total_valid_block_count > avail_user_block_count)) {
>>>>>>> diff = sbi->total_valid_block_count - avail_user_block_count;
>>>>>>> *count -= diff;
>>>>>>> @@ -1549,6 +1551,10 @@ static inline void dec_valid_block_count(struct f2fs_sb_info *sbi,
>>>>>>> f2fs_bug_on(sbi, sbi->total_valid_block_count < (block_t) count);
>>>>>>> f2fs_bug_on(sbi, inode->i_blocks < sectors);
>>>>>>> sbi->total_valid_block_count -= (block_t)count;
>>>>>>> + if (sbi->reserved_blocks &&
>>>>>>> + sbi->reserved_blocks != sbi->cur_reserved_blocks)
>>>>> It's redundent check here...
>>>> I think in most cases, cur_reserved_blocks is equal to reserved_blocks, so we do not need to calculate min any more, otherwise,
>>>> if reserved_blocks is not 0, it will calculate min and set current_reserved_blocks each time.
>>> OK, IMO, in some condition, we can save dirtying cache line to decrease cache
>>> line missing with that check.
>>>
>>>>>>> + sbi->cur_reserved_blocks = min(sbi->reserved_blocks,
>>>>>>> + sbi->cur_reserved_blocks + count);
>>>>>>> spin_unlock(&sbi->stat_lock);
>>>>>>> f2fs_i_blocks_write(inode, count, false, true);
>>>>>>> }
>>>>>>> @@ -1695,7 +1701,7 @@ static inline int inc_valid_node_count(struct f2fs_sb_info *sbi,
>>>>>>> spin_lock(&sbi->stat_lock);
>>>>>>> valid_block_count = sbi->total_valid_block_count + 1;
>>>>>>> - if (unlikely(valid_block_count + sbi->reserved_blocks >
>>>>>>> + if (unlikely(valid_block_count + sbi->cur_reserved_blocks >
>>>>>>> sbi->user_block_count)) {
>>>>>>> spin_unlock(&sbi->stat_lock);
>>>>>>> goto enospc;
>>>>>>> @@ -1738,6 +1744,9 @@ static inline void dec_valid_node_count(struct f2fs_sb_info *sbi,
>>>>>>> sbi->total_valid_node_count--;
>>>>>>> sbi->total_valid_block_count--;
>>>>>>> + if (sbi->reserved_blocks &&
>>>>>>> + sbi->reserved_blocks != sbi->cur_reserved_blocks)
>>>>> Checking low boundary is more safe here.
>>>> I think cur_reserved_blocks can never be larger than reserved_blocks in any case. so min(reserved_blocks,
>>>> cur_reserved_blocks +1) is same to cur_reserved_blocks++ when reserved_blocks != cur_reserved_blocks
>>>> (which means reserved_blocks > cur_reserved_block )
>>> Ditto.
>>>
>>>>>>> + sbi->cur_reserved_blocks++;
>>>>>>> spin_unlock(&sbi->stat_lock);
>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>>> index 4c1bdcb..16a805f 100644
>>>>>>> --- a/fs/f2fs/super.c
>>>>>>> +++ b/fs/f2fs/super.c
>>>>>>> @@ -957,7 +957,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>>>>>> buf->f_blocks = total_count - start_count;
>>>>>>> buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
>>>>>>> buf->f_bavail = user_block_count - valid_user_blocks(sbi) -
>>>>>>> - sbi->reserved_blocks;
>>>>>>> + sbi->cur_reserved_blocks;
>>>>>>> avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
>>>>>>> @@ -2411,6 +2411,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>>>>> le64_to_cpu(sbi->ckpt->valid_block_count);
>>>>>>> sbi->last_valid_block_count = sbi->total_valid_block_count;
>>>>>>> sbi->reserved_blocks = 0;
>>>>>>> + sbi->cur_reserved_blocks = 0;
>>>>>>> for (i = 0; i < NR_INODE_TYPE; i++) {
>>>>>>> INIT_LIST_HEAD(&sbi->inode_list[i]);
>>>>>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>>>>>> index a1be5ac..75c37bb 100644
>>>>>>> --- a/fs/f2fs/sysfs.c
>>>>>>> +++ b/fs/f2fs/sysfs.c
>>>>>>> @@ -104,12 +104,22 @@ static ssize_t features_show(struct f2fs_attr *a,
>>>>>>> return len;
>>>>>>> }
>>>>>>> +static ssize_t f2fs_reserved_blocks_show(struct f2fs_attr *a,
>>>>>>> + struct f2fs_sb_info *sbi, char *buf)
>>>>>>> +{
>>>>>>> + return snprintf(buf, PAGE_SIZE, "expected: %u\ncurrent: %u\n",
>>>>>>> + sbi->reserved_blocks, sbi->cur_reserved_blocks);
>>>>>>> +}
>>>>>>> +
>>>>>>> static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
>>>>>>> struct f2fs_sb_info *sbi, char *buf)
>>>>>>> {
>>>>>>> unsigned char *ptr = NULL;
>>>>>>> unsigned int *ui;
>>>>>>> + if (a->struct_type == RESERVED_BLOCKS)
>>>>>>> + return f2fs_reserved_blocks_show(a, sbi, buf);
>>>>>>> +
>>>>>>> ptr = __struct_ptr(sbi, a->struct_type);
>>>>>>> if (!ptr)
>>>>>>> return -EINVAL;
>>>>>>> @@ -143,12 +153,13 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
>>>>>>> #endif
>>>>>>> if (a->struct_type == RESERVED_BLOCKS) {
>>>>>>> spin_lock(&sbi->stat_lock);
>>>>>>> - if ((unsigned long)sbi->total_valid_block_count + t >
>>>>>>> - (unsigned long)sbi->user_block_count) {
>>>>>>> + if (t > (unsigned long)sbi->user_block_count) {
>>>>>>> spin_unlock(&sbi->stat_lock);
>>>>>>> return -EINVAL;
>>>>>>> }
>>>>>>> *ui = t;
>>>>>>> + if (t < (unsigned long)sbi->cur_reserved_blocks)
>>>>>>> + sbi->cur_reserved_blocks = t;
>>>>> No, for 't < cur_reserved_blocks' case, cur_reserved_blocks will out of update
>>>>> even if there is enough free space. You know, for soft block resevation, we need
>>>>> to reserve blocks as many as possible, making free space being zero suddenly is
>>>>> possible.
>>>> I do not understand why it is not safe to decrease cur_reserved_blocks, for example,
>>>> if current cur_reserved_blocks is 100, now decrease it to 80, is there any problem?
>>>> If 80 will make free space zero, how does 100 exist?
>>>> And I do not think it is safe as following:
>>>> *ui = t;
>>>> + sbi->current_reserved_blocks = min(sbi->reserved_blocks,
>>>> + sbi->user_block_count - valid_user_blocks(sbi));
>>>>
>>>> If user_block_count = 200, valid_user_blocks=150, reserved_blocks = 100,
>>>> then current_reserved_block = min(100,200-50) = 50, in this case, free space
>>>> is suddenly becoming zero.
>>> Free space becomes zero suddenly is safe, as I said before, I don't expect this
>>> feature can be used in android, instead, it may be used in distributed storage
>>> scenario, in where, once we configure soft_reserved_block making one server out
>>> of free space, it's not critical issue to this system since we can move current
>>> copy to another server which has enough free space.
>>>
>>> Secondly, as an global configuration, it's due to usage of administrator with
>>> it, if there is critical application which is sensitive with free space,
>>> administrator should make sure our reservation should not overload consuming free
>>> space, which means soft reservation is not suitable.
>>>
>>>> To avoid this, I change the code to:
>>>>
>>>> + if (t < (unsigned long)sbi->cur_reserved_blocks)
>>>> + sbi->cur_reserved_blocks = t;
>>>>
>>>> I think it is only safe to decrease the value of cur_reserved_blocks, and leave increase operation to
>>>> dec_valid_block(,node)_count, it is safe to increase cur_reserved_blocks there.
>>> For initialization of reserved_blocks, cur_reserved_blocks will always be zero
>>> due to this check, and will be updated to close to reserved_blocks after block
>>> allocation and deallocation of user, IMO, it's not looks reasonable to user.
>>>
>>> Anyway, it's due to how you define semantics of soft reservation, so what is your
>>> understanding of it?
>>>
>>> Thanks,
>>>
>>>>> Thanks,
>>>>>
>>>>>>> spin_unlock(&sbi->stat_lock);
>>>>>>> return count;
>>>>>>> }
>>>>> .
>>>>>
>>> .
>>>
>
> .
>

--
Thanks,
Yunlong Song


\
 
 \ /
  Last update: 2017-10-26 05:32    [W:0.076 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site