Re: [PATCH 2/2] Save current timestamp part of dmesg while writing oops message to pstore

[Ankit Kumar <ankit@linux.vnet.ibm.com>]

Hi Kees,



On Tuesday 23 May 2017 05:21 AM, Kees Cook wrote:
> On Mon, May 22, 2017 at 3:20 AM, Ankit Kumar <ankit@linux.vnet.ibm.com> wrote:
>> Currently on panic or Oops, kernel saves the last few bytes from dmesg
>> buffer to nvram. Usually kdump does capture kernel memory and provide
>> dmesg logs as well. But in some cases where kdump fails to capture
>> vmcore, the dmesg buffer stored in nvram/pstore turns out to be very
>> helpful in analyzing root cause.
>>
>> Present code creates pstore dump file(/sys/fs/pstore/dmesg-***) based on
>> timestamp(retrieved from header). Current pstore code creates dump file
>> (/sys/fs/pstore/dmesg-***) with that timestamp. Dump file can be analyzed
>> based on file creation time and we can make out whether dump file has latest
>> data or not.
>>
>> But when we transfer pstore dump file(/sys/fs/pstore/dmesg-***) to other
>> machine or collect file using some utilities(sosreport/supportconfig) then file
>> timestamp gets changed and hence by looking at device file (dmesg-***) we won't
>> be able to identify whether dump has latest data or not.
>>
>> Above issue can be fixed if we also have timestamp(dump creation time) as
>> initial few bytes while capturing dmesg buffer to pstore dump file
>> (/sys/fs/pstore/dmesg-***).
>>
>>
>> This patch enhances pstore write code to also write timestamp as part of data.
>>
>> Here is sample log of dump file:(/sys/fs/pstore/dmesg-***)
>> Oops#1 Part1 [timestamp:1494939359.590463]
> While I understand your rationale about possibly losing file timestamp
> information in userspace, I think this is a solvable problem on the
> collection side. If an additional header is needed, perhaps copy the
> dmesg files like this:
>
> for i in dmesg-*; do
>      (stat --format=%y /sys/fs/pstore/$i; \
>       cat /sys/fs/pstore/$i) > $collect_dir/$i
> done

Yes. We can handle this in userspace. But we wanted to see if we can add 
this as part of pstore
log itself.


> One of the primary concerns for pstore is the stored dump size,

I understand. How about adding timestamp to file name itself? Something 
like below

index 792a4e5..0837365 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -349,9 +349,10 @@ int pstore_mkfile(struct dentry *root, struct 
pstore_record *record)

         switch (record->type) {
         case PSTORE_TYPE_DMESG:
-               scnprintf(name, sizeof(name), "dmesg-%s-%lld%s",
+               scnprintf(name, sizeof(name), "dmesg-%s-%lld%s-%lu.%lu",
                           record->psi->name, record->id,
-                         record->compressed ? ".enc.z" : "");
+                         record->compressed ? ".enc.z" : "",
+                         record->time.tv_sec, record->time.tv_nsec / 1000);
                 break;
         case PSTORE_TYPE_CONSOLE:


~Ankit