On 2014/4/25 16:03, Petr Tesarik wrote:
> On Fri, 25 Apr 2014 13:25:23 +0800
> Wang Nan <wangnan0 at huawei.com> wrote:
>
>> On 2014/4/24 17:28, Petr Tesarik wrote:
>>> On Thu, 24 Apr 2014 17:11:13 +0800
>>> Wang Nan <wangnan0 at 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
>
