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:   Wed, 20 Nov 2019 07:42:24 -0500
From:   Brian Foster <bfoster@redhat.com>
To:     Dave Chinner <david@fromorbit.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: <20191120124224.GA15542@bfoster>
References: <20191031234618.15403-1-david@fromorbit.com>
 <20191031234618.15403-29-david@fromorbit.com>
 <20191106221846.GE37080@bfoster>
 <20191114221602.GJ4614@dread.disaster.area>
 <20191115172600.GC55854@bfoster>
 <20191118010047.GS4614@dread.disaster.area>
 <20191119151344.GD10763@bfoster>
 <20191119211834.GA4614@dread.disaster.area>
MIME-Version: 1.0
In-Reply-To: <20191119211834.GA4614@dread.disaster.area>
User-Agent: Mutt/1.12.1 (2019-06-15)
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 20, 2019 at 08:18:34AM +1100, Dave Chinner wrote:
> On Tue, Nov 19, 2019 at 10:13:44AM -0500, Brian Foster wrote:
> > On Mon, Nov 18, 2019 at 12:00:47PM +1100, Dave Chinner wrote:
> > > 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....
> > > > >=20
> > > > > > > +=09 * attached to the buffer so we don't need to do anything=
 more here.
> > > > > > >  =09 */
> > > > > > > -=09if (ip !=3D free_ip) {
> > > > > > > -=09=09if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
> > > > > > > -=09=09=09rcu_read_unlock();
> > > > > > > -=09=09=09delay(1);
> > > > > > > -=09=09=09goto retry;
> > > > > > > -=09=09}
> > > > > > > -
> > > > > > > -=09=09/*
> > > > > > > -=09=09 * Check the inode number again in case we're racing w=
ith
> > > > > > > -=09=09 * freeing in xfs_reclaim_inode().  See the comments i=
n that
> > > > > > > -=09=09 * function for more information as to why the initial=
 check is
> > > > > > > -=09=09 * not sufficient.
> > > > > > > -=09=09 */
> > > > > > > -=09=09if (ip->i_ino !=3D inum) {
> > > > > > > +=09if (__xfs_iflags_test(ip, XFS_ISTALE)) {
> > > > > >=20
> > > > > > Is there a correctness reason for why we move the stale check t=
o under
> > > > > > ilock (in both iflush/ifree)?
> > > > >=20
> > > > > It's under the i_flags_lock, and so I moved it up under the looku=
p
> > > > > hold of the i_flags_lock so we don't need to cycle it again.
> > > > >=20
> > > >=20
> > > > Yeah, but in both cases it looks like it moved to under the ilock a=
s
> > > > well, which comes after i_flags_lock. IOW, why grab ilock for stale
> > > > inodes when we're just going to skip them?
> > >=20
> > > 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....
> > >=20
> >=20
> > Yeah, I think my question still isn't clear... I'm not talking about
> > setting ISTALE. The code I referenced above is where we test for it and
> > skip the inode if it is already set. For example, the code referenced
> > above in xfs_ifree_get_one_inode() currently does the following with
> > respect to i_flags_lock, ILOCK and XFS_ISTALE:
> >=20
> > =09...
> > =09spin_lock(i_flags_lock)
> > =09xfs_ilock_nowait(XFS_ILOCK_EXCL)
> > =09if !XFS_ISTALE
> > =09=09skip
> > =09set XFS_ISTALE
> > =09...
>=20
> There is another place in xfs_ifree_cluster that sets ISTALE without
> the ILOCK held, so the ILOCK is being used here for a different
> purpose...
>=20
> > The reclaim isolate code does this, however:
> >=20
> > =09spin_trylock(i_flags_lock)
> > =09if !XFS_ISTALE
> > =09=09skip
> > =09xfs_ilock(XFS_ILOCK_EXCL)
> > =09...=09
>=20
> Which is fine, because we're not trying to avoid racing with reclaim
> here. :) i.e. all we need is the i_flags lock to check the ISTALE
> flag safely.
>=20
> > So my question is why not do something like the following in the
> > _get_one_inode() case?
> >=20
> > =09...
> > =09spin_lock(i_flags_lock)
> > =09if !XFS_ISTALE
> > =09=09skip
> > =09xfs_ilock_nowait(XFS_ILOCK_EXCL)
> > =09set XFS_ISTALE
> > =09...
>=20
> Because, like I said, I focussed on the lookup racing with reclaim
> first. The above code could be used, but it puts object internal
> state checks before we really know whether the object is safe to
> access and whether we can trust it.
>=20
> I'm just following a basic RCU/lockless lookup principle here:
> don't try to use object state before you've fully validated that the
> object is live and guaranteed that it can be safely referenced.
>=20
> > IOW, what is the need, if any, to acquire ilock in the iflush/ifree
> > paths before testing for XFS_ISTALE? Is there some specific intermediat=
e
> > state I'm missing or is this just unintentional?
>=20
> It's entirely intentional - validate and claim the object we've
> found in the lockless lookup, then run the code that checks/changes
> the object state. Smashing state checks and lockless lookup
> validation together is a nasty landmine to leave behind...
>=20

Ok, so this is intentional, but the purpose is simplification vs.
technically being part of the lookup dance. I'm not sure I see the
advantage given that IMO this trades off one landmine for another, but
I'm not worried that much about it as long as the code is correct.

I guess we'll see how things change after reevaluation of the whole
holding ilock across contexts behavior, but if we do end up with a
similar pattern in the iflush/ifree paths please document that
explicitly in the comments. Otherwise in a patch that swizzles this code
around and explicitly plays games with ilock, the intent of this
particular change is not clear to somebody reading the code IMO. In
fact, I think it might be interesting to see if we could define a couple
helpers (located closer to the reclaim code) to perform an unreferenced
lookup/release of an inode, but that is secondary to nailing down the
fundamental rules.

Brian

> Cheers,
>=20
> Dave.
> --=20
> Dave Chinner
> david@fromorbit.com
>=20