Re: [PATCH] Unlock inode before calling xfs_idestroy()

[Lachlan McIlroy <lachlan@xxxxxxx>]

Timothy Shimmin wrote:
> Lachlan McIlroy wrote:
> > Lock debugging reported the ilock was being destroyed
> > without being unlocked.
> >
> > --- a/fs/xfs/xfs_iget.c    2008-09-19 13:03:57.000000000 +1000
> > +++ b/fs/xfs/xfs_iget.c    2008-09-19 13:12:38.000000000 +1000
> > @@ -214,6 +214,7 @@ finish_inode:
> >         xfs_ilock(ip, lock_flags);
> >
> >     if ((ip->i_d.di_mode == 0) && !(flags & XFS_IGET_CREATE)) {
> > +        xfs_iunlock(ip, lock_flags);
> >         xfs_idestroy(ip);
> >         xfs_put_perag(mp, pag);
> >         return ENOENT;
> > @@ -224,6 +225,7 @@ finish_inode:
> >      * write spinlock.
> >      */
> >     if (radix_tree_preload(GFP_KERNEL)) {
> > +        xfs_iunlock(ip, lock_flags);
> >         xfs_idestroy(ip);
> >         delay(1);
> >         goto again;
> > @@ -239,6 +241,7 @@ finish_inode:
> >         BUG_ON(error != -EEXIST);
> >         write_unlock(&pag->pag_ici_lock);
> >         radix_tree_preload_end();
> > +        xfs_iunlock(ip, lock_flags);
> >         xfs_idestroy(ip);
> >         XFS_STATS_INC(xs_ig_dup);
> >         goto again;
>
> I'm just wondering about the case where lock_flags==0
> and the inode is not locked.
> I think it would fail an assert in xfs_iunlock().
Good catch Tim, thanks.

> --Tim
>
>
>     if (lock_flags)
>                 xfs_ilock(ip, lock_flags);
>
>         if ((ip->i_d.di_mode == 0) && !(flags & XFS_IGET_CREATE)) {
>                 xfs_iunlock(ip, lock_flags);
>                 xfs_idestroy(ip);
>                 xfs_put_perag(mp, pag);
>                 return ENOENT;
>         }
>
> void
> xfs_iunlock(
>         xfs_inode_t             *ip,
>         uint                    lock_flags)
> {
>         /*
>          * You can't set both SHARED and EXCL for the same lock,
>          * and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED,
>          * and XFS_ILOCK_EXCL are valid values to set in lock_flags.
>          */
>         ASSERT((lock_flags & (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)) !=
>                (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
>         ASSERT((lock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)) !=
>                (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
>         ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_IUNLOCK_NONOTIFY |
>                         XFS_LOCK_DEP_MASK)) == 0);
>         ASSERT(lock_flags != 0);