From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761482AbcINIZt (ORCPT ); Wed, 14 Sep 2016 04:25:49 -0400 Received: from smtp2.provo.novell.com ([137.65.250.81]:37742 "EHLO smtp2.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760265AbcINIZo (ORCPT ); Wed, 14 Sep 2016 04:25:44 -0400 Subject: Re: Question about commit f9a67b1182e5 ("md/bitmap: clear bitmap if bitmap_create failed"). To: Shaohua Li , Christophe JAILLET References: <752ab1d9-412a-149b-a241-e604040ebaff@wanadoo.fr> <20160913172433.GB24264@kernel.org> Cc: linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org From: Guoqing Jiang Message-ID: <57D9097C.5050202@suse.com> Date: Wed, 14 Sep 2016 04:25:32 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20160913172433.GB24264@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/13/2016 01:24 PM, Shaohua Li wrote: > On Mon, Sep 12, 2016 at 09:09:48PM +0200, Christophe JAILLET wrote: >> Hi, >> >> I'm puzzled by commit f9a67b1182e5 ("md/bitmap: clear bitmap if >> bitmap_create failed"). > Hi Christophe, > Thank you very much to help check this! > >> Part of the commit is: >> >> @@ -1865,8 +1866,10 @@ int bitmap_copy_from_slot(struct mddev *mddev, int >> slot, >> struct bitmap_counts *counts; >> struct bitmap *bitmap = bitmap_create(mddev, slot); >> >> - if (IS_ERR(bitmap)) >> + if (IS_ERR(bitmap)) { >> + bitmap_free(bitmap); >> return PTR_ERR(bitmap); >> + } >> >> but if 'bitmap' is an error, I think that bad things will happen in >> 'bitmap_free()' when, at the beginning of the function, we will execute: >> >> if (bitmap->sysfs_can_clear) <----------------- >> sysfs_put(bitmap->sysfs_can_clear); I guess it is safe, since below part is at the beginning of bitmap_free. if (!bitmap) /* there was no bitmap */ return; > Add Guoqing. > > Yeah, you are right, this bitmap_free isn't required. This must be something > slip in in the v2 patch. I'll delete that line. > >> However, the commit log message is really explicit and adding this call to >> 'bitmap_free' has really been done one purpose. ("If bitmap_create returns >> an error, we need to call either bitmap_destroy or bitmap_free to do clean >> up, ...") > this log is a little confusing, I thought it really means the bitmap_free called > in bitmap_create. The V1 patch calls bitmap_destroy in bitmap_create. I double checked v1 patch, it called bitmap_destroy once bitmap_create returned error inside bitmap_copy_from_slot, also bitmap_destroy is also not called in location_store once failed to create bitmap. But since bitmap_free within bitmap_create is used to handle related failure, seems we don't need the patch, and maybe we also don't need the second line of below comments (the patch is motivated by the comment IIRC). /* * initialize the bitmap structure * if this returns an error, bitmap_destroy must be called to do clean up */ Thanks, Guoqing