>On 2014/4/25 16:03, Petr Tesarik wrote: >> On Fri, 25 Apr 2014 13:25:23 +0800 >> Wang Nan <[email protected]> wrote: >> >>> On 2014/4/24 17:28, Petr Tesarik wrote: >>>> On Thu, 24 Apr 2014 17:11:13 +0800 >>>> Wang Nan <[email protected]> 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 [email protected] http://lists.infradead.org/mailman/listinfo/kexec
