From: Ira Weiny <[email protected]>
xfs_reinit_inode() -> inode_init_always() already handles calling
init_rwsem(i_rwsem). Doing so again is unneeded.
Signed-off-by: Ira Weiny <[email protected]>
---
New for V4:
NOTE: This was found while ensuring the new i_aops_sem was properly
handled. It seems like this is a layering violation so I think it is
worth cleaning up so as to not confuse others.
---
fs/xfs/xfs_icache.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 8dc2e5414276..836a1f09be03 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -419,6 +419,7 @@ xfs_iget_cache_hit(
spin_unlock(&ip->i_flags_lock);
rcu_read_unlock();
+ ASSERT(!rwsem_is_locked(&inode->i_rwsem));
error = xfs_reinit_inode(mp, inode);
if (error) {
bool wake;
@@ -452,9 +453,6 @@ xfs_iget_cache_hit(
ip->i_sick = 0;
ip->i_checked = 0;
- ASSERT(!rwsem_is_locked(&inode->i_rwsem));
- init_rwsem(&inode->i_rwsem);
-
spin_unlock(&ip->i_flags_lock);
spin_unlock(&pag->pag_ici_lock);
} else {
--
2.21.0
On Thu, Feb 20, 2020 at 04:41:22PM -0800, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> xfs_reinit_inode() -> inode_init_always() already handles calling
> init_rwsem(i_rwsem). Doing so again is unneeded.
>
> Signed-off-by: Ira Weiny <[email protected]>
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.
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Fri, Feb 21, 2020 at 12:26:25PM +1100, Dave Chinner wrote:
> On Thu, Feb 20, 2020 at 04:41:22PM -0800, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > xfs_reinit_inode() -> inode_init_always() already handles calling
> > init_rwsem(i_rwsem). Doing so again is unneeded.
> >
> > Signed-off-by: Ira Weiny <[email protected]>
>
> 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
> [email protected]