[Crash-utility] Re: PATCH] Fix "kmem -i" and "swap" commands on Linux 6.10-rc1 and later kernels

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 12, 2024 at 2:08 PM <devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
Date: Wed, 12 Jun 2024 06:06:35 +0000
From: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
Subject: Re: PATCH] Fix "kmem -i" and "swap" commands
        on Linux 6.10-rc1 and later kernels
To: lijiang <lijiang@xxxxxxxxxx>, "devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx"
        <devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
Message-ID: <2db40a61-d404-4733-a62a-6068f2911346@xxxxxxx>
Content-Type: text/plain; charset="utf-8"

On 2024/06/12 13:55, lijiang wrote:
> Thank you for the fix, Kazu.
>
> On Tue, Jun 11, 2024 at 10:42 AM <devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx <mailto:devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx>> wrote:
>
>     Date: Tue, 11 Jun 2024 02:40:55 +0000
>     From: HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>>
>     Subject: [PATCH] Fix "kmem -i" and "swap" commands on
>              Linux 6.10-rc1 and later kernels
>     To: "devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx <mailto:devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx>"
>              <devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx <mailto:devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx>>
>     Message-ID: <1718073652-13444-1-git-send-email-k-hagio-ab@xxxxxxx <mailto:1718073652-13444-1-git-send-email-k-hagio-ab@xxxxxxx>>
>     Content-Type: text/plain; charset="iso-2022-jp"
>
>     Kernel commit 798cb7f9aec3 ("swapon(2)/swapoff(2): don't bother with
>     block size") removed swap_info_struct.old_block_size member at Linux
>     6.10-rc1.  The crash-utility has used this to determine whether a swap
>     is a partition or file and to determine the way to get the swap path.
>
>     Withtout the patch, the "kmem -i" and "swap" commands fail with the
>     following error messsage:
>
>        crash> kmem -i
>        ...
>           TOTAL HUGE  13179392      50.3 GB         ----
>            HUGE FREE  13179392      50.3 GB  100% of TOTAL HUGE
>
>        swap: invalid (optional) structure member offsets: swap_info_struct_swap_device or swap_info_struct_old_block_size
>              FILE: memory.c  LINE: 16032  FUNCTION: dump_swap_info()
>
>     The swap_file member of recent swap_info_struct is a pointer to a
>     struct file (once upon a time it was dentry), use this fact directly.
>
>     Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>>
>     ---
>     NOTE: This patch can be applied on top of my '[PATCH] Fix "kmem -v"
>     option on Linux 6.9 and later kernels' patch.  Otherwise, please adjust
>     the hunk for offset_table.
>
>       defs.h    |  1 +
>       filesys.c |  1 +
>       memory.c  | 28 +++++++++++++++++++++++-----
>       symbols.c |  1 +
>       4 files changed, 26 insertions(+), 5 deletions(-)
>
>     diff --git a/defs.h b/defs.h
>     index 95de33188070..64bf785b7a1b 100644
>     --- a/defs.h
>     +++ b/defs.h
>     @@ -2242,6 +2242,7 @@ struct offset_table {                    /* stash of commonly-used offsets */
>              long log_caller_id;
>              long vmap_node_busy;
>              long rb_list_head;
>     +       long file_f_inode;
>       };
>
>       struct size_table {         /* stash of commonly-used sizes */
>     diff --git a/filesys.c b/filesys.c
>     index 81fe856699e1..406ebb299780 100644
>     --- a/filesys.c
>     +++ b/filesys.c
>     @@ -2038,6 +2038,7 @@ vfs_init(void)
>              MEMBER_OFFSET_INIT(file_f_dentry, "file", "f_dentry");
>              MEMBER_OFFSET_INIT(file_f_vfsmnt, "file", "f_vfsmnt");
>              MEMBER_OFFSET_INIT(file_f_count, "file", "f_count");
>     +       MEMBER_OFFSET_INIT(file_f_inode, "file", "f_inode");
>              MEMBER_OFFSET_INIT(path_mnt, "path", "mnt");
>              MEMBER_OFFSET_INIT(path_dentry, "path", "dentry");
>              if (INVALID_MEMBER(file_f_dentry)) {
>     diff --git a/memory.c b/memory.c
>     index acb8507cfb75..28f901f16adf 100644
>     --- a/memory.c
>     +++ b/memory.c
>     @@ -16075,6 +16075,8 @@ dump_swap_info(ulong swapflags, ulong *totalswap_pages, ulong *totalused_pages)
>              char buf3[BUFSIZE];
>              char buf4[BUFSIZE];
>              char buf5[BUFSIZE];
>     +       int swap_file_is_file =
>     +               STREQ(MEMBER_TYPE_NAME("swap_info_struct", "swap_file"), "file");
>
>
> This patch looks good, but I still have one question:
> The 'swap_file' has been added into the struct swap_info_struct since linux v2.6.12-rc2,
> and until now it(its type) has been never changed, it indicates that the above checking is always true.
>
> STREQ(MEMBER_TYPE_NAME("swap_info_struct", "swap_file"), "file")
>
> Is that expected behavior? Just want to confirm with you.

Yes.

Thank you for the confirmation, and explanation below, Kazu. 

As far as I checked, swap_file was changed from 'dentry *' to 'file *'
at somewhere between v2.4.0 and v2.6.0, so not always true.

 
 Seems no full history in linux github. I checked it from the linux initial git repo:

commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 (tag: v2.6.12-rc2)
Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxx>
Date:   Sat Apr 16 15:20:36 2005 -0700

    Linux-2.6.12-rc2
   
    Initial git repository build. I'm not bothering with the full history,
    even though we have it. We can create a separate "historical" git
    archive of that later if we want to, and in the meantime it's about
    3.2GB when imported into git - space that would just make the early
    git days unnecessarily complicated, when we don't have a lot of good
    infrastructure for it.
   
    Let it rip!


get_pathname()'s first arg is dentry, we have to choose the else-if block
without old_block_size, so I decided to use its type information directly

                         } else if (VALID_MEMBER(swap_info_struct_old_block_size) ||
                                     swap_file_is_file) {

The other if / else blocks are for very old kernels, so maybe they won't
be used, but if we don't remove these, I think this is a reasonable way.

 
Sounds good to me.
 
Does this answer your question?
 
Yes. Thank you, Kazu. I have no other issues. For the patch: Ack.

Thanks
Lianbo
 

Thanks,
Kazu

>
> Thanks
> Lianbo
>
>              if (!symbol_exists("nr_swapfiles"))
>                      error(FATAL, "nr_swapfiles doesn't exist in this kernel!\n");
>     @@ -16118,9 +16120,21 @@ dump_swap_info(ulong swapflags, ulong *totalswap_pages, ulong *totalused_pages)
>                      swap_file = ULONG(vt->swap_info_struct +
>                              OFFSET(swap_info_struct_swap_file));
>
>     -                swap_device = INT(vt->swap_info_struct +
>     -                        OFFSET_OPTION(swap_info_struct_swap_device,
>     -                       swap_info_struct_old_block_size));
>     +               /* Linux 6.10 and later */
>     +               if (INVALID_MEMBER(swap_info_struct_swap_device) &&
>     +                   INVALID_MEMBER(swap_info_struct_old_block_size) &&
>     +                   swap_file_is_file) {
>     +                       ulong inode;
>     +                       ushort mode;
>     +                       readmem(swap_file + OFFSET(file_f_inode), KVADDR, &inode,
>     +                               sizeof(ulong), "swap_file.f_inode", FAULT_ON_ERROR);
>     +                       readmem(inode + OFFSET(inode_i_mode), KVADDR, &mode,
>     +                               sizeof(ushort), "inode.i_mode", FAULT_ON_ERROR);
>     +                       swap_device = S_ISBLK(mode);
>     +               } else
>     +                       swap_device = INT(vt->swap_info_struct +
>     +                               OFFSET_OPTION(swap_info_struct_swap_device,
>     +                               swap_info_struct_old_block_size));
>
>                       pages = INT(vt->swap_info_struct +
>                               OFFSET(swap_info_struct_pages));
>     @@ -16161,8 +16175,12 @@ dump_swap_info(ulong swapflags, ulong *totalswap_pages, ulong *totalused_pages)
>                                              OFFSET(swap_info_struct_swap_vfsmnt));
>                                      get_pathname(swap_file, buf, BUFSIZE,
>                                              1, vfsmnt);
>     -                       } else if (VALID_MEMBER
>     -                               (swap_info_struct_old_block_size)) {
>     +                       } else if (VALID_MEMBER(swap_info_struct_old_block_size) ||
>     +                                   swap_file_is_file) {
>     +                               /*
>     +                                * Linux 6.10 and later kernels do not have old_block_size,
>     +                                * but this still should work, if swap_file is file.
>     +                                */
>                                      devname = vfsmount_devname(file_to_vfsmnt(swap_file),
>                                              buf1, BUFSIZE);
>                                      get_pathname(file_to_dentry(swap_file),
>     diff --git a/symbols.c b/symbols.c
>     index 283b98a8fbfe..0bf050ab62d0 100644
>     --- a/symbols.c
>     +++ b/symbols.c
>     @@ -10522,6 +10522,7 @@ dump_offset_table(char *spec, ulong makestruct)
>                      OFFSET(file_f_count));
>               fprintf(fp, "                   file_f_path: %ld\n",
>                      OFFSET(file_f_path));
>     +        fprintf(fp, "                  file_f_inode: %ld\n", OFFSET(file_f_inode));
>               fprintf(fp, "                      path_mnt: %ld\n",
>                      OFFSET(path_mnt));
>               fprintf(fp, "                   path_dentry: %ld\n",
>     --
>     2.31.1
>
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki

[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux