Message ID | 49AF0AA0.3080506@redhat.com |
---|---|
State | Rejected, archived |
Headers | show |
On Wed, Mar 04, 2009 at 05:11:28PM -0600, Eric Sandeen wrote: > Eric Sandeen wrote: > > Index: linux-2.6/fs/ext4/ialloc.c > =================================================================== > --- linux-2.6.orig/fs/ext4/ialloc.c > +++ linux-2.6/fs/ext4/ialloc.c > @@ -609,26 +609,33 @@ static int ext4_claim_inode(struct super > struct buffer_head *inode_bitmap_bh, > unsigned long ino, ext4_group_t group, int mode) > { > - int free = 0, retval = 0, count; > + int free = 0, bitset, count; > struct ext4_sb_info *sbi = EXT4_SB(sb); > struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); > > - spin_lock(sb_bgl_lock(sbi, group)); > - if (ext4_set_bit(ino, inode_bitmap_bh->b_data)) { > - /* not a free inode */ > - retval = 1; > - goto err_ret; > + /* if uninit, protect against ext4_read_inode_bitmap initialization */ > + bitset = -1; > + if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) { > + spin_lock(sb_bgl_lock(sbi, group)); > + if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) > + bitset = ext4_set_bit(ino, inode_bitmap_bh->b_data); > + spin_unlock(sb_bgl_lock(sbi, group)); > } That won't work. We need set the bit and clear the INODE_UNINIT flag by holding the spin_lock. In ext4_read_inode_bitmap we check the INODE_UNINIT flag and re-init the inode bitmap. So we may end up re-initing the bitmap if we don't clear the INODE_UNINIT flag holding the spin lock > + if (bitset < 0) /* we didn't set it above, so not uninit */ > + bitset = ext4_set_bit_atomic(sb_bgl_lock(sbi, group), > + ino, inode_bitmap_bh->b_data); > + if (bitset) /* this is not a free inode */ > + return 1; > ino++; > if ((group == 0 && ino < EXT4_FIRST_INO(sb)) || > ino > EXT4_INODES_PER_GROUP(sb)) { > - spin_unlock(sb_bgl_lock(sbi, group)); > ext4_error(sb, __func__, > "reserved inode or inode > inodes count - " > "block_group = %u, inode=%lu", group, > ino + group * EXT4_INODES_PER_GROUP(sb)); > return 1; > } > + spin_lock(sb_bgl_lock(sbi, group)); > /* If we didn't allocate from within the initialized part of the inode > * table then we need to initialize up to this inode. */ > if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) { > @@ -665,9 +672,8 @@ static int ext4_claim_inode(struct super > ext4_used_dirs_set(sb, gdp, count); > } > gdp->bg_checksum = ext4_group_desc_csum(sbi, group, gdp); > -err_ret: > spin_unlock(sb_bgl_lock(sbi, group)); > - return retval; > + return 0; > } > -aneesh -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Aneesh Kumar K.V wrote: > On Wed, Mar 04, 2009 at 05:11:28PM -0600, Eric Sandeen wrote: >> Eric Sandeen wrote: >> >> Index: linux-2.6/fs/ext4/ialloc.c >> =================================================================== >> --- linux-2.6.orig/fs/ext4/ialloc.c >> +++ linux-2.6/fs/ext4/ialloc.c >> @@ -609,26 +609,33 @@ static int ext4_claim_inode(struct super >> struct buffer_head *inode_bitmap_bh, >> unsigned long ino, ext4_group_t group, int mode) >> { >> - int free = 0, retval = 0, count; >> + int free = 0, bitset, count; >> struct ext4_sb_info *sbi = EXT4_SB(sb); >> struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); >> >> - spin_lock(sb_bgl_lock(sbi, group)); >> - if (ext4_set_bit(ino, inode_bitmap_bh->b_data)) { >> - /* not a free inode */ >> - retval = 1; >> - goto err_ret; >> + /* if uninit, protect against ext4_read_inode_bitmap initialization */ >> + bitset = -1; >> + if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) { >> + spin_lock(sb_bgl_lock(sbi, group)); >> + if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) >> + bitset = ext4_set_bit(ino, inode_bitmap_bh->b_data); >> + spin_unlock(sb_bgl_lock(sbi, group)); >> } > > > That won't work. We need set the bit and clear the INODE_UNINIT flag > by holding the spin_lock. In ext4_read_inode_bitmap we check the > INODE_UNINIT flag and re-init the inode bitmap. So we may end up > re-initing the bitmap if we don't clear the INODE_UNINIT flag holding > the spin lock Yeah, I guess maybe that's the same old race we had to start with isn't it. Remind me again why we don't just clear UNINIT when we read in the bitmap in and ... init it? -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6/fs/ext4/ialloc.c =================================================================== --- linux-2.6.orig/fs/ext4/ialloc.c +++ linux-2.6/fs/ext4/ialloc.c @@ -609,26 +609,33 @@ static int ext4_claim_inode(struct super struct buffer_head *inode_bitmap_bh, unsigned long ino, ext4_group_t group, int mode) { - int free = 0, retval = 0, count; + int free = 0, bitset, count; struct ext4_sb_info *sbi = EXT4_SB(sb); struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); - spin_lock(sb_bgl_lock(sbi, group)); - if (ext4_set_bit(ino, inode_bitmap_bh->b_data)) { - /* not a free inode */ - retval = 1; - goto err_ret; + /* if uninit, protect against ext4_read_inode_bitmap initialization */ + bitset = -1; + if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) { + spin_lock(sb_bgl_lock(sbi, group)); + if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) + bitset = ext4_set_bit(ino, inode_bitmap_bh->b_data); + spin_unlock(sb_bgl_lock(sbi, group)); } + if (bitset < 0) /* we didn't set it above, so not uninit */ + bitset = ext4_set_bit_atomic(sb_bgl_lock(sbi, group), + ino, inode_bitmap_bh->b_data); + if (bitset) /* this is not a free inode */ + return 1; ino++; if ((group == 0 && ino < EXT4_FIRST_INO(sb)) || ino > EXT4_INODES_PER_GROUP(sb)) { - spin_unlock(sb_bgl_lock(sbi, group)); ext4_error(sb, __func__, "reserved inode or inode > inodes count - " "block_group = %u, inode=%lu", group, ino + group * EXT4_INODES_PER_GROUP(sb)); return 1; } + spin_lock(sb_bgl_lock(sbi, group)); /* If we didn't allocate from within the initialized part of the inode * table then we need to initialize up to this inode. */ if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) { @@ -665,9 +672,8 @@ static int ext4_claim_inode(struct super ext4_used_dirs_set(sb, gdp, count); } gdp->bg_checksum = ext4_group_desc_csum(sbi, group, gdp); -err_ret: spin_unlock(sb_bgl_lock(sbi, group)); - return retval; + return 0; } /*