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
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.

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));

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:11    [W:0.929 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site