Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3] f2fs: add cur_reserved_blocks to support soft block reservation | From | Yunlong Song <> | Date | Wed, 25 Oct 2017 22:06:57 +0800 |
| |
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. > >>> + 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 ) > >>> + 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. 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. > > Thanks, > >>> spin_unlock(&sbi->stat_lock); >>> return count; >>> } > . >
-- Thanks, Yunlong Song
| |