* [PATCH] f2fs: fix comment of f2fs_evict_inode @ 2019-09-25 9:30 Chao Yu 2019-09-25 13:47 ` [f2fs-dev] " Gao Xiang 2019-09-27 18:31 ` Jaegeuk Kim 0 siblings, 2 replies; 8+ messages in thread From: Chao Yu @ 2019-09-25 9:30 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode 2019-09-25 9:30 [PATCH] f2fs: fix comment of f2fs_evict_inode Chao Yu @ 2019-09-25 13:47 ` Gao Xiang 2019-09-26 1:04 ` Chao Yu 2019-09-27 18:31 ` Jaegeuk Kim 1 sibling, 1 reply; 8+ messages in thread From: Gao Xiang @ 2019-09-25 13:47 UTC (permalink / raw) To: Chao Yu; +Cc: jaegeuk, linux-kernel, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode 2019-09-25 13:47 ` [f2fs-dev] " Gao Xiang @ 2019-09-26 1:04 ` Chao Yu 0 siblings, 0 replies; 8+ messages in thread From: Chao Yu @ 2019-09-26 1:04 UTC (permalink / raw) To: Gao Xiang; +Cc: jaegeuk, linux-kernel, linux-f2fs-devel 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 > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: fix comment of f2fs_evict_inode 2019-09-25 9:30 [PATCH] f2fs: fix comment of f2fs_evict_inode Chao Yu 2019-09-25 13:47 ` [f2fs-dev] " Gao Xiang @ 2019-09-27 18:31 ` Jaegeuk Kim 2019-09-27 19:01 ` Gao Xiang 2019-09-29 0:53 ` Chao Yu 1 sibling, 2 replies; 8+ messages in thread From: Jaegeuk Kim @ 2019-09-27 18:31 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: fix comment of f2fs_evict_inode 2019-09-27 18:31 ` Jaegeuk Kim @ 2019-09-27 19:01 ` Gao Xiang 2019-09-29 0:53 ` Chao Yu 1 sibling, 0 replies; 8+ messages in thread From: Gao Xiang @ 2019-09-27 19:01 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel, chao 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: fix comment of f2fs_evict_inode 2019-09-27 18:31 ` Jaegeuk Kim 2019-09-27 19:01 ` Gao Xiang @ 2019-09-29 0:53 ` Chao Yu 2019-09-29 2:20 ` [f2fs-dev] " Gao Xiang 1 sibling, 1 reply; 8+ messages in thread From: Chao Yu @ 2019-09-29 0:53 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao 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 > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode 2019-09-29 0:53 ` Chao Yu @ 2019-09-29 2:20 ` Gao Xiang 2019-09-29 2:52 ` Chao Yu 0 siblings, 1 reply; 8+ messages in thread From: Gao Xiang @ 2019-09-29 2:20 UTC (permalink / raw) To: linux-f2fs-devel; +Cc: Chao Yu, Jaegeuk Kim, linux-kernel, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix comment of f2fs_evict_inode 2019-09-29 2:20 ` [f2fs-dev] " Gao Xiang @ 2019-09-29 2:52 ` Chao Yu 0 siblings, 0 replies; 8+ messages in thread From: Chao Yu @ 2019-09-29 2:52 UTC (permalink / raw) To: Gao Xiang, linux-f2fs-devel; +Cc: Jaegeuk Kim, linux-kernel 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 > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-29 2:57 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-25 9:30 [PATCH] f2fs: fix comment of f2fs_evict_inode Chao Yu 2019-09-25 13:47 ` [f2fs-dev] " Gao Xiang 2019-09-26 1:04 ` Chao Yu 2019-09-27 18:31 ` Jaegeuk Kim 2019-09-27 19:01 ` Gao Xiang 2019-09-29 0:53 ` Chao Yu 2019-09-29 2:20 ` [f2fs-dev] " Gao Xiang 2019-09-29 2:52 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).