----- Original Message -----
> Hello Dave,
>
> New kernel has moved some elements of struct vfsmount to a new struct
> mount. So crash will not act normally on new kernel. The patch is used
> to fix the problem.
>
> Please check.

Hi Qiao,

This patch-set is very much appreciated. I saw Al Viro's original LKML patch
posted back in December (?), and had been waiting to see an actual kernel with
it in place. The most recent in-house RHEL/Fedora test kernels I have on-hand
are all 3.2.x-era kernels, which don't have the patch, so I'm presuming you're
running on a 3.3.x kernel?

Anyway, I'll check this patch out for backwards-compatibility and try to
get a more recent kernel to test with. It looks good on paper...

And again, thanks for taking on this task.

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

----- Original Message -----
>
>
> ----- Original Message -----
> > Hello Dave,
> >
> > New kernel has moved some elements of struct vfsmount to a new struct
> > mount. So crash will not act normally on new kernel. The patch is used
> > to fix the problem.
> >
> > Please check.
>
> Hi Qiao,
>
> This patch-set is very much appreciated. I saw Al Viro's original LKML patch
> posted back in December (?), and had been waiting to see an actual kernel with
> it in place. The most recent in-house RHEL/Fedora test kernels I have on-hand
> are all 3.2.x-era kernels, which don't have the patch, so I'm presuming you're
> running on a 3.3.x kernel?
>
> Anyway, I'll check this patch out for backwards-compatibility and try to
> get a more recent kernel to test with. It looks good on paper...
>
> And again, thanks for taking on this task.
>
> Dave

I provisioned a system with a Fedora 3.3.0-0.rc6.git2.1.fc17 kernel, which
has the vfsmount->mount patch. The session comes up cleanly, but the
following commands fail:

crash> mount
... [ cut ] ...
mount: invalid structure member offset: vfsmount_mnt_list
...

crash> files
... [cut] ...
files: invalid structure member offset: dentry_d_covers
...

The "fuser" command generates the above error because it uses the
"files" command behind the scenes.

crash> vm
... [cut] ...
vm: invalid structure member offset: dentry_d_covers
...

crash> swap
... [cut] ...
swap: invalid structure member offset: dentry_d_covers
...

With your patch applied, "mount", "files", "fuser" and "vm" all
work OK.

But "swap" now fails in a different manner:

crash> swap
FILENAME TYPE SIZE USED PCT PRIORITY
swap: invalid kernel virtual address: 1d8ec8 type: "fill_dentry_cache"
crash>

It's failing in its call to get_pathname(). I haven't had a chance to
fully check into why it's getting a bogus parent dentry address
while walking through the swap file's "/dev/mapper/vg_dellpec610002-lv_swap"
pathname -- and I won't be able to until next week. But presumably it's
related, and so if you (or anybody) gets a chance this weekend, please
take a look.

Thanks,
Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

----- Original Message -----
> At 2012-3-10 5:50, Dave Anderson wrote:
> >
> > It's failing in its call to get_pathname(). I haven't had a chance to
>
> You are right. I forgot to check every call to get_pathname. The new
> patch has fixed the problem of swap.

Although the patch appears to work in the simple cases that I have tested,
I have a few issues with it.

First, the get_pathname() function prototype is -- and has always been -- this:

void get_pathname(ulong dentry, char *pathname, int length, int full, ulong vfsmnt)

where the "vfsmnt" argument has always been a struct vfsmount pointer.

With your patch, prior to making get_pathname() calls, you typically
make a VALID_STRUCT(mount) call first, and if true, you modify the
vfsmnt argument to be a struct mount pointer.

It would make more sense maintenance-wise, and would simplify the patch,
if the get_pathname() "vfsmnt" argument would continue to be a struct
vfsmount pointer. And at the top of get_pathname(), you could make the
adjustments for when it is embedded inside a struct mount.

For example, taking these patches to open_files_dump() and file_dump():

@@ -2226,12 +2299,16 @@ open_files_dump(ulong task, int flags, struct reference *ref)
if (VALID_MEMBER(fs_struct_rootmnt)) {
vfsmnt = ULONG(fs_struct_buf +
OFFSET(fs_struct_rootmnt));
+ if (VALID_STRUCT(mount))
+ vfsmnt = vfsmnt - OFFSET(mount_mnt);
get_pathname(root_dentry, root_pathname,
BUFSIZE, 1, vfsmnt);
} else if (use_path) {
vfsmnt = ULONG(fs_struct_buf +
OFFSET(fs_struct_root) +
OFFSET(path_mnt));
+ if (VALID_STRUCT(mount))
+ vfsmnt = vfsmnt - OFFSET(mount_mnt);
get_pathname(root_dentry, root_pathname,
BUFSIZE, 1, vfsmnt);
} else {
@@ -2250,12 +2327,16 @@ open_files_dump(ulong task, int flags, struct reference *ref)
if (VALID_MEMBER(fs_struct_pwdmnt)) {
vfsmnt = ULONG(fs_struct_buf +
OFFSET(fs_struct_pwdmnt));
+ if (VALID_STRUCT(mount))
+ vfsmnt = vfsmnt - OFFSET(mount_mnt);
get_pathname(pwd_dentry, pwd_pathname,
BUFSIZE, 1, vfsmnt);
} else if (use_path) {
vfsmnt = ULONG(fs_struct_buf +
OFFSET(fs_struct_pwd) +
OFFSET(path_mnt));
+ if (VALID_STRUCT(mount))
+ vfsmnt = vfsmnt - OFFSET(mount_mnt);
get_pathname(pwd_dentry, pwd_pathname,
BUFSIZE, 1, vfsmnt);
@@ -2686,7 +2767,12 @@ file_dump(ulong file, ulong dentry, ulong inode, int fd, int flags)
if (flags & DUMP_FULL_NAME) {
if (VALID_MEMBER(file_f_vfsmnt)) {
vfsmnt = get_root_vfsmount(file_buf);
- get_pathname(dentry, pathname, BUFSIZE, 1, vfsmnt);
+ if (VALID_STRUCT(mount))
+ get_pathname(dentry, pathname, BUFSIZE, 1,
+ vfsmnt - OFFSET(mount_mnt));
+ else
+ get_pathname(dentry, pathname, BUFSIZE, 1,
+ vfsmnt);
if (STRNEQ(pathname, "/pts/") &&
STREQ(vfsmount_devname(vfsmnt, buf1, BUFSIZE),
"devpts"))

If the vfsmount-to-mount calculation were always done in get_pathname() itself,
then the patches above would be unnecessary. The same is true for a few other
parts of the patch.

Did you consider doing it that way?

And consider the following patched version of display_dentry_info() below.
The two get_pathname() calls below would generate fatal errors on older
kernels because OFFSET(mount_mnt) will be invalid:

if (!found && symbol_exists("pipe_mnt")) {
get_symbol_data("pipe_mnt", sizeof(long), &vfs);
if (VALID_STRUCT(mount))
readmem(vfs - OFFSET(mount_mnt), KVADDR, mount_buf, SIZE(mount),
"mount buffer", FAULT_ON_ERROR);
else
readmem(vfs, KVADDR, vfsmount_buf, SIZE(vfsmount),
"vfsmount buffer", FAULT_ON_ERROR);
sb = ULONG(vfsmount_buf + OFFSET(vfsmount_mnt_sb));
if (superblock && (sb == superblock)) {
get_pathname(dentry, pathname, BUFSIZE, 1,
vfs - OFFSET(mount_mnt));
found = TRUE;
}
}
if (!found && symbol_exists("sock_mnt")) {
get_symbol_data("sock_mnt", sizeof(long), &vfs);
if (VALID_STRUCT(mount))
readmem(vfs - OFFSET(mount_mnt), KVADDR, mount_buf, SIZE(mount),
"mount buffer", FAULT_ON_ERROR);
else
readmem(vfs, KVADDR, vfsmount_buf, SIZE(vfsmount),
"vfsmount buffer", FAULT_ON_ERROR);
sb = ULONG(vfsmount_buf + OFFSET(vfsmount_mnt_sb));
if (superblock && (sb == superblock)) {
get_pathname(dentry, pathname, BUFSIZE, 1,
vfs - OFFSET(mount_mnt));
found = TRUE;
}
}

If we can continue to have get_pathname() receive a vfsmount pointer,
it reduces the chance of introducing new bugs like the above, and future
maintenance will be easier.

Comments?

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

----- Original Message -----
> At 2012-3-13 11:45, qiaonuohan wrote:
> > At 2012-3-13 3:27, Dave Anderson wrote:
> >>
> >>
> >> ----- Original Message -----
> >>> At 2012-3-10 5:50, Dave Anderson wrote:
> >>>>
> >>>> It's failing in its call to get_pathname(). I haven't had a chance to
> >>>
> >>> You are right. I forgot to check every call to get_pathname. The new
> >>> patch has fixed the problem of swap.
> >>
> >> Although the patch appears to work in the simple cases that I have tested,
> >> I have a few issues with it.
> >>
> >> First, the get_pathname() function prototype is -- and has always been -- this:
> >>
> >> void get_pathname(ulong dentry, char *pathname, int length, int full, ulong vfsmnt)
> >>
> >> where the "vfsmnt" argument has always been a struct vfsmount pointer.
> >>
> >> With your patch, prior to making get_pathname() calls, you typically
> >> make a VALID_STRUCT(mount) call first, and if true, you modify the
> >> vfsmnt argument to be a struct mount pointer.
> >>
> >> It would make more sense maintenance-wise, and would simplify the patch,
> >> if the get_pathname() "vfsmnt" argument would continue to be a struct
> >> vfsmount pointer. And at the top of get_pathname(), you could make the
> >> adjustments for when it is embedded inside a struct mount.

... [ cut ] ...

> >> If we can continue to have get_pathname() receive a vfsmount pointer,
> >> it reduces the chance of introducing new bugs like the above, and future
> >> maintenance will be easier.
> >
> > It does reduce complication of the code. Still, places needing the
> > judgement of struct mount's existing remain. I will modify the patch,
> > and post later.
> >
> > About the function, OFFSET_OPTION(), thanks for your advice.
> >
>
> The attachment is the modified patch.

I have reviewed and tested this patch, and it looks to be both complete
and simpler to maintain. Queued for crash-6.0.5.

Thanks again for undertaking this task,
Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility