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

[Jan Kara]

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@xxxxxxxxx wrote:
> > > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > 
> > > 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@xxxxxxxxx>
> > 
> > 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@xxxxxxxx>
> > SUSE Labs, CR
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR