[V7,7/9] fs: Define I_DONTCACNE in VFS layer

From: Ira Weiny <ira.weiny@intel.com>

DAX effective mode changes (setting of S_DAX) require inode eviction.

Define a flag which can be set to inform the VFS layer that inodes
should not be cached.  This will expedite the eviction of those nodes
requiring reload.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 include/linux/fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

> Subject: [PATCH V7 7/9] fs: Define I_DONTCACNE in VFS layer

CACNE -> CACHE.

On Sun, Apr 12, 2020 at 10:40:44PM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> DAX effective mode changes (setting of S_DAX) require inode eviction.
> 
> Define a flag which can be set to inform the VFS layer that inodes
> should not be cached.  This will expedite the eviction of those nodes
> requiring reload.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  include/linux/fs.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a818ced22961..e2db71d150c3 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2151,6 +2151,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
>   *
>   * I_CREATING		New object's inode in the middle of setting up.
>   *
> + * I_DONTCACHE		Do not cache the inode

"Do not cache" is a bit vague, how about:

"Evict the inode when the last reference is dropped.
Do not put it on the LRU list."

Also, shouldn't xfs_ioctl_setattr be setting I_DONTCACHE if someone
changes FS_XFLAG_DAX (and there are no mount option overrides)?  I don't
see any user of I_DONTCACHE in this series.

(Also also, please convert XFS_IDONTCACHE, since it's a straightforward
conversion...)

--D

> + *
>   * Q: What is the difference between I_WILL_FREE and I_FREEING?
>   */
>  #define I_DIRTY_SYNC		(1 << 0)
> @@ -2173,6 +2175,7 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
>  #define I_WB_SWITCH		(1 << 13)
>  #define I_OVL_INUSE		(1 << 14)
>  #define I_CREATING		(1 << 15)
> +#define I_DONTCACHE		(1 << 16)
>  
>  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
>  #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> @@ -3042,7 +3045,8 @@ 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);
> +	return !inode->i_nlink || inode_unhashed(inode) ||
> +		(inode->i_state & I_DONTCACHE);
>  }
>  
>  extern struct inode *ilookup5_nowait(struct super_block *sb,
> -- 
> 2.25.1
>

On Mon, Apr 13, 2020 at 09:09:29AM -0700, Darrick J. Wong wrote:
> > Subject: [PATCH V7 7/9] fs: Define I_DONTCACNE in VFS layer
> 
> CACNE -> CACHE.
> 
> On Sun, Apr 12, 2020 at 10:40:44PM -0700, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > DAX effective mode changes (setting of S_DAX) require inode eviction.
> > 
> > Define a flag which can be set to inform the VFS layer that inodes
> > should not be cached.  This will expedite the eviction of those nodes
> > requiring reload.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  include/linux/fs.h | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index a818ced22961..e2db71d150c3 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2151,6 +2151,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >   *
> >   * I_CREATING		New object's inode in the middle of setting up.
> >   *
> > + * I_DONTCACHE		Do not cache the inode
> 
> "Do not cache" is a bit vague, how about:
> 
> "Evict the inode when the last reference is dropped.
> Do not put it on the LRU list."
> 
> Also, shouldn't xfs_ioctl_setattr be setting I_DONTCACHE if someone
> changes FS_XFLAG_DAX (and there are no mount option overrides)?  I don't
> see any user of I_DONTCACHE in this series.

Oops, brain fart, ignore this question ^^^^.

--D

> (Also also, please convert XFS_IDONTCACHE, since it's a straightforward
> conversion...)
> 
> --D
> 
> > + *
> >   * Q: What is the difference between I_WILL_FREE and I_FREEING?
> >   */
> >  #define I_DIRTY_SYNC		(1 << 0)
> > @@ -2173,6 +2175,7 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >  #define I_WB_SWITCH		(1 << 13)
> >  #define I_OVL_INUSE		(1 << 14)
> >  #define I_CREATING		(1 << 15)
> > +#define I_DONTCACHE		(1 << 16)
> >  
> >  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> >  #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> > @@ -3042,7 +3045,8 @@ 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);
> > +	return !inode->i_nlink || inode_unhashed(inode) ||
> > +		(inode->i_state & I_DONTCACHE);
> >  }
> >  
> >  extern struct inode *ilookup5_nowait(struct super_block *sb,
> > -- 
> > 2.25.1
> >

On Mon, Apr 13, 2020 at 09:09:29AM -0700, Darrick J. Wong wrote:
> > Subject: [PATCH V7 7/9] fs: Define I_DONTCACNE in VFS layer
> 
> CACNE -> CACHE.
> 
> On Sun, Apr 12, 2020 at 10:40:44PM -0700, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > DAX effective mode changes (setting of S_DAX) require inode eviction.
> > 
> > Define a flag which can be set to inform the VFS layer that inodes
> > should not be cached.  This will expedite the eviction of those nodes
> > requiring reload.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  include/linux/fs.h | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index a818ced22961..e2db71d150c3 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2151,6 +2151,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >   *
> >   * I_CREATING		New object's inode in the middle of setting up.
> >   *
> > + * I_DONTCACHE		Do not cache the inode
> 
> "Do not cache" is a bit vague, how about:
> 
> "Evict the inode when the last reference is dropped.
> Do not put it on the LRU list."
> 
> Also, shouldn't xfs_ioctl_setattr be setting I_DONTCACHE if someone
> changes FS_XFLAG_DAX (and there are no mount option overrides)?  I don't
> see any user of I_DONTCACHE in this series.
> 
> (Also also, please convert XFS_IDONTCACHE, since it's a straightforward
> conversion...)

AFAICT XFS_IDONTCACHE is not exactly the same because it can be cleared if
someone access' the inode before it is evicted.  Dave mentioned that we could
probably do this but I was not 100% sure if that would change some other
behavior.

I'm happy to remove XFS_IDONTCACHE if we are sure that it will not regress
something in the bulkstat code?  (I don't know exactly what bulkstat does so
I'm not expert here...  Was just doing what seemed safest)

Ira

> 
> --D
> 
> > + *
> >   * Q: What is the difference between I_WILL_FREE and I_FREEING?
> >   */
> >  #define I_DIRTY_SYNC		(1 << 0)
> > @@ -2173,6 +2175,7 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >  #define I_WB_SWITCH		(1 << 13)
> >  #define I_OVL_INUSE		(1 << 14)
> >  #define I_CREATING		(1 << 15)
> > +#define I_DONTCACHE		(1 << 16)
> >  
> >  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> >  #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> > @@ -3042,7 +3045,8 @@ 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);
> > +	return !inode->i_nlink || inode_unhashed(inode) ||
> > +		(inode->i_state & I_DONTCACHE);
> >  }
> >  
> >  extern struct inode *ilookup5_nowait(struct super_block *sb,
> > -- 
> > 2.25.1
> >

On Sun 12-04-20 22:40:44, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> DAX effective mode changes (setting of S_DAX) require inode eviction.
> 
> Define a flag which can be set to inform the VFS layer that inodes
> should not be cached.  This will expedite the eviction of those nodes
> requiring reload.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

This inode flag will have a limited impact because usually dentry will
still hold inode reference. So until dentry is evicted, inode stays as
well. So I think we'd need something like DCACHE_DONTCACHE flag as well to
discard a dentry whenever dentry usecount hits zero (which will be
generally on last file close). What do you think?

And I'd note that checking for I_DONTCACHE flag in dput() isn't
straightforward because of locking so that's why I suggest separate dentry
flag.

								Honza

> ---
>  include/linux/fs.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a818ced22961..e2db71d150c3 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2151,6 +2151,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
>   *
>   * I_CREATING		New object's inode in the middle of setting up.
>   *
> + * I_DONTCACHE		Do not cache the inode
> + *
>   * Q: What is the difference between I_WILL_FREE and I_FREEING?
>   */
>  #define I_DIRTY_SYNC		(1 << 0)
> @@ -2173,6 +2175,7 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
>  #define I_WB_SWITCH		(1 << 13)
>  #define I_OVL_INUSE		(1 << 14)
>  #define I_CREATING		(1 << 15)
> +#define I_DONTCACHE		(1 << 16)
>  
>  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
>  #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> @@ -3042,7 +3045,8 @@ 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);
> +	return !inode->i_nlink || inode_unhashed(inode) ||
> +		(inode->i_state & I_DONTCACHE);
>  }
>  
>  extern struct inode *ilookup5_nowait(struct super_block *sb,
> -- 
> 2.25.1
>

On Tue, Apr 14, 2020 at 05:26:30PM +0200, Jan Kara wrote:
> On Sun 12-04-20 22:40:44, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > DAX effective mode changes (setting of S_DAX) require inode eviction.
> > 
> > Define a flag which can be set to inform the VFS layer that inodes
> > should not be cached.  This will expedite the eviction of those nodes
> > requiring reload.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> This inode flag will have a limited impact because usually dentry will
> still hold inode reference. So until dentry is evicted, inode stays as
> well.

Agreed but at least this keeps the inode from being cached until that time.

FWIW the ext4 patches seem to have a much longer delay when issuing drop_caches
and I'm not 100% sure why.  I've sent out those patches RFC to get the
discussions started.  I feel like I have missed something there but it does
eventually flip the S_DAX flag.

> So I think we'd need something like DCACHE_DONTCACHE flag as well to
> discard a dentry whenever dentry usecount hits zero (which will be
> generally on last file close). What do you think?

I wanted to do something like this but I was not sure how to trigger the
DCACHE_DONTCACHE on the correct 'parent' dentry.  Can't their be multiple
dentries pointing to the same inode?

In which case, would you need to flag them all?

Ira

> 
> And I'd note that checking for I_DONTCACHE flag in dput() isn't
> straightforward because of locking so that's why I suggest separate dentry
> flag.
> 
> 								Honza
> 
> > ---
> >  include/linux/fs.h | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index a818ced22961..e2db71d150c3 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2151,6 +2151,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >   *
> >   * I_CREATING		New object's inode in the middle of setting up.
> >   *
> > + * I_DONTCACHE		Do not cache the inode
> > + *
> >   * Q: What is the difference between I_WILL_FREE and I_FREEING?
> >   */
> >  #define I_DIRTY_SYNC		(1 << 0)
> > @@ -2173,6 +2175,7 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >  #define I_WB_SWITCH		(1 << 13)
> >  #define I_OVL_INUSE		(1 << 14)
> >  #define I_CREATING		(1 << 15)
> > +#define I_DONTCACHE		(1 << 16)
> >  
> >  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> >  #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> > @@ -3042,7 +3045,8 @@ 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);
> > +	return !inode->i_nlink || inode_unhashed(inode) ||
> > +		(inode->i_state & I_DONTCACHE);
> >  }
> >  
> >  extern struct inode *ilookup5_nowait(struct super_block *sb,
> > -- 
> > 2.25.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

On Tue 14-04-20 08:45:01, Ira Weiny wrote:
> On Tue, Apr 14, 2020 at 05:26:30PM +0200, Jan Kara wrote:
> > On Sun 12-04-20 22:40:44, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > DAX effective mode changes (setting of S_DAX) require inode eviction.
> > > 
> > > Define a flag which can be set to inform the VFS layer that inodes
> > > should not be cached.  This will expedite the eviction of those nodes
> > > requiring reload.
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > This inode flag will have a limited impact because usually dentry will
> > still hold inode reference. So until dentry is evicted, inode stays as
> > well.
> 
> Agreed but at least this keeps the inode from being cached until that time.
> 
> FWIW the ext4 patches seem to have a much longer delay when issuing drop_caches
> and I'm not 100% sure why.  I've sent out those patches RFC to get the
> discussions started.  I feel like I have missed something there but it does
> eventually flip the S_DAX flag.
> 
> > So I think we'd need something like DCACHE_DONTCACHE flag as well to
> > discard a dentry whenever dentry usecount hits zero (which will be
> > generally on last file close). What do you think?
> 
> I wanted to do something like this but I was not sure how to trigger the
> DCACHE_DONTCACHE on the correct 'parent' dentry.  Can't their be multiple
> dentries pointing to the same inode?
> 
> In which case, would you need to flag them all?

There can be multiple dentries in case there are hardlinks. There can be
also multiple entries in case the filesystem is NFS-exported and there are
some disconnected dentries (those will however get discarded automatically
once they are unused). You could actually iterate the list of all dentries
(they are all part of inode->i_dentry list) and mark them all. This would
still miss the case if there are more hardlinks and a dentry for a new link
gets instantiated later but I guess I would not bother with this
cornercase.

								Honza

> > And I'd note that checking for I_DONTCACHE flag in dput() isn't
> > straightforward because of locking so that's why I suggest separate dentry
> > flag.
> > 
> > 								Honza
> > 
> > > ---
> > >  include/linux/fs.h | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index a818ced22961..e2db71d150c3 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2151,6 +2151,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> > >   *
> > >   * I_CREATING		New object's inode in the middle of setting up.
> > >   *
> > > + * I_DONTCACHE		Do not cache the inode
> > > + *
> > >   * Q: What is the difference between I_WILL_FREE and I_FREEING?
> > >   */
> > >  #define I_DIRTY_SYNC		(1 << 0)
> > > @@ -2173,6 +2175,7 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> > >  #define I_WB_SWITCH		(1 << 13)
> > >  #define I_OVL_INUSE		(1 << 14)
> > >  #define I_CREATING		(1 << 15)
> > > +#define I_DONTCACHE		(1 << 16)
> > >  
> > >  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> > >  #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> > > @@ -3042,7 +3045,8 @@ 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);
> > > +	return !inode->i_nlink || inode_unhashed(inode) ||
> > > +		(inode->i_state & I_DONTCACHE);
> > >  }
> > >  
> > >  extern struct inode *ilookup5_nowait(struct super_block *sb,
> > > -- 
> > > 2.25.1
> > > 
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR