[PATCH] udf deadlock fix

Yes, me too.  generic_shutdown_super() takes lock_super().  And udf uses
lock_super for protecting its block allocation data strutures.  Trivial
deadlock on unmount.

Below is a fix to switch udf to it's own private locking.  It's safe
because it doesn't intefere with VFS lock_super usage anywhere.

udf_free_inode has some more updates than simply switching the used
lock:

 - clear_inode() call moved outside locked section to avoid another
   deadlock
 - unused variable ino killed
 - is_directory moved into the conditional it's actually used in

Signed-off-by: Christoph Hellwig <hch@lst.de>
(note that I see memory corruption in UDF_I_DATA(inode), but I've
reproduced that with a kernel without all recent udf changes.  I'll
debug that one further)
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index cddb733d20fb41cb9bc5cec2e9ccd72be2b4d55a..b9ded26b10a90b49972e3654d15ad198054c5faf 100644 (file)
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -148,6 +148,7 @@ static void udf_bitmap_free_blocks(struct super_block * sb,
        struct udf_bitmap *bitmap,
        kernel_lb_addr bloc, uint32_t offset, uint32_t count)
 {
+       struct udf_sb_info *sbi = UDF_SB(sb);
        struct buffer_head * bh = NULL;
        unsigned long block;
        unsigned long block_group;
@@ -156,7 +157,7 @@ static void udf_bitmap_free_blocks(struct super_block * sb,
        int bitmap_nr;
        unsigned long overflow;
 
-       lock_super(sb);
+       down(&sbi->s_alloc_sem);
        if (bloc.logicalBlockNum < 0 ||
                (bloc.logicalBlockNum + count) > UDF_SB_PARTLEN(sb, bloc.partitionReferenceNum))
        {
@@ -215,7 +216,7 @@ error_return:
        sb->s_dirt = 1;
        if (UDF_SB_LVIDBH(sb))
                mark_buffer_dirty(UDF_SB_LVIDBH(sb));
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
        return;
 }
 
@@ -224,13 +225,13 @@ static int udf_bitmap_prealloc_blocks(struct super_block * sb,
        struct udf_bitmap *bitmap, uint16_t partition, uint32_t first_block,
        uint32_t block_count)
 {
+       struct udf_sb_info *sbi = UDF_SB(sb);
        int alloc_count = 0;
        int bit, block, block_group, group_start;
        int nr_groups, bitmap_nr;
        struct buffer_head *bh;
 
-       lock_super(sb);
-
+       down(&sbi->s_alloc_sem);
        if (first_block < 0 || first_block >= UDF_SB_PARTLEN(sb, partition))
                goto out;
 
@@ -279,7 +280,7 @@ out:
                mark_buffer_dirty(UDF_SB_LVIDBH(sb));
        }
        sb->s_dirt = 1;
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
        return alloc_count;
 }
 
@@ -287,6 +288,7 @@ static int udf_bitmap_new_block(struct super_block * sb,
        struct inode * inode,
        struct udf_bitmap *bitmap, uint16_t partition, uint32_t goal, int *err)
 {
+       struct udf_sb_info *sbi = UDF_SB(sb);
        int newbit, bit=0, block, block_group, group_start;
        int end_goal, nr_groups, bitmap_nr, i;
        struct buffer_head *bh = NULL;
@@ -294,7 +296,7 @@ static int udf_bitmap_new_block(struct super_block * sb,
        int newblock = 0;
 
        *err = -ENOSPC;
-       lock_super(sb);
+       down(&sbi->s_alloc_sem);
 
 repeat:
        if (goal < 0 || goal >= UDF_SB_PARTLEN(sb, partition))
@@ -367,7 +369,7 @@ repeat:
        }
        if (i >= (nr_groups*2))
        {
-               unlock_super(sb);
+               up(&sbi->s_alloc_sem);
                return newblock;
        }
        if (bit < sb->s_blocksize << 3)
@@ -376,7 +378,7 @@ repeat:
                bit = udf_find_next_one_bit(bh->b_data, sb->s_blocksize << 3, group_start << 3);
        if (bit >= sb->s_blocksize << 3)
        {
-               unlock_super(sb);
+               up(&sbi->s_alloc_sem);
                return 0;
        }
 
@@ -390,7 +392,7 @@ got_block:
         */
        if (inode && DQUOT_ALLOC_BLOCK(inode, 1))
        {
-               unlock_super(sb);
+               up(&sbi->s_alloc_sem);
                *err = -EDQUOT;
                return 0;
        }
@@ -413,13 +415,13 @@ got_block:
                mark_buffer_dirty(UDF_SB_LVIDBH(sb));
        }
        sb->s_dirt = 1;
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
        *err = 0;
        return newblock;
 
 error_return:
        *err = -EIO;
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
        return 0;
 }
 
@@ -428,6 +430,7 @@ static void udf_table_free_blocks(struct super_block * sb,
        struct inode * table,
        kernel_lb_addr bloc, uint32_t offset, uint32_t count)
 {
+       struct udf_sb_info *sbi = UDF_SB(sb);
        uint32_t start, end;
        uint32_t nextoffset, oextoffset, elen;
        kernel_lb_addr nbloc, obloc, eloc;
@@ -435,7 +438,7 @@ static void udf_table_free_blocks(struct super_block * sb,
        int8_t etype;
        int i;
 
-       lock_super(sb);
+       down(&sbi->s_alloc_sem);
        if (bloc.logicalBlockNum < 0 ||
                (bloc.logicalBlockNum + count) > UDF_SB_PARTLEN(sb, bloc.partitionReferenceNum))
        {
@@ -669,7 +672,7 @@ static void udf_table_free_blocks(struct super_block * sb,
 
 error_return:
        sb->s_dirt = 1;
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
        return;
 }
 
@@ -678,6 +681,7 @@ static int udf_table_prealloc_blocks(struct super_block * sb,
        struct inode *table, uint16_t partition, uint32_t first_block,
        uint32_t block_count)
 {
+       struct udf_sb_info *sbi = UDF_SB(sb);
        int alloc_count = 0;
        uint32_t extoffset, elen, adsize;
        kernel_lb_addr bloc, eloc;
@@ -694,8 +698,7 @@ static int udf_table_prealloc_blocks(struct super_block * sb,
        else
                return 0;
 
-       lock_super(sb);
-
+       down(&sbi->s_alloc_sem);
        extoffset = sizeof(struct unallocSpaceEntry);
        bloc = UDF_I_LOCATION(table);
 
@@ -739,7 +742,7 @@ static int udf_table_prealloc_blocks(struct super_block * sb,
                mark_buffer_dirty(UDF_SB_LVIDBH(sb));
                sb->s_dirt = 1;
        }
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
        return alloc_count;
 }
 
@@ -747,6 +750,7 @@ static int udf_table_new_block(struct super_block * sb,
        struct inode * inode,
        struct inode *table, uint16_t partition, uint32_t goal, int *err)
 {
+       struct udf_sb_info *sbi = UDF_SB(sb);
        uint32_t spread = 0xFFFFFFFF, nspread = 0xFFFFFFFF;
        uint32_t newblock = 0, adsize;
        uint32_t extoffset, goal_extoffset, elen, goal_elen = 0;
@@ -763,8 +767,7 @@ static int udf_table_new_block(struct super_block * sb,
        else
                return newblock;
 
-       lock_super(sb);
-
+       down(&sbi->s_alloc_sem);
        if (goal < 0 || goal >= UDF_SB_PARTLEN(sb, partition))
                goal = 0;
 
@@ -814,7 +817,7 @@ static int udf_table_new_block(struct super_block * sb,
        if (spread == 0xFFFFFFFF)
        {
                udf_release_data(goal_bh);
-               unlock_super(sb);
+               up(&sbi->s_alloc_sem);
                return 0;
        }
 
@@ -830,7 +833,7 @@ static int udf_table_new_block(struct super_block * sb,
        if (inode && DQUOT_ALLOC_BLOCK(inode, 1))
        {
                udf_release_data(goal_bh);
-               unlock_super(sb);
+               up(&sbi->s_alloc_sem);
                *err = -EDQUOT;
                return 0;
        }
@@ -849,7 +852,7 @@ static int udf_table_new_block(struct super_block * sb,
        }
 
        sb->s_dirt = 1;
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
        *err = 0;
        return newblock;
 }

diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c
index 1604da9dce955872049ce08909ad63d09f590ccf..a7e5d40f1ebc002a354abb68c6a0a89e95594656 100644 (file)
--- a/fs/udf/ialloc.c
+++ b/fs/udf/ialloc.c
@@ -35,11 +35,8 @@
 
 void udf_free_inode(struct inode * inode)
 {
-       struct super_block * sb = inode->i_sb;
-       int is_directory;
-       unsigned long ino;
-
-       ino = inode->i_ino;
+       struct super_block *sb = inode->i_sb;
+       struct udf_sb_info *sbi = UDF_SB(sb);
 
        /*
         * Note: we must free any quota before locking the superblock,
@@ -48,36 +45,32 @@ void udf_free_inode(struct inode * inode)
        DQUOT_FREE_INODE(inode);
        DQUOT_DROP(inode);
 
-       lock_super(sb);
-
-       is_directory = S_ISDIR(inode->i_mode);
-
        clear_inode(inode);
 
-       if (UDF_SB_LVIDBH(sb))
-       {
-               if (is_directory)
+       down(&sbi->s_alloc_sem);
+       if (sbi->s_lvidbh) {
+               if (S_ISDIR(inode->i_mode))
                        UDF_SB_LVIDIU(sb)->numDirs =
                                cpu_to_le32(le32_to_cpu(UDF_SB_LVIDIU(sb)->numDirs) - 1);
                else
                        UDF_SB_LVIDIU(sb)->numFiles =
                                cpu_to_le32(le32_to_cpu(UDF_SB_LVIDIU(sb)->numFiles) - 1);
                
-               mark_buffer_dirty(UDF_SB_LVIDBH(sb));
+               mark_buffer_dirty(sbi->s_lvidbh);
        }
-       unlock_super(sb);
+       up(&sbi->s_alloc_sem);
 
        udf_free_blocks(sb, NULL, UDF_I_LOCATION(inode), 0, 1);
 }
 
 struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 {
-       struct super_block *sb;
+       struct super_block *sb = dir->i_sb;
+       struct udf_sb_info *sbi = UDF_SB(sb);
        struct inode * inode;
        int block;
        uint32_t start = UDF_I_LOCATION(dir).logicalBlockNum;
 
-       sb = dir->i_sb;
        inode = new_inode(sb);
 
        if (!inode)
@@ -94,8 +87,8 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
                iput(inode);
                return NULL;
        }
-       lock_super(sb);
 
+       down(&sbi->s_alloc_sem);
        UDF_I_UNIQUE(inode) = 0;
        UDF_I_LENEXTENTS(inode) = 0;
        UDF_I_NEXT_ALLOC_BLOCK(inode) = 0;
@@ -160,8 +153,8 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
                UDF_I_CRTIME(inode) = current_fs_time(inode->i_sb);
        insert_inode_hash(inode);
        mark_inode_dirty(inode);
+       up(&sbi->s_alloc_sem);
 
-       unlock_super(sb);
        if (DQUOT_ALLOC_INODE(inode))
        {
                DQUOT_DROP(inode);

diff --git a/fs/udf/super.c b/fs/udf/super.c
index e2fdb47ce82e1ee495cfbfded0c134933a68e373..15bd4f24c5b762907d643c231a53e8016d23160b 100644 (file)
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1504,6 +1504,8 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
        sb->s_fs_info = sbi;
        memset(UDF_SB(sb), 0x00, sizeof(struct udf_sb_info));
 
+       init_MUTEX(&sbi->s_alloc_sem);
+
        if (!udf_parse_options((char *)options, &uopt))
                goto error_out;
 

diff --git a/include/linux/udf_fs_sb.h b/include/linux/udf_fs_sb.h
index b73cebba5551c1527f27f44db925937b38366344..1966a6dbb4b6f70301470e381d845d7efe4b824e 100644 (file)
--- a/include/linux/udf_fs_sb.h
+++ b/include/linux/udf_fs_sb.h
@@ -18,6 +18,8 @@
 #ifndef _UDF_FS_SB_H
 #define _UDF_FS_SB_H 1
 
+#include <asm/semaphore.h>
+
 #pragma pack(1)
 
 #define UDF_MAX_BLOCK_LOADED   8
@@ -113,6 +115,8 @@ struct udf_sb_info
 
        /* VAT inode */
        struct inode            *s_vat;
+
+       struct semaphore        s_alloc_sem;
 };
 
 #endif /* _UDF_FS_SB_H */