Linux-f2fs-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Chao Yu <yuchao0@huawei.com>
To: Gao Xiang <gaoxiang25@huawei.com>,
	<linux-f2fs-devel@lists.sourceforge.net>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode
Date: Sun, 29 Sep 2019 10:52:49 +0800
Message-ID: <bcfbe250-09f4-3cb2-391f-97f574e82d35@huawei.com> (raw)
In-Reply-To: <20190929021939.GA136917@architecture4>

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

      reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-25  9:30 Chao Yu
2019-09-25 13:47 ` Gao Xiang
2019-09-26  1:04   ` Chao Yu
2019-09-27 18:31 ` Jaegeuk Kim
2019-09-27 19:01   ` Gao Xiang via Linux-f2fs-devel
2019-09-29  0:53   ` Chao Yu
2019-09-29  2:20     ` Gao Xiang
2019-09-29  2:52       ` Chao Yu [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bcfbe250-09f4-3cb2-391f-97f574e82d35@huawei.com \
    --to=yuchao0@huawei.com \
    --cc=gaoxiang25@huawei.com \
    --cc=jaegeuk@kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-f2fs-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-f2fs-devel/0 linux-f2fs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-f2fs-devel linux-f2fs-devel/ https://lore.kernel.org/linux-f2fs-devel \
		linux-f2fs-devel@lists.sourceforge.net linux-f2fs-devel@archiver.kernel.org
	public-inbox-index linux-f2fs-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.linux-f2fs-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox