Re: [PATCH 4/4] xfs: reactivate XFS_NEED_INACTIVE inodes from xfs_iget

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Mar 20, 2024 at 09:39:57AM +0100, Andre Noll wrote:
> On Tue, Mar 19, 11:16, Dave Chinner wrote
> > +		/*
> > +		 * Well, that sucks. Put the inode back on the inactive queue.
> > +		 * Do this while still under the ILOCK so that we can set the
> > +		 * NEED_INACTIVE flag and clear the INACTIVATING flag an not
> > +		 * have another lookup race with us before we've finished
> > +		 * putting the inode back on the inodegc queue.
> > +		 */
> > +		spin_unlock(&ip->i_flags_lock);
> > +		ip->i_flags |= XFS_NEED_INACTIVE;
> > +		ip->i_flags &= ~XFS_INACTIVATING;
> > +		spin_unlock(&ip->i_flags_lock);
> 
> This doesn't look right. Shouldn't the first spin_unlock() be spin_lock()?

Yes.  So much for my hand inspection of code. :(

(Doesn't simple lock debugging catch these sorts of things?)

((It sure would be nice if locking returned a droppable "object" to do
the unlock ala Rust and then spin_lock could be __must_check.))

--D

> Also, there's a typo in the comment (s/an/and).
> Best
> Andre
> -- 
> Max Planck Institute for Biology
> Tel: (+49) 7071 601 829
> Max-Planck-Ring 5, 72076 Tübingen, Germany
> http://people.tuebingen.mpg.de/maan/