[PATCH] makedumpfile: code cleanup: set_bitmap

[PATCH] makedumpfile: code cleanup: set_bitmap

[Wang Nan]

This patch makes set_bitmap() to call sync_bitmap() instead rewrite
identical code to do same thing.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@huawei.com>
---
 makedumpfile.c | 70 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 75092a8..8e6ddce 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);
 
-	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));
@@ -3379,33 +3398,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);
-- 
1.8.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH] makedumpfile: code cleanup: set_bitmap

[Atsushi Kumagai]

Hello Wang,

>This patch makes set_bitmap() to call sync_bitmap() instead rewrite
>identical code to do same thing.

I found a simple mistake.

>@@ -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);
>
>-	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;
> 		}

sync_bitmap() returns TRUE(1) when it succeeds, so this check is wrong.


Thanks
Atsushi Kumagai

>-	}
>-	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));
>@@ -3379,33 +3398,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);
>--
>1.8.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2] makedumpfile: code cleanup: set_bitmap

[Wang Nan]

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.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@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);
 
-	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);
-- 
1.8.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] makedumpfile: code cleanup: set_bitmap

[Petr Tesarik]

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] makedumpfile: code cleanup: set_bitmap

[Wang Nan]

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

[ .. 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

Re: [PATCH v2] makedumpfile: code cleanup: set_bitmap

[Petr Tesarik]

On Fri, 25 Apr 2014 13:25:23 +0800
Wang Nan <wangnan0@huawei.com> wrote:

> On 2014/4/24 17:28, Petr Tesarik wrote:
> > On Thu, 24 Apr 2014 17:11:13 +0800
> > Wang Nan <wangnan0@huawei.com> wrote:
>[...]
> >>  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.

True, but it will be calculated only once (in sync_bitmap). OTOH the
block number is always needed, because it is stored in bitmap->no_block.

Okay, that can be changed, and you can store the offset instead. I
don't have a strong opinion on this.

> In addition, I have another idea: what about to replace all lseek .. read/write pattern to pread/pwrite?

Definitely! After doing that, we could even reuse the same file descriptor
for all processes.

Now, since the original patch looks good as it is, let's see if Atsushi
Kumagai takes it into the tree and post more clean up patches afterwards.

Petr T

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] makedumpfile: code cleanup: set_bitmap

[Wang Nan]

On 2014/4/25 16:03, Petr Tesarik wrote:
> On Fri, 25 Apr 2014 13:25:23 +0800
> Wang Nan <wangnan0@huawei.com> wrote:
> 
>> On 2014/4/24 17:28, Petr Tesarik wrote:
>>> On Thu, 24 Apr 2014 17:11:13 +0800
>>> Wang Nan <wangnan0@huawei.com> wrote:
>> [...]
>>>>  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.
> 
> True, but it will be calculated only once (in sync_bitmap). OTOH the
> block number is always needed, because it is stored in bitmap->no_block.
> 
> Okay, that can be changed, and you can store the offset instead. I
> don't have a strong opinion on this.
> 
>> In addition, I have another idea: what about to replace all lseek .. read/write pattern to pread/pwrite?
> 
> Definitely! After doing that, we could even reuse the same file descriptor
> for all processes.
> 

I posted a series of patches for lseek. If Atsushi Kumagai accept them, I will
redo this patch on top of them.

> Now, since the original patch looks good as it is, let's see if Atsushi
> Kumagai takes it into the tree and post more clean up patches afterwards.
> 
> Petr T
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH v2] makedumpfile: code cleanup: set_bitmap

[Atsushi Kumagai]

>On 2014/4/25 16:03, Petr Tesarik wrote:
>> On Fri, 25 Apr 2014 13:25:23 +0800
>> Wang Nan <wangnan0@huawei.com> wrote:
>>
>>> On 2014/4/24 17:28, Petr Tesarik wrote:
>>>> On Thu, 24 Apr 2014 17:11:13 +0800
>>>> Wang Nan <wangnan0@huawei.com> wrote:
>>> [...]
>>>>>  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.
>>
>> True, but it will be calculated only once (in sync_bitmap). OTOH the
>> block number is always needed, because it is stored in bitmap->no_block.
>>
>> Okay, that can be changed, and you can store the offset instead. I
>> don't have a strong opinion on this.
>>
>>> In addition, I have another idea: what about to replace all lseek .. read/write pattern to pread/pwrite?
>>
>> Definitely! After doing that, we could even reuse the same file descriptor
>> for all processes.
>>
>
>I posted a series of patches for lseek. If Atsushi Kumagai accept them, I will
>redo this patch on top of them.

I haven't gotten a chance to review them yet, but I accept your idea.
So please give me a time for reviewing.


Thanks
Atsushi Kumagai

>> Now, since the original patch looks good as it is, let's see if Atsushi
>> Kumagai takes it into the tree and post more clean up patches afterwards.
>>
>> Petr T
>>
>


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec