| Message ID | 20200904075939.176366-1-lihao2018.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode() | expand |
On Fri, Sep 04, 2020 at 03:59:39PM +0800, Hao Li wrote: > If generic_drop_inode() returns true, it means iput_final() can evict > this inode regardless of whether it is dirty or not. If we check > I_DONTCACHE in generic_drop_inode(), any inode with this bit set will be > evicted unconditionally. This is not the desired behavior because > I_DONTCACHE only means the inode shouldn't be cached on the LRU list. > As for whether we need to evict this inode, this is what > generic_drop_inode() should do. This patch corrects the usage of > I_DONTCACHE. > > This patch was proposed in [1]. > > [1]: https://lore.kernel.org/linux-fsdevel/20200831003407.GE12096@dread.disaster.area/ > > Fixes: dae2f8ed7992 ("fs: Lift XFS_IDONTCACHE to the VFS layer") > Signed-off-by: Hao Li <lihao2018.fnst@cn.fujitsu.com> > --- > Changes in v2: > - Adjust code format > - Add Fixes tag in commit message > > fs/inode.c | 4 +++- > include/linux/fs.h | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) Looks good. Reviewed-by: Dave Chinner <dchinner@redhat.com>
On Fri, Sep 04, 2020 at 03:59:39PM +0800, Hao Li wrote: > If generic_drop_inode() returns true, it means iput_final() can evict > this inode regardless of whether it is dirty or not. If we check > I_DONTCACHE in generic_drop_inode(), any inode with this bit set will be > evicted unconditionally. This is not the desired behavior because > I_DONTCACHE only means the inode shouldn't be cached on the LRU list. > As for whether we need to evict this inode, this is what > generic_drop_inode() should do. This patch corrects the usage of > I_DONTCACHE. > > This patch was proposed in [1]. > > [1]: https://lore.kernel.org/linux-fsdevel/20200831003407.GE12096@dread.disaster.area/ > > Fixes: dae2f8ed7992 ("fs: Lift XFS_IDONTCACHE to the VFS layer") > Signed-off-by: Hao Li <lihao2018.fnst@cn.fujitsu.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > Changes in v2: > - Adjust code format > - Add Fixes tag in commit message > > fs/inode.c | 4 +++- > include/linux/fs.h | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 72c4c347afb7..19ad823f781c 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1625,7 +1625,9 @@ static void iput_final(struct inode *inode) > else > drop = generic_drop_inode(inode); > > - if (!drop && (sb->s_flags & SB_ACTIVE)) { > + if (!drop && > + !(inode->i_state & I_DONTCACHE) && > + (sb->s_flags & SB_ACTIVE)) { > inode_add_lru(inode); > spin_unlock(&inode->i_lock); > return; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index e019ea2f1347..93caee80ce47 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2922,8 +2922,7 @@ extern int inode_needs_sync(struct inode *inode); > extern int generic_delete_inode(struct inode *inode); > static inline int generic_drop_inode(struct inode *inode) > { > - return !inode->i_nlink || inode_unhashed(inode) || > - (inode->i_state & I_DONTCACHE); > + return !inode->i_nlink || inode_unhashed(inode); > } > extern void d_mark_dontcache(struct inode *inode); > > -- > 2.28.0 > > >
On Mon, Sep 07, 2020 at 07:40:02AM +1000, Dave Chinner wrote: > On Fri, Sep 04, 2020 at 03:59:39PM +0800, Hao Li wrote: > > If generic_drop_inode() returns true, it means iput_final() can evict > > this inode regardless of whether it is dirty or not. If we check > > I_DONTCACHE in generic_drop_inode(), any inode with this bit set will be > > evicted unconditionally. This is not the desired behavior because > > I_DONTCACHE only means the inode shouldn't be cached on the LRU list. > > As for whether we need to evict this inode, this is what > > generic_drop_inode() should do. This patch corrects the usage of > > I_DONTCACHE. > > > > This patch was proposed in [1]. > > > > [1]: https://lore.kernel.org/linux-fsdevel/20200831003407.GE12096@dread.disaster.area/ > > > > Fixes: dae2f8ed7992 ("fs: Lift XFS_IDONTCACHE to the VFS layer") > > Signed-off-by: Hao Li <lihao2018.fnst@cn.fujitsu.com> > > --- > > Changes in v2: > > - Adjust code format > > - Add Fixes tag in commit message > > > > fs/inode.c | 4 +++- > > include/linux/fs.h | 3 +-- > > 2 files changed, 4 insertions(+), 3 deletions(-) > > Looks good. > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > Hi, As discussed in [1], this patch is the basis of another one. Could I submit the second patch now to change the DCACHE_DONTCACHE behavior or I have to wait for this patch to be merged. [1]: https://lkml.org/lkml/2020/8/30/360 Thanks, Hao Li > -- > Dave Chinner > david@fromorbit.com > >
Hello, Ping. This patch need to be merged... Thanks. On 2020/9/9 7:03, Ira Weiny wrote: > On Fri, Sep 04, 2020 at 03:59:39PM +0800, Hao Li wrote: >> If generic_drop_inode() returns true, it means iput_final() can evict >> this inode regardless of whether it is dirty or not. If we check >> I_DONTCACHE in generic_drop_inode(), any inode with this bit set will be >> evicted unconditionally. This is not the desired behavior because >> I_DONTCACHE only means the inode shouldn't be cached on the LRU list. >> As for whether we need to evict this inode, this is what >> generic_drop_inode() should do. This patch corrects the usage of >> I_DONTCACHE. >> >> This patch was proposed in [1]. >> >> [1]: https://lore.kernel.org/linux-fsdevel/20200831003407.GE12096@dread.disaster.area/ >> >> Fixes: dae2f8ed7992 ("fs: Lift XFS_IDONTCACHE to the VFS layer") >> Signed-off-by: Hao Li <lihao2018.fnst@cn.fujitsu.com> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > >> --- >> Changes in v2: >> - Adjust code format >> - Add Fixes tag in commit message >> >> fs/inode.c | 4 +++- >> include/linux/fs.h | 3 +-- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/fs/inode.c b/fs/inode.c >> index 72c4c347afb7..19ad823f781c 100644 >> --- a/fs/inode.c >> +++ b/fs/inode.c >> @@ -1625,7 +1625,9 @@ static void iput_final(struct inode *inode) >> else >> drop = generic_drop_inode(inode); >> >> - if (!drop && (sb->s_flags & SB_ACTIVE)) { >> + if (!drop && >> + !(inode->i_state & I_DONTCACHE) && >> + (sb->s_flags & SB_ACTIVE)) { >> inode_add_lru(inode); >> spin_unlock(&inode->i_lock); >> return; >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index e019ea2f1347..93caee80ce47 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -2922,8 +2922,7 @@ extern int inode_needs_sync(struct inode *inode); >> extern int generic_delete_inode(struct inode *inode); >> static inline int generic_drop_inode(struct inode *inode) >> { >> - return !inode->i_nlink || inode_unhashed(inode) || >> - (inode->i_state & I_DONTCACHE); >> + return !inode->i_nlink || inode_unhashed(inode); >> } >> extern void d_mark_dontcache(struct inode *inode); >> >> -- >> 2.28.0 >> >> >> >
Ping :) On 2020/10/23 15:49, Li, Hao wrote: > Hello, > > Ping. > > This patch need to be merged... Thanks. > > On 2020/9/9 7:03, Ira Weiny wrote: >> On Fri, Sep 04, 2020 at 03:59:39PM +0800, Hao Li wrote: >>> If generic_drop_inode() returns true, it means iput_final() can evict >>> this inode regardless of whether it is dirty or not. If we check >>> I_DONTCACHE in generic_drop_inode(), any inode with this bit set will be >>> evicted unconditionally. This is not the desired behavior because >>> I_DONTCACHE only means the inode shouldn't be cached on the LRU list. >>> As for whether we need to evict this inode, this is what >>> generic_drop_inode() should do. This patch corrects the usage of >>> I_DONTCACHE. >>> >>> This patch was proposed in [1]. >>> >>> [1]: https://lore.kernel.org/linux-fsdevel/20200831003407.GE12096@dread.disaster.area/ >>> >>> Fixes: dae2f8ed7992 ("fs: Lift XFS_IDONTCACHE to the VFS layer") >>> Signed-off-by: Hao Li <lihao2018.fnst@cn.fujitsu.com> >> Reviewed-by: Ira Weiny <ira.weiny@intel.com> >> >>> --- >>> Changes in v2: >>> - Adjust code format >>> - Add Fixes tag in commit message >>> >>> fs/inode.c | 4 +++- >>> include/linux/fs.h | 3 +-- >>> 2 files changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/inode.c b/fs/inode.c >>> index 72c4c347afb7..19ad823f781c 100644 >>> --- a/fs/inode.c >>> +++ b/fs/inode.c >>> @@ -1625,7 +1625,9 @@ static void iput_final(struct inode *inode) >>> else >>> drop = generic_drop_inode(inode); >>> >>> - if (!drop && (sb->s_flags & SB_ACTIVE)) { >>> + if (!drop && >>> + !(inode->i_state & I_DONTCACHE) && >>> + (sb->s_flags & SB_ACTIVE)) { >>> inode_add_lru(inode); >>> spin_unlock(&inode->i_lock); >>> return; >>> diff --git a/include/linux/fs.h b/include/linux/fs.h >>> index e019ea2f1347..93caee80ce47 100644 >>> --- a/include/linux/fs.h >>> +++ b/include/linux/fs.h >>> @@ -2922,8 +2922,7 @@ extern int inode_needs_sync(struct inode *inode); >>> extern int generic_delete_inode(struct inode *inode); >>> static inline int generic_drop_inode(struct inode *inode) >>> { >>> - return !inode->i_nlink || inode_unhashed(inode) || >>> - (inode->i_state & I_DONTCACHE); >>> + return !inode->i_nlink || inode_unhashed(inode); >>> } >>> extern void d_mark_dontcache(struct inode *inode); >>> >>> -- >>> 2.28.0 >>> >>> >>> >
diff --git a/fs/inode.c b/fs/inode.c index 72c4c347afb7..19ad823f781c 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1625,7 +1625,9 @@ static void iput_final(struct inode *inode) else drop = generic_drop_inode(inode); - if (!drop && (sb->s_flags & SB_ACTIVE)) { + if (!drop && + !(inode->i_state & I_DONTCACHE) && + (sb->s_flags & SB_ACTIVE)) { inode_add_lru(inode); spin_unlock(&inode->i_lock); return; diff --git a/include/linux/fs.h b/include/linux/fs.h index e019ea2f1347..93caee80ce47 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2922,8 +2922,7 @@ extern int inode_needs_sync(struct inode *inode); extern int generic_delete_inode(struct inode *inode); static inline int generic_drop_inode(struct inode *inode) { - return !inode->i_nlink || inode_unhashed(inode) || - (inode->i_state & I_DONTCACHE); + return !inode->i_nlink || inode_unhashed(inode); } extern void d_mark_dontcache(struct inode *inode);
If generic_drop_inode() returns true, it means iput_final() can evict this inode regardless of whether it is dirty or not. If we check I_DONTCACHE in generic_drop_inode(), any inode with this bit set will be evicted unconditionally. This is not the desired behavior because I_DONTCACHE only means the inode shouldn't be cached on the LRU list. As for whether we need to evict this inode, this is what generic_drop_inode() should do. This patch corrects the usage of I_DONTCACHE. This patch was proposed in [1]. [1]: https://lore.kernel.org/linux-fsdevel/20200831003407.GE12096@dread.disaster.area/ Fixes: dae2f8ed7992 ("fs: Lift XFS_IDONTCACHE to the VFS layer") Signed-off-by: Hao Li <lihao2018.fnst@cn.fujitsu.com> --- Changes in v2: - Adjust code format - Add Fixes tag in commit message fs/inode.c | 4 +++- include/linux/fs.h | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-)