[PATCH] f2fs: fix comment of f2fs_evict_inode

[PATCH] f2fs: fix comment of f2fs_evict_inode

[Chao Yu]

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
+ * Called at the last iput() if i_count is zero
  */
 void f2fs_evict_inode(struct inode *inode)
 {
-- 
2.18.0.rc1

Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode

[Gao Xiang]

Hi Chao,

On Wed, Sep 25, 2019 at 05:30:50PM +0800, 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
> + * Called at the last iput() if i_count is zero

Yeah, I'd suggest taking some time to look at other
inconsistent comments, it makes other folks confused
and ask me with such-"strong" reason...

Thanks,
Gao Xiang

>   */
>  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

Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode

[Chao Yu]

On 2019/9/25 21:47, Gao Xiang wrote:
> Hi Chao,
> 
> On Wed, Sep 25, 2019 at 05:30:50PM +0800, 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
>> + * Called at the last iput() if i_count is zero
> 
> Yeah, I'd suggest taking some time to look at other
> inconsistent comments, it makes other folks confused
> and ask me with such-"strong" reason...

Xiang, I'm looking into it, will fix those inconsistent comments in another
patch, please wait a while. ;)

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>   */
>>  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
> .
> 

Re: [PATCH] f2fs: fix comment of f2fs_evict_inode

[Jaegeuk Kim]

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.

> + * Called at the last iput() if i_count is zero
>   */
>  void f2fs_evict_inode(struct inode *inode)
>  {
> -- 
> 2.18.0.rc1

Re: [PATCH] f2fs: fix comment of f2fs_evict_inode

[Gao Xiang]

Hi Jaegeuk,

On Fri, Sep 27, 2019 at 11:31:50AM -0700, 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.

Actually I don't really care what this line means, but someone really
told me that .evict_inode() is called on inode is finally removed
because he saw this line.

In practice, I have no idea what the above line (especially the word i_nlink
== 0) mainly emphasizes, just from some documentation (not even refer some code):

Documentation/filesystems/porting.rst
326 **mandatory**
327 
328 ->clear_inode() and ->delete_inode() are gone; ->evict_inode() should
329 be used instead.  It gets called whenever the inode is evicted, whether it has
330 remaining links or not. 

And it seems it's the same comment exists in ext2/ext4. But yes, it's up
to you. However, it misleaded someone and I had to explain more about this.

Thanks,
Gao Xiang

> 
> > + * Called at the last iput() if i_count is zero
> >   */
> >  void f2fs_evict_inode(struct inode *inode)
> >  {
> > -- 
> > 2.18.0.rc1

Re: [PATCH] f2fs: fix comment of f2fs_evict_inode

[Chao Yu]

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"

Thanks,

> 
>> + * Called at the last iput() if i_count is zero
>>   */
>>  void f2fs_evict_inode(struct inode *inode)
>>  {
>> -- 
>> 2.18.0.rc1
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode

[Gao Xiang]

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. )

+
+/*
+ * 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

Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode

[Chao Yu]

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