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.
<skip>
--
Vitaly
