Hi Kazu,

On Fri, Jun 28, 2024 at 12:41 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio...@nec.com> wrote:
>
> Hi Tao,
>
> thank you for your review.
>
> On 2024/06/28 9:19, Tao Liu wrote:
> > Hi Kazu,
> >
> > sorry for the late response.
> >
> > On Tue, Jun 11, 2024 at 2:41 PM HAGIO KAZUHITO(萩尾 一仁)
> > <k-hagio...@nec.com> wrote:
> >>
> >> 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...@nec.com>
> >> ---
> >> 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");
> >>
> >>          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));
> >>
> >
> > The above code change is OK to me, just my curiosity: it looks to me
> > the swap_file->f_inode->i_mode->S_ISBLK approach will always work, not
> > only for the Linux 6.10 and later versions right? E.g. I made the
> > following change which works as expected on a 4.0.4-201 kernel:
>
> This S_ISBLK approach will work with kernels that swap_file is 'file',
> and at least 2.6.0 and later kernels have it, as far as I checked (as
> written in [1]).
>
> but I didn't want to change the current behavior (the else block) for
> 6.9 and older kernels, because it has worked well with them for a long
> time and there is no need to change.
>

OK, ageed. Thanks a lot for your explanation! The patch is good to me, so ack.

Thanks,
Tao Liu

> Does this answer your question?
>
> [1]
> https://lists.crash-utility.osci.io/archives/list/devel@lists.crash-utility.osci.io/message/46C66KSU6JN3RURAPMQAKON6TPHCLVNW/
>
> Thanks,
> Kazu
>
> >
> > @@ -16121,9 +16121,7 @@ dump_swap_info(ulong swapflags, ulong
> > *totalswap_pages, ulong *totalused_pages)
> >                          OFFSET(swap_info_struct_swap_file));
> >
> >                  /* 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) {
> > +               if (1) {
> >                          ulong inode;
> >                          ushort mode;
> >                          readmem(swap_file + OFFSET(file_f_inode),
> > KVADDR, &inode,
> >
> >        KERNEL: 
> > burn.khw2.lab.eng.bos.redhat.com/4.0.4-201.fc21_sysrq-c/vmlinux.gz
> >      DUMPFILE: 
> > burn.khw2.lab.eng.bos.redhat.com/4.0.4-201.fc21_sysrq-c/vmcore
> >   [PARTIAL DUMP]
> >          CPUS: 4
> >          DATE: Wed May 27 14:23:22 EDT 2015
> >        UPTIME: 02:47:56
> > LOAD AVERAGE: 0.00, 0.04, 0.05
> >         TASKS: 135
> >      NODENAME: dell-pec5125-04.lab.bos.redhat.com
> >       RELEASE: 4.0.4-201.fc21.x86_64
> >       VERSION: #1 SMP Thu May 21 15:58:47 UTC 2015
> >       MACHINE: x86_64  (2599 Mhz)
> >        MEMORY: 2 GB
> >         PANIC: "sysrq: SysRq : Trigger a crash"
> >           PID: 14037
> >       COMMAND: "bash"
> >          TASK: ffff88006a03d970  [THREAD_INFO: ffff880074734000]
> >           CPU: 1
> >         STATE: TASK_RUNNING (SYSRQ)
> >
> > crash> swap
> > SWAP_INFO_STRUCT    TYPE       SIZE       USED     PCT  PRI  FILENAME
> > ffff88007bf62000  PARTITION  2097148k      0k       0%   -1  /dev/dm-1
> > crash> kmem -i
> >                   PAGES        TOTAL      PERCENTAGE
> >      TOTAL MEM   445983       1.7 GB         ----
> >           FREE   275307       1.1 GB   61% of TOTAL MEM
> > ...
> >
> > Thanks,
> > Tao Liu
> >
> >>                   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@lists.crash-utility.osci.io
> >> To unsubscribe send an email to devel-le...@lists.crash-utility.osci.io
> >> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> >> Contribution Guidelines: https://github.com/crash-utility/crash/wiki
--
Crash-utility mailing list -- devel@lists.crash-utility.osci.io
To unsubscribe send an email to devel-le...@lists.crash-utility.osci.io
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki

Reply via email to