lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 25 Oct 2017 22:06:57 +0800
From:   Yunlong Song <yunlong.song@...wei.com>
To:     Chao Yu <chao@...nel.org>, <jaegeuk@...nel.org>,
        <yuchao0@...wei.com>, <yunlong.song@...oud.com>
CC:     <miaoxie@...wei.com>, <bintian.wang@...wei.com>,
        <linux-fsdevel@...r.kernel.org>,
        <linux-f2fs-devel@...ts.sourceforge.net>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] f2fs: add cur_reserved_blocks to support soft block
 reservation

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


Powered by blists - more mailing lists