On Mon, 2007-07-16 at 18:06 -0600, Andreas Dilger wrote: > On Jul 16, 2007 16:52 -0700, Mingming Cao wrote: > > I am not sure why we need GFP_KERNEL flag here. I think we should use > > GFP_NOFS instead. The following patch use the GFP_NOFS flag, as well as > > fixing memory leak issue introduced by the ext4 expand inode extra isize > > patch. > > > > Fixing memory allocation issue with expand inode extra isize patch. > > > > - use GFP_NOFS instead of GFP_KERNEL flag for memory allocation > > - use kzalloc instead of kmalloc > > This doesn't need kzalloc() for buffer and b_entry_name, since they are > immediately overwritten by memcpy(). > ok. > > - fix memory leak in the success case, at the end of while loop. > > goto cleanup; > > @@ -1302,7 +1302,15 @@ retry: > > error = ext4_xattr_block_set(handle, inode, &i, bs); > > if (error) > > goto cleanup; > > + kfree(b_entry_name); > > + kfree(buffer); > > + brelse(is->iloc.bh); > > + kfree(is); > > + kfree(bs); > > + brelse(bh); > > } > > + up_write(&EXT4_I(inode)->xattr_sem); > > + return 0; > > > > cleanup: > > kfree(b_entry_name); > > I don't think you should have brelse(bh) inside the loop, since it is > allocated before the loop starts. > Ahh right. thanks. Updated fix. Fixing memory allocation issue with expand inode extra isize patch. - use GFP_NOFS instead of GFP_KERNEL to prevent fs reenter - fix memory leak in the success case, at the end of while loop. Signed-off-by: Mingming Cao <cmm@xxxxxxxxxx> --- fs/ext4/xattr.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) Index: linux-2.6.22/fs/ext4/xattr.c =================================================================== --- linux-2.6.22.orig/fs/ext4/xattr.c 2007-07-16 17:21:14.000000000 -0700 +++ linux-2.6.22/fs/ext4/xattr.c 2007-07-16 17:22:25.000000000 -0700 @@ -1204,8 +1204,8 @@ retry: unsigned int shift_bytes; /* No. of bytes to shift EAs by? */ unsigned int min_total_size = ~0U; - is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_KERNEL); - bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_KERNEL); + is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS); + bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_NOFS); if (!is || !bs) { error = -ENOMEM; goto cleanup; @@ -1251,8 +1251,8 @@ retry: size = le32_to_cpu(entry->e_value_size); entry_size = EXT4_XATTR_LEN(entry->e_name_len); i.name_index = entry->e_name_index, - buffer = kmalloc(EXT4_XATTR_SIZE(size), GFP_KERNEL); - b_entry_name = kmalloc(entry->e_name_len + 1, GFP_KERNEL); + buffer = kmalloc(EXT4_XATTR_SIZE(size), GFP_NOFS); + b_entry_name = kmalloc(entry->e_name_len + 1, GFP_NOFS); if (!buffer || !b_entry_name) { error = -ENOMEM; goto cleanup; @@ -1302,7 +1302,15 @@ retry: error = ext4_xattr_block_set(handle, inode, &i, bs); if (error) goto cleanup; + kfree(b_entry_name); + kfree(buffer); + brelse(is->iloc.bh); + kfree(is); + kfree(bs); } + brelse(bh); + up_write(&EXT4_I(inode)->xattr_sem); + return 0; cleanup: kfree(b_entry_name); - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html