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

[Wang Nan <wangnan0@huawei.com>]

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