Re: [PATCH 14/19] xfs: convert to new i_version API

[Dave Chinner]

On Wed, Dec 13, 2017 at 07:10:22PM -0500, Jeff Layton wrote:
> On Thu, 2017-12-14 at 10:25 +1100, Dave Chinner wrote:
> > So now I've looked at the last patch .....
> > 
> > On Thu, Dec 14, 2017 at 09:48:37AM +1100, Dave Chinner wrote:
> > > On Wed, Dec 13, 2017 at 09:20:12AM -0500, Jeff Layton wrote:
> > > > From: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_inode_buf.c | 5 +++--
> > > >  fs/xfs/xfs_icache.c           | 4 ++--
> > > >  fs/xfs/xfs_inode.c            | 2 +-
> > > >  fs/xfs/xfs_inode_item.c       | 2 +-
> > > >  fs/xfs/xfs_trans_inode.c      | 2 +-
> > > >  5 files changed, 8 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > > > index 6b7989038d75..6b47de201391 100644
> > > > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > > > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > > > @@ -264,7 +264,8 @@ xfs_inode_from_disk(
> > > >  	to->di_flags	= be16_to_cpu(from->di_flags);
> > > >  
> > > >  	if (to->di_version == 3) {
> > > > -		inode->i_version = be64_to_cpu(from->di_changecount);
> > > > +		inode_set_iversion_queried(inode,
> > > > +					   be64_to_cpu(from->di_changecount));
> > > 
> > > So we use the "kernel managed" (really not sure what that means)
> > > set function here to read it off disk, but...
> > 
> > This stores the value from disk in the incore inode as "val << 1",
> > then sets the lowest bit to indicate that it has been "queried"
> > so that it will be incremented on the first modification.
> > 
> > Why do we initialise values read from disk as "queried"? This means
> > the i_version will change once every time it's brought into memory
> > and modified, regardless of whether anyone is looking at it. What
> > purpose does this serve?
> > 
> 
> I don't think we want to store the QUERIED bit.
> 
> It's always possible that we crash at an inopportune time and a query
> happened vs. this value before this thing hit the backing store.
> 
> If we always set the queried bit when we load it from disk, then we know
> that that scenario is harmless, at the negligible expense of having to
> bump it on the first write.

Reasonable. Needs documentation.

> > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > index 801274126648..be6d87980dd5 100644
> > > > --- a/fs/xfs/xfs_inode.c
> > > > +++ b/fs/xfs/xfs_inode.c
> > > > @@ -833,7 +833,7 @@ xfs_ialloc(
> > > >  	ip->i_d.di_flags = 0;
> > > >  
> > > >  	if (ip->i_d.di_version == 3) {
> > > > -		inode->i_version = 1;
> > > > +		inode_set_iversion(inode, 1);
> > > 
> > > But here you are using the "filesystem managed" mdoe to set the
> > > new value. Why? How is this any different from reading the value
> > > off disk and setting it?
> > 
> > Still don't understand why this is different to reading the inode
> > from disk....
> 
> This is a allocating a brand new, never before seen inode. There's no
> way this i_version could have ever been seen, so there's no need to flag
> it as queried.

More documentation. People are going to need to know this stuff to
be able to implement/maintain this stuff in working order - it's no
longer a simple, obvious "just increment the counter on
modification" variable and that has potential ramifications for
filesystems that store this on disk.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx