On Fri, Feb 21, 2020 at 12:26:25PM +1100, Dave Chinner wrote:
> On Thu, Feb 20, 2020 at 04:41:22PM -0800, ira.weiny@xxxxxxxxx wrote:
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > xfs_reinit_inode() -> inode_init_always() already handles calling
> > init_rwsem(i_rwsem). Doing so again is unneeded.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
>
> Except that this inode has been destroyed and freed by the VFS, and
> we are now recycling it back into the VFS before we actually
> physically freed it.
>
> Hence we have re-initialise the semaphore because the semaphore can
> contain internal state that is specific to it's new life cycle (e.g.
> the lockdep context) that will cause problems if we just assume that
> the inode is the same inode as it was before we recycled it back
> into the VFS caches.
>
> So, yes, we actually do need to re-initialise the rwsem here.
I'm fine dropping the patch...
But I'm not following how the xfs_reinit_inode() on line 422 does not take care
of this?
fs/xfs/xfs_icache.c:
422 error = xfs_reinit_inode(mp, inode);
423 if (error) {
424 bool wake;
425 /*
426 * Re-initializing the inode failed, and we are in deep
427 * trouble. Try to re-add it to the reclaim list.
428 */
429 rcu_read_lock();
430 spin_lock(&ip->i_flags_lock);
431 wake = !!__xfs_iflags_test(ip, XFS_INEW);
432 ip->i_flags &= ~(XFS_INEW | XFS_IRECLAIM);
433 if (wake)
434 wake_up_bit(&ip->i_flags, __XFS_INEW_BIT);
435 ASSERT(ip->i_flags & XFS_IRECLAIMABLE);
436 trace_xfs_iget_reclaim_fail(ip);
437 goto out_error;
438 }
439
440 spin_lock(&pag->pag_ici_lock);
441 spin_lock(&ip->i_flags_lock);
442
443 /*
444 * Clear the per-lifetime state in the inode as we are now
445 * effectively a new inode and need to return to the initial
446 * state before reuse occurs.
447 */
448 ip->i_flags &= ~XFS_IRECLAIM_RESET_FLAGS;
449 ip->i_flags |= XFS_INEW;
450 xfs_inode_clear_reclaim_tag(pag, ip->i_ino);
451 inode->i_state = I_NEW;
452 ip->i_sick = 0;
453 ip->i_checked = 0;
454
455 ASSERT(!rwsem_is_locked(&inode->i_rwsem));
456 init_rwsem(&inode->i_rwsem);
457
458 spin_unlock(&ip->i_flags_lock);
459 spin_unlock(&pag->pag_ici_lock);
Does this need to be done under one of these locks?
Ira
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
