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:
>
>> 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 at huawei.com>
>> Cc: Atsushi Kumagai <kumagai-atsushi at mxc.nes.nec.co.jp>
>> Cc: kexec at lists.infradead.org
>> Cc: Geng Hui <hui.geng at 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);
>
