On 2019/9/29 10:20, Gao Xiang wrote:
> On Sun, Sep 29, 2019 at 08:53:05AM +0800, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2019/9/28 2:31, Jaegeuk Kim wrote:
>>> Hi Chao,
>>>
>>> On 09/25, Chao Yu wrote:
>>>> evict() should be called once i_count is zero, rather than i_nlinke
>>>> is zero.
>>>>
>>>> Reported-by: Gao Xiang <gaoxian...@huawei.com>
>>>> Signed-off-by: Chao Yu <yuch...@huawei.com>
>>>> ---
>>>> fs/f2fs/inode.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>> index db4fec30c30d..8262f4a483d3 100644
>>>> --- a/fs/f2fs/inode.c
>>>> +++ b/fs/f2fs/inode.c
>>>> @@ -632,7 +632,7 @@ int f2fs_write_inode(struct inode *inode, struct
>>>> writeback_control *wbc)
>>>> }
>>>>
>>>> /*
>>>> - * Called at the last iput() if i_nlink is zero
>>>
>>> I don't think this comment is wrong. You may be able to add on top of this.
>>
>> It actually misleads the developer or user.
>>
>> How do you think of:
>>
>> "Called at the last iput() if i_count is zero, and will release all meta/data
>> blocks allocated in the inode if i_nlink is zero"
>
> (sigh... side note: I just took some time to check the original meaning
> out of curiosity. AFAIK, the above word was added in Linux-2.1.45 [1]
> due to ext2_delete_inode behavior, which is called when i_nlink == 0,
> and .delete_inode was gone in 2010 (commit 72edc4d0873b merge ext2
> delete_inode and clear_inode, switch to ->evict_inode()), it may be
> helpful to understand the story so I write here for later folks reference.
> And it's also good to just kill it. )
Xiang,
Thanks for providing tracked info, I guess it's due to historical reason, we
kept the wrong comments in f2fs, anyway I agree that it should be fixed in
ext*/f2fs codes, let me send patches to fix comment in ext* as well.
Thanks,
>
> +
> +/*
> + * Called at the last iput() if i_nlink is zero.
> + */
> +void ext2_delete_inode (struct inode * inode)
> +{
> + if (inode->i_ino == EXT2_ACL_IDX_INO ||
> inode->i_ino == EXT2_ACL_DATA_INO)
> return;
> inode->u.ext2_i.i_dtime = CURRENT_TIME;
> - inode->i_dirt = 1;
> + mark_inode_dirty(inode);
> ext2_update_inode(inode, IS_SYNC(inode));
> inode->i_size = 0;
> if (inode->i_blocks)
> @@ -248,7 +258,7 @@
> if (IS_SYNC(inode) || inode->u.ext2_i.i_osync)
> ext2_sync_inode (inode);
> else
> - inode->i_dirt = 1;
> + mark_inode_dirty(inode);
> return result;
> }
>
> +void iput(struct inode *inode)
> {
> - struct inode * inode = get_empty_inode();
> + if (inode) {
> + struct super_operations *op = NULL;
>
> - PIPE_BASE(*inode) = (char*)__get_free_page(GFP_USER);
> - if (!(PIPE_BASE(*inode))) {
> - iput(inode);
> - return NULL;
> + if (inode->i_sb && inode->i_sb->s_op)
> + op = inode->i_sb->s_op;
> + if (op && op->put_inode)
> + op->put_inode(inode);
> +
> + spin_lock(&inode_lock);
> + if (!--inode->i_count) {
> + if (!inode->i_nlink) {
> + list_del(&inode->i_hash);
> + INIT_LIST_HEAD(&inode->i_hash);
> + if (op && op->delete_inode) {
> + void (*delete)(struct inode *) =
> op->delete_inode;
> + spin_unlock(&inode_lock);
> + delete(inode);
> + spin_lock(&inode_lock);
> + }
> + }
> + if (list_empty(&inode->i_hash)) {
> + list_del(&inode->i_list);
> + list_add(&inode->i_list, &inode_unused);
> + }
> + }
> + spin_unlock(&inode_lock);
> }
>
> [1] https://www.kernel.org/pub/linux/kernel/v2.1/patch-2.1.45.xz
>
> Thanks,
> Gao Xiang
>
>>
>> Thanks,
>>
>>>
>>>> + * Called at the last iput() if i_count is zero
>>>> */
>>>> void f2fs_evict_inode(struct inode *inode)
>>>> {
>>>> --
>>>> 2.18.0.rc1
>>> .
>>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
