* [PATCH] Unlock inode before calling xfs_idestroy()
@ 2008-09-19 3:15 Lachlan McIlroy
2008-09-19 4:12 ` Timothy Shimmin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Lachlan McIlroy @ 2008-09-19 3:15 UTC (permalink / raw)
To: xfs-oss, xfs-dev
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;
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Unlock inode before calling xfs_idestroy()
2008-09-19 3:15 [PATCH] Unlock inode before calling xfs_idestroy() Lachlan McIlroy
@ 2008-09-19 4:12 ` Timothy Shimmin
2008-09-22 3:52 ` Lachlan McIlroy
2008-09-19 7:26 ` Christoph Hellwig
2008-09-20 7:01 ` Dave Chinner
2 siblings, 1 reply; 7+ messages in thread
From: Timothy Shimmin @ 2008-09-19 4:12 UTC (permalink / raw)
To: lachlan; +Cc: xfs-oss, xfs-dev
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().
--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);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Unlock inode before calling xfs_idestroy()
2008-09-19 4:12 ` Timothy Shimmin
@ 2008-09-22 3:52 ` Lachlan McIlroy
0 siblings, 0 replies; 7+ messages in thread
From: Lachlan McIlroy @ 2008-09-22 3:52 UTC (permalink / raw)
To: Timothy Shimmin; +Cc: xfs-oss, xfs-dev
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);
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Unlock inode before calling xfs_idestroy()
2008-09-19 3:15 [PATCH] Unlock inode before calling xfs_idestroy() Lachlan McIlroy
2008-09-19 4:12 ` Timothy Shimmin
@ 2008-09-19 7:26 ` Christoph Hellwig
2008-09-22 3:57 ` Lachlan McIlroy
2008-09-20 7:01 ` Dave Chinner
2 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2008-09-19 7:26 UTC (permalink / raw)
To: Lachlan McIlroy; +Cc: xfs-oss, xfs-dev
On Fri, Sep 19, 2008 at 01:15:08PM +1000, 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;
Just move the xfs_ilock call after these two statements, there is no
need to have it locked before inserting it into the radix tree.
> @@ -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;
But here we still need the fix. But as Tim mention we need to check
for lock_flags != 0 first. Long-term it might make sense to just make
xfs_iunlock a no-op if lock_flags == 0, but let's do that separately.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Unlock inode before calling xfs_idestroy()
2008-09-19 7:26 ` Christoph Hellwig
@ 2008-09-22 3:57 ` Lachlan McIlroy
0 siblings, 0 replies; 7+ messages in thread
From: Lachlan McIlroy @ 2008-09-22 3:57 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs-oss, xfs-dev
Christoph Hellwig wrote:
> On Fri, Sep 19, 2008 at 01:15:08PM +1000, 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;
>
> Just move the xfs_ilock call after these two statements, there is no
> need to have it locked before inserting it into the radix tree.
Okay, thanks. I thought we may need it locked while doing the mode
check so I left it as is.
>
>> @@ -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;
>
> But here we still need the fix. But as Tim mention we need to check
> for lock_flags != 0 first. Long-term it might make sense to just make
> xfs_iunlock a no-op if lock_flags == 0, but let's do that separately.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Unlock inode before calling xfs_idestroy()
2008-09-19 3:15 [PATCH] Unlock inode before calling xfs_idestroy() Lachlan McIlroy
2008-09-19 4:12 ` Timothy Shimmin
2008-09-19 7:26 ` Christoph Hellwig
@ 2008-09-20 7:01 ` Dave Chinner
2008-09-22 3:59 ` Lachlan McIlroy
2 siblings, 1 reply; 7+ messages in thread
From: Dave Chinner @ 2008-09-20 7:01 UTC (permalink / raw)
To: Lachlan McIlroy; +Cc: xfs-oss, xfs-dev
On Fri, Sep 19, 2008 at 01:15:08PM +1000, Lachlan McIlroy wrote:
> Lock debugging reported the ilock was being destroyed
> without being unlocked.
Is this going to go in before or after my changes to refactor
this function? If it is before< can you please push it out to
the master git tree ASAP (and all the other outstanding changes)
so I can rediff all the outstanding patches I have before I
send them again?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Unlock inode before calling xfs_idestroy()
2008-09-20 7:01 ` Dave Chinner
@ 2008-09-22 3:59 ` Lachlan McIlroy
0 siblings, 0 replies; 7+ messages in thread
From: Lachlan McIlroy @ 2008-09-22 3:59 UTC (permalink / raw)
To: Lachlan McIlroy, xfs-oss, xfs-dev
Dave Chinner wrote:
> On Fri, Sep 19, 2008 at 01:15:08PM +1000, Lachlan McIlroy wrote:
>> Lock debugging reported the ilock was being destroyed
>> without being unlocked.
>
> Is this going to go in before or after my changes to refactor
> this function? If it is before< can you please push it out to
> the master git tree ASAP (and all the other outstanding changes)
> so I can rediff all the outstanding patches I have before I
> send them again?
Yeah sure. I need to get Christoph's btree patch set in real soon
so I'll do that then update oss/master.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-09-22 3:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-09-19 3:15 [PATCH] Unlock inode before calling xfs_idestroy() Lachlan McIlroy
2008-09-19 4:12 ` Timothy Shimmin
2008-09-22 3:52 ` Lachlan McIlroy
2008-09-19 7:26 ` Christoph Hellwig
2008-09-22 3:57 ` Lachlan McIlroy
2008-09-20 7:01 ` Dave Chinner
2008-09-22 3:59 ` Lachlan McIlroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox