linux-kernel - Re: [PATCH 14/19] xfs: convert to new i

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1513250179.3504.2.camel@kernel.org>
Date:   Thu, 14 Dec 2017 06:16:19 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     Dave Chinner <david@...morbit.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        hch@....de, neilb@...e.de, bfields@...ldses.org,
        amir73il@...il.com, jack@...e.de, viro@...iv.linux.org.uk
Subject: Re: [PATCH 14/19] xfs: convert to new i_version API

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@...hat.com>
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@...hat.com>
> > > > > ---
> > > > >  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@...nel.org>