On Wed, Mar 20, 07:53, Darrick J. Wong wrote > 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. :( Given enough hand inspections, all bugs are shallow :) > (Doesn't simple lock debugging catch these sorts of things?) Maybe this error path doesn't get exercised because xfs_reinit_inode() never fails. AFAICT, it can only fail if security_inode_alloc() can't allocate the composite inode blob. > ((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.)) There's the *LOCK_GUARD* macros which employ gcc's cleanup attribute to automatically call e.g. spin_unlock() when a variable goes out of scope (see 54da6a0924311). 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/
Attachment:
signature.asc
Description: PGP signature
