Messages in this thread Patch in this message | | | Date | Fri, 02 Oct 1998 22:19:43 -0700 | From | Bill Hawes <> | Subject | Patch for 2.1.123 ext2 (dquota race problem) |
| |
I've attached a patch to correct an ext2 race problem with disk quota for a deleted inode, following a report sent by David Miller.
Dropping disk quota for a deleted inode requires writing to the quota file, and this may lead to attempting to lock the super block for one of the ext2 block allocations. But since ext2_free_inode already holds the superblock lock, a deadlock can result.
The attached patch avoids the problem by reordering operations to drop the disk quota before locking the superblock. Since there are no other references to the inode at this time, the reordering shouldn't cause any problems. (The subsequent DQUOT_DROP() in clear_inode will be a nop.)
I've tested the changes with intensive cp and rm commands to a directory with disk quotas, and all seems to be working OK. But I'm not sure this setup would always triggger the race, and so would appreciate further testing by anyone who has seen processes stuck in wait_on_super when using disk quotas.
Regards, Bill
--- linux-2.1.123/fs/ext2/ialloc.c.old Wed Aug 26 13:20:32 1998 +++ linux-2.1.123/fs/ext2/ialloc.c Fri Oct 2 20:09:59 1998 @@ -179,9 +179,9 @@ */ void ext2_free_inode (struct inode * inode) { + struct super_block * sb = inode->i_sb; int is_directory; unsigned long ino; - struct super_block * sb; struct buffer_head * bh; struct buffer_head * bh2; unsigned long block_group; @@ -190,8 +190,6 @@ struct ext2_group_desc * gdp; struct ext2_super_block * es; - if (!inode) - return; if (!inode->i_dev) { printk ("ext2_free_inode: inode has no device\n"); return; @@ -205,7 +203,7 @@ inode->i_nlink); return; } - if (!inode->i_sb) { + if (!sb) { printk("ext2_free_inode: inode on nonexistent device\n"); return; } @@ -213,15 +211,21 @@ ino = inode->i_ino; ext2_debug ("freeing inode %lu\n", ino); - sb = inode->i_sb; + /* + * Note: we must free any quota before locking the superblock, + * as writing the quota to disk may need the lock as well. + */ + DQUOT_FREE_INODE(sb, inode); + DQUOT_DROP(inode); + lock_super (sb); - if (ino < EXT2_FIRST_INO(sb) || - ino > le32_to_cpu(sb->u.ext2_sb.s_es->s_inodes_count)) { + es = sb->u.ext2_sb.s_es; + if (ino < EXT2_FIRST_INO(sb) || + ino > le32_to_cpu(es->s_inodes_count)) { ext2_error (sb, "free_inode", "reserved inode or nonexistent inode"); goto error_return; } - es = sb->u.ext2_sb.s_es; block_group = (ino - 1) / EXT2_INODES_PER_GROUP(sb); bit = (ino - 1) % EXT2_INODES_PER_GROUP(sb); bitmap_nr = load_inode_bitmap (sb, block_group); @@ -233,7 +237,6 @@ is_directory = S_ISDIR(inode->i_mode); /* Do this BEFORE marking the inode not in use */ - DQUOT_FREE_INODE(sb, inode); clear_inode (inode); /* Ok, now we can actually update the inode bitmaps.. */ | |