[PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[Pingfan Liu]

In refiltering mode, each sub process inherits bitmap2->fd from parent.
Then they lseek()/read() on the same fd, which means that they interference
with each other.

This breaks the purpose of SPLITTING_FD_BITMAP(i) for each sub process.
Fix it by assigning a sub process dedicated fd to bitmap2->fd.

Signed-off-by: Pingfan Liu <piliu@redhat.com>
---
 makedumpfile.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index d76a435..1dc8640 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -8857,7 +8857,8 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
 	if (info->flag_cyclic) {
 		if (!prepare_bitmap2_buffer())
 			return FALSE;
-	}
+	} else if (info->flag_refiltering)
+		info->bitmap2->fd = info->fd_bitmap;
 
 	/*
 	 * Write pages and bitmap cyclically.
-- 
2.7.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[Kazuhito Hagio]

Hi Pingfan,

> -----Original Message-----
> In refiltering mode, each sub process inherits bitmap2->fd from parent.
> Then they lseek()/read() on the same fd, which means that they interference
> with each other.
> 
> This breaks the purpose of SPLITTING_FD_BITMAP(i) for each sub process.
> Fix it by assigning a sub process dedicated fd to bitmap2->fd.
> 
> Signed-off-by: Pingfan Liu <piliu@redhat.com>

Thanks for the patch.
I'm still reading the code, but it might be better to apply this to bitmap1->fd
as well?  see you next week..

Thanks,
Kazu

> ---
>  makedumpfile.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index d76a435..1dc8640 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -8857,7 +8857,8 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
>  	if (info->flag_cyclic) {
>  		if (!prepare_bitmap2_buffer())
>  			return FALSE;
> -	}
> +	} else if (info->flag_refiltering)
> +		info->bitmap2->fd = info->fd_bitmap;
> 
>  	/*
>  	 * Write pages and bitmap cyclically.
> --
> 2.7.5
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[piliu]

On 12/07/2019 06:11 AM, Kazuhito Hagio wrote:
> Hi Pingfan,
> 
>> -----Original Message-----
>> In refiltering mode, each sub process inherits bitmap2->fd from parent.
>> Then they lseek()/read() on the same fd, which means that they interference
>> with each other.
>>
>> This breaks the purpose of SPLITTING_FD_BITMAP(i) for each sub process.
>> Fix it by assigning a sub process dedicated fd to bitmap2->fd.
>>
>> Signed-off-by: Pingfan Liu <piliu@redhat.com>
> 
> Thanks for the patch.
> I'm still reading the code, but it might be better to apply this to bitmap1->fd
> as well?  see you next week..
Yes. Although during my test, bitmap1 is not touched, but it is a
reasonable step to against any future bug.

Thanks,
Pingfan
> 
> Thanks,
> Kazu
> 
>> ---
>>  makedumpfile.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/makedumpfile.c b/makedumpfile.c
>> index d76a435..1dc8640 100644
>> --- a/makedumpfile.c
>> +++ b/makedumpfile.c
>> @@ -8857,7 +8857,8 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
>>  	if (info->flag_cyclic) {
>>  		if (!prepare_bitmap2_buffer())
>>  			return FALSE;
>> -	}
>> +	} else if (info->flag_refiltering)
>> +		info->bitmap2->fd = info->fd_bitmap;
>>
>>  	/*
>>  	 * Write pages and bitmap cyclically.
>> --
>> 2.7.5
>>
> 
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[Kazuhito Hagio]

> -----Original Message-----
> From: piliu <piliu@redhat.com>
> Sent: Monday, December 9, 2019 1:06 AM
> To: Hagio Kazuhito(萩尾 一仁) <k-hagio@ab.jp.nec.com>; kexec@lists.infradead.org
> Subject: Re: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering
> 
> 
> 
> On 12/07/2019 06:11 AM, Kazuhito Hagio wrote:
> > Hi Pingfan,
> >
> >> -----Original Message-----
> >> In refiltering mode, each sub process inherits bitmap2->fd from parent.
> >> Then they lseek()/read() on the same fd, which means that they interference
> >> with each other.
> >>
> >> This breaks the purpose of SPLITTING_FD_BITMAP(i) for each sub process.
> >> Fix it by assigning a sub process dedicated fd to bitmap2->fd.
> >>
> >> Signed-off-by: Pingfan Liu <piliu@redhat.com>
> >
> > Thanks for the patch.
> > I'm still reading the code, but it might be better to apply this to bitmap1->fd
> > as well?  see you next week..
> Yes. Although during my test, bitmap1 is not touched, but it is a
> reasonable step to against any future bug.

Reading the code, I think
- the issue might occur not only in refiltering, but also the first filtering
  with --split and --work-dir option (forced non-cyclic mode).
- pefer to gather things for --split option into writeout_multiple_dumpfiles()
  if we can, for readability.

So does the following patch work for you and your test?
I could not have reproduced the issue yet.

diff --git a/makedumpfile.c b/makedumpfile.c
index b9e9dfbd45ba..674c6a00e2dd 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -10091,6 +10091,10 @@ writeout_multiple_dumpfiles(void)
 			info->split_start_pfn = SPLITTING_START_PFN(i);
 			info->split_end_pfn   = SPLITTING_END_PFN(i);
 
+			if (!info->flag_cyclic) {
+				info->bitmap1->fd = info->fd_bitmap;
+				info->bitmap2->fd = info->fd_bitmap;
+			}
 			if (!reopen_dump_memory())
 				exit(1);
 			if ((status = writeout_dumpfile()) == FALSE)


BTW, what do you see when the issue occurs? an error or broken dump?

Thanks,
Kazu

> 
> Thanks,
> Pingfan
> >
> > Thanks,
> > Kazu
> >
> >> ---
> >>  makedumpfile.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/makedumpfile.c b/makedumpfile.c
> >> index d76a435..1dc8640 100644
> >> --- a/makedumpfile.c
> >> +++ b/makedumpfile.c
> >> @@ -8857,7 +8857,8 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
> >>  	if (info->flag_cyclic) {
> >>  		if (!prepare_bitmap2_buffer())
> >>  			return FALSE;
> >> -	}
> >> +	} else if (info->flag_refiltering)
> >> +		info->bitmap2->fd = info->fd_bitmap;
> >>
> >>  	/*
> >>  	 * Write pages and bitmap cyclically.
> >> --
> >> 2.7.5
> >>
> >
> >
> >
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
> >
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[piliu]

On 12/10/2019 04:10 AM, Kazuhito Hagio wrote:
> 
>> -----Original Message-----
>> From: piliu <piliu@redhat.com>
>> Sent: Monday, December 9, 2019 1:06 AM
>> To: Hagio Kazuhito(萩尾 一仁) <k-hagio@ab.jp.nec.com>; kexec@lists.infradead.org
>> Subject: Re: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering
>>
>>
>>
>> On 12/07/2019 06:11 AM, Kazuhito Hagio wrote:
>>> Hi Pingfan,
>>>
>>>> -----Original Message-----
>>>> In refiltering mode, each sub process inherits bitmap2->fd from parent.
>>>> Then they lseek()/read() on the same fd, which means that they interference
>>>> with each other.
>>>>
>>>> This breaks the purpose of SPLITTING_FD_BITMAP(i) for each sub process.
>>>> Fix it by assigning a sub process dedicated fd to bitmap2->fd.
>>>>
>>>> Signed-off-by: Pingfan Liu <piliu@redhat.com>
>>>
>>> Thanks for the patch.
>>> I'm still reading the code, but it might be better to apply this to bitmap1->fd
>>> as well?  see you next week..
>> Yes. Although during my test, bitmap1 is not touched, but it is a
>> reasonable step to against any future bug.
> 
> Reading the code, I think
> - the issue might occur not only in refiltering, but also the first filtering
>   with --split and --work-dir option (forced non-cyclic mode).
> - pefer to gather things for --split option into writeout_multiple_dumpfiles()
>   if we can, for readability.
Yes, all of the cases suffer from sharing fd across processes
> 
> So does the following patch work for you and your test?
> I could not have reproduced the issue yet.
I tried to fetch a machine to test. It pass 50 times test with your
patch. While if without this patch, it failed about 1 out of 4 times.
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index b9e9dfbd45ba..674c6a00e2dd 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -10091,6 +10091,10 @@ writeout_multiple_dumpfiles(void)
>  			info->split_start_pfn = SPLITTING_START_PFN(i);
>  			info->split_end_pfn   = SPLITTING_END_PFN(i);
>  
> +			if (!info->flag_cyclic) {
> +				info->bitmap1->fd = info->fd_bitmap;
> +				info->bitmap2->fd = info->fd_bitmap;
> +			}
>  			if (!reopen_dump_memory())
>  				exit(1);
>  			if ((status = writeout_dumpfile()) == FALSE)
> 
> 
> BTW, what do you see when the issue occurs? an error or broken dump?
The test case is refiltering, "makedumpfile --split  -d 31
/root/vmcore-p9b-21 dumpfile_{1,2,3} 2>&1"
And it can not complete the dump.

Thanks,
Pingfan

> 
> Thanks,
> Kazu
> 
>>
>> Thanks,
>> Pingfan
>>>
>>> Thanks,
>>> Kazu
>>>
>>>> ---
>>>>  makedumpfile.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/makedumpfile.c b/makedumpfile.c
>>>> index d76a435..1dc8640 100644
>>>> --- a/makedumpfile.c
>>>> +++ b/makedumpfile.c
>>>> @@ -8857,7 +8857,8 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
>>>>  	if (info->flag_cyclic) {
>>>>  		if (!prepare_bitmap2_buffer())
>>>>  			return FALSE;
>>>> -	}
>>>> +	} else if (info->flag_refiltering)
>>>> +		info->bitmap2->fd = info->fd_bitmap;
>>>>
>>>>  	/*
>>>>  	 * Write pages and bitmap cyclically.
>>>> --
>>>> 2.7.5
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> kexec mailing list
>>> kexec@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>
>>
> 
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[Kazuhito Hagio]

Hi Pingfan,

> -----Original Message-----
> > Reading the code, I think
> > - the issue might occur not only in refiltering, but also the first filtering
> >   with --split and --work-dir option (forced non-cyclic mode).
> > - pefer to gather things for --split option into writeout_multiple_dumpfiles()
> >   if we can, for readability.
> Yes, all of the cases suffer from sharing fd across processes
> >
> > So does the following patch work for you and your test?
> > I could not have reproduced the issue yet.
> I tried to fetch a machine to test. It pass 50 times test with your
> patch. While if without this patch, it failed about 1 out of 4 times.

Good. Thank you for testing.

> > BTW, what do you see when the issue occurs? an error or broken dump?
> The test case is refiltering, "makedumpfile --split  -d 31
> /root/vmcore-p9b-21 dumpfile_{1,2,3} 2>&1"
> And it can not complete the dump.

Ah, I got it. I probably could reproduce the issue:
---
Excluding unnecessary pages                       : [100.0 %] \
readpage_kdump_compressed: pfn(2fc1000) is excluded from vmcore.
readmem: type_addr: 1, addr:2fc1000000, size:4096
read_pfn: Can't get the page data.
...
writeout_multiple_dumpfiles: Child process(30993) finished incompletely.(256)

makedumpfile Failed.
---

Refiltering was the factor that makedumpfile failed, but I think this bug
also can break the dumpfile silently by excluding pages wrongly even in
first filtering.

I guess this bug was hard to debug. Thank you so much for working on this!
Modified your patch and pushed.
https://sourceforge.net/p/makedumpfile/code/ci/5519b3eba68544dc484d85e9540d440d93f8c924/

Thanks,
Kazu




_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] makedumpfile: assign bitmap2 fd for sub process during refiltering

[piliu]

On 12/12/2019 01:44 AM, Kazuhito Hagio wrote:
> Hi Pingfan,
> 
>> -----Original Message-----
>>> Reading the code, I think
>>> - the issue might occur not only in refiltering, but also the first filtering
>>>   with --split and --work-dir option (forced non-cyclic mode).
>>> - pefer to gather things for --split option into writeout_multiple_dumpfiles()
>>>   if we can, for readability.
>> Yes, all of the cases suffer from sharing fd across processes
>>>
>>> So does the following patch work for you and your test?
>>> I could not have reproduced the issue yet.
>> I tried to fetch a machine to test. It pass 50 times test with your
>> patch. While if without this patch, it failed about 1 out of 4 times.
> 
> Good. Thank you for testing.
> 
>>> BTW, what do you see when the issue occurs? an error or broken dump?
>> The test case is refiltering, "makedumpfile --split  -d 31
>> /root/vmcore-p9b-21 dumpfile_{1,2,3} 2>&1"
>> And it can not complete the dump.
> 
> Ah, I got it. I probably could reproduce the issue:
> ---
> Excluding unnecessary pages                       : [100.0 %] \
> readpage_kdump_compressed: pfn(2fc1000) is excluded from vmcore.
> readmem: type_addr: 1, addr:2fc1000000, size:4096
> read_pfn: Can't get the page data.
> ...
> writeout_multiple_dumpfiles: Child process(30993) finished incompletely.(256)
> 
> makedumpfile Failed.
> ---
> 
> Refiltering was the factor that makedumpfile failed, but I think this bug
> also can break the dumpfile silently by excluding pages wrongly even in
> first filtering.
Yes, you are right.
> 
> I guess this bug was hard to debug. Thank you so much for working on this!
Yeah, it took me some time. I had thought it should be a kernel bug
about the file{}.
> Modified your patch and pushed.
> https://sourceforge.net/p/makedumpfile/code/ci/5519b3eba68544dc484d85e9540d440d93f8c924/
> 
Thanks,
Pingfan


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec