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

[Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>]

>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