On Fri, Oct 11, 2019 at 05:55:22AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 02:21:23PM +1100, Dave Chinner wrote:
> > 4. it xfs_ilock_nowait() fails until the rcu grace period
>
> Should this be:
>
> > 4. if xfs_ilock_nowait() fails before the rcu grace period
>
> ?
>
> > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > kmem_zone_free(xfs_inode_zone, ip);
> > }
> >
> > @@ -131,6 +132,7 @@ xfs_inode_free(
> > * free state. The ip->i_flags_lock provides the barrier against lookup
> > * races.
> > */
> > + xfs_ilock(ip, XFS_ILOCK_EXCL);
>
> This introduceѕ a non-owner unlock of an exclusively held rwsem. As-is
> this will make lockdep very unhappy. We have a non-owner unlock version
> of up_read, but not of up_write currently. I'm also not sure if those
> are allowed from RCU callback, which IIRC can run from softirq context.
>
> That being said this scheme of only unlocking the inode in the rcu free
> callback makes totaly sense to me, so I wish we can accomodate it
> somehow.
AFAICT it is safe to do this. Lockdep just needs to be bashed about
the head a bit to make it shut up.
> > @@ -312,7 +327,8 @@ xfs_iget_cache_hit(
> > rcu_read_lock();
> > spin_lock(&ip->i_flags_lock);
> > wake = !!__xfs_iflags_test(ip, XFS_INEW);
> > - ip->i_flags &= ~(XFS_INEW | XFS_IRECLAIM);
> > + ip->i_flags &= ~XFS_INEW | XFS_IRECLAIM;
>
> This change looks wrong to me, or did I miss something? We now
> clear all bits that are not XFS_I_NEW and XFS_IRECLAIM, which
> already is set in ~XFS_INEW. So if that was the intent just:
>
> ip->i_flags &= ~XFS_INEW;
Nah, I screwed up backing out a change. This line should not be
modified at all.
>
> > + * This requires code that requires such pins to do the following under a single
>
> This adds an > 80 char line. (there are a few more below.
>
> > + /* push the AIL to clean dirty reclaimable inodes */
> > + xfs_ail_push_all(mp->m_ail);
> > +
> > + /* push the AIL to clean dirty reclaimable inodes */
> > + xfs_ail_push_all(mp->m_ail);
> > +
>
> This looks spurious vs the rest of the patch.
Looks like rebase failure fallout. I must have missed it on
cleanup. I'll sort that out.
> > + if (__xfs_iflags_test(ip, XFS_ISTALE)) {
> > + spin_unlock(&ip->i_flags_lock);
> > + if (ip != free_ip)
> > xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > - }
> > + rcu_read_unlock();
> > + continue;
>
> This unlock out of order. Should be harmless, but also pointless.
>
> I think this code would be a lot easier to understand if we fatored
> this inner loop into a new helper. Untested patch that does, and
> also removes a no incorrect comment below:
*nod*
I'll put a refacting patch at the start of the series to split this
into separate code movement and algorithm modification patches....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
