On Thu, 24 Apr 2014 17:11:13 +0800
Wang Nan <wangnan0 at huawei.com> 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 <wangnan0 at huawei.com>
> Cc: Atsushi Kumagai <kumagai-atsushi at mxc.nes.nec.co.jp>
> Cc: kexec at lists.infradead.org
> Cc: Geng Hui <hui.geng at huawei.com>
> ---
> 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 ) {
> + 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));
> @@ -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);
