[PATCH v2] makedumpfile: support _count -> _refcount rename in struct page

[Atsushi Kumagai]

>Pratyush Anand <panand at redhat.com> writes:
>
>> On 21/06/2016:10:05:42 AM, Vitaly Kuznetsov wrote:
>>> Pratyush Anand <panand at redhat.com> writes:
>>>
>>> > On 21/06/2016:05:43:29 AM, Atsushi Kumagai wrote:
>>> >> Hello Vitaly,
>>> >>
>>> >> >_count member was renamed to _refcount in linux commit 0139aa7b7fa12
>>> >> >("mm: rename _count, field of the struct page, to _refcount") and this
>>> >> >broke makedumpfile. The reason for making the change was to find all users
>>> >> >accessing it directly and not through the recommended API. I tried
>>> >> >suggesting to revert the change but failed, I see no other choice than to
>>> >> >start supporting both _count and _refcount in makedumpfile.
>>> >> >
>>> >> >Signed-off-by: Vitaly Kuznetsov <vkuznets at redhat.com>
>>> >> >---
>>> >> >Changes since 'v1':
>>> >> >- Make '_refcount' the default [Atsushi Kumagai]
>>> >
>>> > Sorry, I missed this conversation earlier.
>>> >
>>> >>
>>> >> Good fix, I'll merge this into v1.6.1.
>>> >>
>>> >> Thanks,
>>> >> Atsushi Kumagai
>>> >>
>>> >> >---
>>> >> > makedumpfile.c | 26 +++++++++++++++++++++-----
>>> >> > makedumpfile.h |  3 ++-
>>> >> > 2 files changed, 23 insertions(+), 6 deletions(-)
>>> >> >
>>> >> >diff --git a/makedumpfile.c b/makedumpfile.c
>>> >> >index 853b999..fd884d3 100644
>>> >> >--- a/makedumpfile.c
>>> >> >+++ b/makedumpfile.c
>>> >> >@@ -1579,7 +1579,14 @@ get_structure_info(void)
>>> >> > 	 */
>>> >> > 	SIZE_INIT(page, "page");
>>> >> > 	OFFSET_INIT(page.flags, "page", "flags");
>>> >> >-	OFFSET_INIT(page._count, "page", "_count");
>>> >> >+	OFFSET_INIT(page._refcount, "page", "_refcount");
>>> >> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
>>> >> >+		info->flag_use_count = TRUE;
>>> >> >+		OFFSET_INIT(page._refcount, "page", "_count");
>>> >> >+	} else {
>>> >> >+		info->flag_use_count = FALSE;
>>> >
>>> > Probably we could have avoided this flag and could have solved it same way as we
>>> > have solved for other such kernel changes, like that of "module.module_core".
>>> > Infact, we do not need to add "_refcount" in makedumpfile's page struct.
>>> >
>>>
>>> Yes, the flag is only needed to do the write. Why do you think we don't
>>> need it?
>>
>> IMHO, even if we write with "page._count", we would be able to get correct
>> value in next read, because we prefer reading with "page._count" over
>> "page._refcount". I think "crash utility" also gives priority for reading with
>> _count, so things should work without any issue.
>
>While things will probably work now while everyone still remembers the
>old "_count" name it may happen that some tool will drop this support in
>future and this will lead to an awkward situation when everythin works
>with /proc/vmcore but doesn't work with makedumpfile's output. Said that
>I think it makes sense for makedumpfile to be precise and write what we
>read. IMO, of course, I'm not a regular makedumpfile contributor.

Yeah, I think it would be better to write down the correct current name
even if the code gets a bit more complicated, thus I accepted the Vitaly's patch.


Thanks,
Atsushi Kumagai