Skip to contentSkip to conversation

reiserfs: fix deadlocks with quotas

PATCH In review Jeff Mahoney 2 participants 2 messages Aug 3, 2012 – Aug 14, 2012 VFS

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 reiserfs: fix deadlocks with quotas

The BKL push-down for reiserfs made lock recursion a special case that needs to be handled explicitly. One of the cases that was unhandled is dropping the quota during inode eviction. Both reiserfs_evict_inode and reiserfs_write_dquot take the write lock, but when the journal lock is taken it only drops one the references. The locking rules are that the journal lock be acquired before the write lock so leaving the reference open leads to a ABBA deadlock.

This patch pushes the unlock up before clear_inode and avoids the recursive locking.

Another ABBA situation can occur when the write lock is dropped while reading the bitmap buffer while in the quota code. When the lock is reacquired, it will deadlock against dquot->dq_lock and dqopt->dqio_mutex in the dquot_acquire path. It's safe to retain the lock across the read and should be cached under write load.

---
 fs/reiserfs/bitmap.c |    2 --
 fs/reiserfs/inode.c  |    2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)
fs/reiserfs/bitmap.c +0 -2
@@ -1334,9 +1334,7 @@ struct buffer_head *reiserfs_read_bitmap
 	else if (bitmap == 0)
 		block = (REISERFS_DISK_OFFSET_IN_BYTES >> sb->s_blocksize_bits) + 1;
 
-	reiserfs_write_unlock(sb);
 	bh = sb_bread(sb, block);
-	reiserfs_write_lock(sb);
 	if (bh == NULL)
 		reiserfs_warning(sb, "sh-2029: %s: bitmap block (#%u) "
 		                 "reading failed", __func__, block);
fs/reiserfs/inode.c +1 -1
@@ -76,10 +76,10 @@ void reiserfs_evict_inode(struct inode *
 		;
 	}
       out:
+	reiserfs_write_unlock_once(inode->i_sb, depth);
 	clear_inode(inode);	/* note this must go after the journal_end to prevent deadlock */
 	dquot_drop(inode);
 	inode->i_blocks = 0;
-	reiserfs_write_unlock_once(inode->i_sb, depth);
 	return;
 
 no_delete:
Signature · 2 lines
Jeff Mahoney
SUSE Labs
reviewer
#1 lore raw
Quoted 11 lines · Jeff Mahoney

The BKL push-down for reiserfs made lock recursion a special case that needs to be handled explicitly. One of the cases that was unhandled is dropping the quota during inode eviction. Both reiserfs_evict_inode and reiserfs_write_dquot take the write lock, but when the journal lock is taken it only drops one the references. The locking rules are that the journal lock be acquired before the write lock so leaving the reference open leads to a ABBA deadlock.

This patch pushes the unlock up before clear_inode and avoids the recursive locking.

Quoted 7 lines

Another ABBA situation can occur when the write lock is dropped while reading the bitmap buffer while in the quota code. When the lock is reacquired, it will deadlock against dquot->dq_lock and dqopt->dqio_mutex in the dquot_acquire path. It's safe to retain the lock across the read and should be cached under write load.

Yeah, the patch looks good. Since we don't have reiserfs maintainer, I
guess I'll push this through my tree.

Honza

Quoted 35 lines
---
 fs/reiserfs/bitmap.c |    2 --
 fs/reiserfs/inode.c  |    2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)
fs/reiserfs/bitmap.c +0 -2
@@ -1334,9 +1334,7 @@ struct buffer_head *reiserfs_read_bitmap
 	else if (bitmap == 0)
 		block = (REISERFS_DISK_OFFSET_IN_BYTES >> sb->s_blocksize_bits) + 1;
 
-	reiserfs_write_unlock(sb);
 	bh = sb_bread(sb, block);
-	reiserfs_write_lock(sb);
 	if (bh == NULL)
 		reiserfs_warning(sb, "sh-2029: %s: bitmap block (#%u) "
 		                 "reading failed", __func__, block);
fs/reiserfs/inode.c +1 -1
@@ -76,10 +76,10 @@ void reiserfs_evict_inode(struct inode *
 		;
 	}
       out:
+	reiserfs_write_unlock_once(inode->i_sb, depth);
 	clear_inode(inode);	/* note this must go after the journal_end to prevent deadlock */
 	dquot_drop(inode);
 	inode->i_blocks = 0;
-	reiserfs_write_unlock_once(inode->i_sb, depth);
 	return;
 
 no_delete:
Signature · 3 lines
-- 
Jeff Mahoney
SUSE Labs
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR