From patchwork Sun Mar 28 08:13:51 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: jing zhang X-Patchwork-Id: 48770 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id C6573B7CCD for ; Sun, 28 Mar 2010 19:14:09 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753872Ab0C1IN5 (ORCPT ); Sun, 28 Mar 2010 04:13:57 -0400 Received: from mail-yx0-f191.google.com ([209.85.210.191]:57666 "EHLO mail-yx0-f191.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753789Ab0C1INy (ORCPT ); Sun, 28 Mar 2010 04:13:54 -0400 Received: by yxe29 with SMTP id 29so1497947yxe.4 for ; Sun, 28 Mar 2010 01:13:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:received:message-id:subject:from:to:cc:content-type; bh=M0u8wtW9t4IZkWAnYWGs0FC2S7jLv/oC6AYaDsZzPnA=; b=CJpCG7mm6m8aJMNgsjHCiaQndl0klvUycWqXnJk4toznDFMkXiUHEWmHZfyxh71UW6 tNNOMs1esPkXuvlh0tY2RgLEsTTt1mp9npfNW48XKaKU8LxUy4aYbyiLlRPuMM3kpN4M 5D1TTN2P3VpJn/hzIUMeA+/w+pgNTipPpZRMo= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; b=u6iLcgeSE/lWXY/mYsEICyH2QST0OmuBffUK6RodWKI+nuL88GzzAes2pxtTnHIvmi CihdbY511gkyG+6tsa6Wt9QN59lFCYx5QfYjm+/aPZPueLWnDKnuK57uOJoUji36lXbn o+ph8OgxNswT9fj0nvs6uEAhcXNZH1e+14mRA= MIME-Version: 1.0 Received: by 10.100.253.6 with HTTP; Sun, 28 Mar 2010 01:13:51 -0700 (PDT) In-Reply-To: <87sk7nv4sp.fsf@linux.vnet.ibm.com> References: <20100322012758.GE11560@thunk.org> <87sk7nv4sp.fsf@linux.vnet.ibm.com> Date: Sun, 28 Mar 2010 16:13:51 +0800 Received: by 10.101.136.13 with SMTP id o13mr2999872ann.235.1269764031956; Sun, 28 Mar 2010 01:13:51 -0700 (PDT) Message-ID: Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init() From: jing zhang To: "Aneesh Kumar K. V" Cc: tytso@mit.edu, linux-ext4 , Andreas Dilger , Dave Kleikamp Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org 2010/3/26, Aneesh Kumar K. V : > On Tue, 23 Mar 2010 20:47:39 +0800, jing zhang wrote: >> 2010/3/22, tytso@mit.edu : >> > On Sun, Mar 21, 2010 at 10:01:06PM +0800, jing zhang wrote: >> >> From: Jing Zhang >> >> >> >> Date: Sun Mar 21 21:59:35 2010 >> >> >> >> Cc: Theodore Ts'o >> >> Cc: Andreas Dilger >> >> Cc: Dave Kleikamp >> >> Signed-off-by: Jing Zhang >> >> >> >> --- >> >> >> >> --- linux-2.6.32/fs/ext4/mballoc.c 2009-12-03 11:51:22.000000000 +0800 >> >> +++ ext4_mm_leak/mballoc3.c 2010-03-21 21:37:18.000000000 +0800 >> >> @@ -2360,6 +2360,24 @@ err_freesgi: >> >> return -ENOMEM; >> >> } >> >> >> >> +static void ext4_mb_destroy_backend(struct super_block *sb) >> >> +{ >> >> + ext4_group_t ngroups = ext4_get_groups_count(sb); >> >> + ext4_group_t i; >> >> + struct ext4_sb_info *sbi = EXT4_SB(sb); >> >> + int j; >> >> + int num_meta_group_infos = (ngroups + EXT4_DESC_PER_BLOCK(sb) -1) >> >> + >> EXT4_DESC_PER_BLOCK_BITS(sb); >> >> + for (i = 0; i < ngroups; i++) >> >> + kfree(ext4_get_group_info(sb, i)); >> >> + >> >> + for (j = 0; j < num_meta_group_infos; j++) >> >> + kfree(sbi->s_group_info[j]); >> >> + >> >> + kfree(sbi->s_group_info); >> >> + iput(sbi->s_buddy_cache); >> >> +} >> >> + >> > >> > It would be better if this could be done by making ext4_mb_release() >> > more flexible, and then calling ext4_mb_release() if there is an error >> > setting up the data structures in ext4_mb_init(). >> > >> > - Ted >> > >> >> Yeah, Ted, going through ext4_mb_release() is clearer. >> >> --- >> >> diff --git a/linux-2.6.32/fs/ext4/mballoc.c b/ext4_mm_leak/mballoc3.c >> index bba1282..99ca2de 100644 >> --- a/linux-2.6.32/fs/ext4/mballoc.c >> +++ b/ext4_mm_leak/mballoc3.c >> @@ -2417,8 +2417,7 @@ int ext4_mb_init(struct super_block *sb, int >> needs_recovery) >> >> sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group); >> if (sbi->s_locality_groups == NULL) { >> - kfree(sbi->s_mb_offsets); >> - kfree(sbi->s_mb_maxs); >> + ext4_mb_release(sb); > > We may want to make sure that we can safely call ext4_mb_release that > early. what i would suggest is to move s_locality_group allocation > before ext4_mb_init. that makes error handling easy > cool idea, Aneesh, and I got it. --- > > >> return -ENOMEM; >> } >> for_each_possible_cpu(i) { >> @@ -2511,7 +2510,8 @@ int ext4_mb_release(struct super_block *sb) >> atomic_read(&sbi->s_mb_discarded)); >> } >> >> - free_percpu(sbi->s_locality_groups); >> + if (sbi->s_locality_groups) >> + free_percpu(sbi->s_locality_groups); >> if (sbi->s_proc) >> remove_proc_entry("mb_groups", sbi->s_proc); >> -- >> 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 > -- 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 --- old-linux-2.6.32/fs/ext4/mballoc.c 2009-12-03 11:51:22.000000000 +0800 +++ ext4_mm_leak/mballoc3-2.c 2010-03-28 16:13:20.000000000 +0800 @@ -2397,11 +2397,27 @@ int ext4_mb_init(struct super_block *sb, i++; } while (i <= sb->s_blocksize_bits + 1); + sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group); + if (sbi->s_locality_groups == NULL) { + kfree(sbi->s_mb_offsets); + kfree(sbi->s_mb_maxs); + return -ENOMEM; + } + for_each_possible_cpu(i) { + struct ext4_locality_group *lg; + lg = per_cpu_ptr(sbi->s_locality_groups, i); + mutex_init(&lg->lg_mutex); + for (j = 0; j < PREALLOC_TB_SIZE; j++) + INIT_LIST_HEAD(&lg->lg_prealloc_list[j]); + spin_lock_init(&lg->lg_prealloc_lock); + } + /* init file for buddy data */ ret = ext4_mb_init_backend(sb); if (ret != 0) { kfree(sbi->s_mb_offsets); kfree(sbi->s_mb_maxs); + free_percpu(sbi->s_locality_groups); return ret; } @@ -2415,20 +2431,6 @@ int ext4_mb_init(struct super_block *sb, sbi->s_mb_order2_reqs = MB_DEFAULT_ORDER2_REQS; sbi->s_mb_group_prealloc = MB_DEFAULT_GROUP_PREALLOC; - sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group); - if (sbi->s_locality_groups == NULL) { - kfree(sbi->s_mb_offsets); - kfree(sbi->s_mb_maxs); - return -ENOMEM; - } - for_each_possible_cpu(i) { - struct ext4_locality_group *lg; - lg = per_cpu_ptr(sbi->s_locality_groups, i); - mutex_init(&lg->lg_mutex); - for (j = 0; j < PREALLOC_TB_SIZE; j++) - INIT_LIST_HEAD(&lg->lg_prealloc_list[j]); - spin_lock_init(&lg->lg_prealloc_lock); - } if (sbi->s_proc) proc_create_data("mb_groups", S_IRUGO, sbi->s_proc,