On Thu, 2017-12-14 at 13:17 +1100, Dave Chinner wrote:
> 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.
>
Will do.
FWIW, there's another reason to do it this way too: backward
compatibility. If we don't try to store the queried bit then we should
be able to go back and forth between legacy kernels and the ones with
the new i_version handling without any trouble. The older kernels will
just bump the count more frequently.
> > > > > 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.
>
>
Definitely. I'm finding that documenting this has been the hardest part.
Thanks for the review so far!
--
Jeff Layton <jlayton@xxxxxxxxxx>
