Re: [PATCH v3] f2fs: add cur_reserved_blocks to support soft block reservation

[Chao Yu]

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@xxxxxxxxxx>
>> Signed-off-by: Chao Yu <yuchao0@xxxxxxxxxx>
>> ---
>> Â 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@xxxxxxxxxx>
>> Â 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...

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

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

Thanks,

>> ÂÂÂÂÂÂÂÂÂ spin_unlock(&sbi->stat_lock);
>> ÂÂÂÂÂÂÂÂÂ return count;
>> ÂÂÂÂÂ }
>