2020-04-14 13:19:27

by Darrick J. Wong

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

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

CACNE -> CACHE.

On Sun, Apr 12, 2020 at 10:40:44PM -0700, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>


2020-04-14 13:20:01

by Darrick J. Wong

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

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, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
> >

2020-04-14 22:16:45

by Ira Weiny

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

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, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
> >