Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 18 Nov 2019 12:00:47 +1100
From:   Dave Chinner <david@fromorbit.com>
To:     Brian Foster <bfoster@redhat.com>
Cc:     linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 28/28] xfs: rework unreferenced inode lookups
Message-ID: <20191118010047.GS4614@dread.disaster.area>
References: <20191031234618.15403-1-david@fromorbit.com>
 <20191031234618.15403-29-david@fromorbit.com>
 <20191106221846.GE37080@bfoster>
 <20191114221602.GJ4614@dread.disaster.area>
 <20191115172600.GC55854@bfoster>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191115172600.GC55854@bfoster>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 15, 2019 at 12:26:00PM -0500, Brian Foster wrote:
> On Fri, Nov 15, 2019 at 09:16:02AM +1100, Dave Chinner wrote:
> > On Wed, Nov 06, 2019 at 05:18:46PM -0500, Brian Foster wrote:
> > If so, most of this patch will go away....
> > 
> > > > +	 * attached to the buffer so we don't need to do anything more here.
> > > >  	 */
> > > > -	if (ip != free_ip) {
> > > > -		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
> > > > -			rcu_read_unlock();
> > > > -			delay(1);
> > > > -			goto retry;
> > > > -		}
> > > > -
> > > > -		/*
> > > > -		 * Check the inode number again in case we're racing with
> > > > -		 * freeing in xfs_reclaim_inode().  See the comments in that
> > > > -		 * function for more information as to why the initial check is
> > > > -		 * not sufficient.
> > > > -		 */
> > > > -		if (ip->i_ino != inum) {
> > > > +	if (__xfs_iflags_test(ip, XFS_ISTALE)) {
> > > 
> > > Is there a correctness reason for why we move the stale check to under
> > > ilock (in both iflush/ifree)?
> > 
> > It's under the i_flags_lock, and so I moved it up under the lookup
> > hold of the i_flags_lock so we don't need to cycle it again.
> > 
> 
> Yeah, but in both cases it looks like it moved to under the ilock as
> well, which comes after i_flags_lock. IOW, why grab ilock for stale
> inodes when we're just going to skip them?

Because I was worrying about serialising against reclaim before
changing the state of the inode. i.e. if the inode has already been
isolated by not yet disposed of, we shouldn't touch the inode state
at all. Serialisation against reclaim in this patch is via the
ILOCK, hence we need to do that before setting ISTALE....

IOWs, ISTALE is not protected by ILOCK, we just can't modify the
inode state until after we've gained the ILOCK to protect against
reclaim....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com