From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from szxga01-in.huawei.com ([119.145.14.64]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WdYep-00084D-8Q for kexec@lists.infradead.org; Fri, 25 Apr 2014 05:26:45 +0000 Message-ID: <5359F1C3.5030100@huawei.com> Date: Fri, 25 Apr 2014 13:25:23 +0800 From: Wang Nan MIME-Version: 1.0 Subject: Re: [PATCH v2] makedumpfile: code cleanup: set_bitmap References: <0910DD04CBD6DE4193FCF86B9C00BE97205CAA@BPXM01GP.gisp.nec.co.jp> <1398330673-23016-1-git-send-email-wangnan0@huawei.com> <20140424112815.398f9852@hananiah.suse.cz> In-Reply-To: <20140424112815.398f9852@hananiah.suse.cz> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Petr Tesarik Cc: kexec@lists.infradead.org, kumagai-atsushi@mxc.nes.nec.co.jp, Geng Hui On 2014/4/24 17:28, Petr Tesarik wrote: > On Thu, 24 Apr 2014 17:11:13 +0800 > Wang Nan wrote: > >> This patch makes set_bitmap() to call sync_bitmap() instead rewrite >> identical code to do same thing. >> >> Change from v1: >> >> - fix a simple mistake: >> sync_bitmap() returns TRUE(1) when it succeeds, so use >> (!sync_bitmap()) for checking. > > Hi Wang Nan, > > I like your change. See my comments below: > >> Signed-off-by: Wang Nan >> Cc: Atsushi Kumagai >> Cc: kexec@lists.infradead.org >> Cc: Geng Hui >> --- >> makedumpfile.c | 70 ++++++++++++++++++++++++++-------------------------------- >> 1 file changed, 31 insertions(+), 39 deletions(-) >> >> diff --git a/makedumpfile.c b/makedumpfile.c >> index ce4a866..cea37a2 100644 >> --- a/makedumpfile.c >> +++ b/makedumpfile.c >> @@ -3309,6 +3309,34 @@ initialize_2nd_bitmap(struct dump_bitmap *bitmap) >> } >> >> int >> +sync_bitmap(struct dump_bitmap *bitmap) >> +{ >> + off_t offset; >> + offset = bitmap->offset + BUFSIZE_BITMAP * bitmap->no_block; >> + >> + /* >> + * The bitmap buffer is not dirty, and it is not necessary >> + * to write out it. >> + */ >> + if (bitmap->no_block < 0) >> + return TRUE; >> + >> + if (lseek(bitmap->fd, offset, SEEK_SET) < 0 ) { [ .. see below .. ] >> + ERRMSG("Can't seek the bitmap(%s). %s\n", >> + bitmap->file_name, strerror(errno)); >> + return FALSE; >> + } >> + if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP) >> + != BUFSIZE_BITMAP) { >> + ERRMSG("Can't write the bitmap(%s). %s\n", >> + bitmap->file_name, strerror(errno)); >> + return FALSE; >> + } >> + return TRUE; >> +} >> + >> + >> +int >> set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn, >> int val) >> { >> @@ -3317,20 +3345,11 @@ set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn, >> old_offset = bitmap->offset + BUFSIZE_BITMAP * bitmap->no_block; >> new_offset = bitmap->offset + BUFSIZE_BITMAP * (pfn / PFN_BUFBITMAP); > > After applying this patch, both old_offset and new_offset are > calculated only to compare for equality. And new_offset is in fact > computed twice (once in set_bitmap and once in sync_bitmap). > > This could be cleaned up by replacing the offsets with: > > int new_no_block = pfn / PFN_BUFBITMAP; > > Then change all occurrences of if (old_offset != new_offset) to: > > if (bitmap->no_block != new_no_block) > > and finally re-use new_no_block in the assignment to bitmap->no_block > near the end of the function, like this: > > - bitmap->no_block = pfn / PFN_BUFBITMAP; > + bitmap->no_block = new_no_block; > > Petr T > >> >> - if (0 <= bitmap->no_block && old_offset != new_offset) { >> - if (lseek(bitmap->fd, old_offset, SEEK_SET) < 0 ) { >> - ERRMSG("Can't seek the bitmap(%s). %s\n", >> - bitmap->file_name, strerror(errno)); >> - return FALSE; >> - } >> - if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP) >> - != BUFSIZE_BITMAP) { >> - ERRMSG("Can't write the bitmap(%s). %s\n", >> - bitmap->file_name, strerror(errno)); >> + if (old_offset != new_offset) { >> + if (!sync_bitmap(bitmap)) { >> + ERRMSG("Can't sync bitmap\n"); >> return FALSE; >> } >> - } >> - if (old_offset != new_offset) { >> if (lseek(bitmap->fd, new_offset, SEEK_SET) < 0 ) { >> ERRMSG("Can't seek the bitmap(%s). %s\n", >> bitmap->file_name, strerror(errno)); [ .. see below .. ] Good suggestion, but new_offset is still needed to be calculated for these lseeks. In addition, I have another idea: what about to replace all lseek .. read/write pattern to pread/pwrite? >> @@ -3386,33 +3405,6 @@ set_bitmap_cyclic(char *bitmap, unsigned long long pfn, int val, struct cycle *c >> } >> >> int >> -sync_bitmap(struct dump_bitmap *bitmap) >> -{ >> - off_t offset; >> - offset = bitmap->offset + BUFSIZE_BITMAP * bitmap->no_block; >> - >> - /* >> - * The bitmap buffer is not dirty, and it is not necessary >> - * to write out it. >> - */ >> - if (bitmap->no_block < 0) >> - return TRUE; >> - >> - if (lseek(bitmap->fd, offset, SEEK_SET) < 0 ) { >> - ERRMSG("Can't seek the bitmap(%s). %s\n", >> - bitmap->file_name, strerror(errno)); >> - return FALSE; >> - } >> - if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP) >> - != BUFSIZE_BITMAP) { >> - ERRMSG("Can't write the bitmap(%s). %s\n", >> - bitmap->file_name, strerror(errno)); >> - return FALSE; >> - } >> - return TRUE; >> -} >> - >> -int >> sync_1st_bitmap(void) >> { >> return sync_bitmap(info->bitmap1); > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec