2013-10-30 09:45:20

by Madper Xie

[permalink] [raw]
Subject: [PATCH 0/2] make all stored entries accessible.

1. checking type, id, psi, count and timespec when finding duplicate entries.
2. adding count and timestamp for differentiating.

Madper Xie (2):
pstore: avoid incorrectly mark entry as duplicate
pstore: Differentiating names by adding count and timestamp

fs/pstore/inode.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)

--
1.8.4.2


2013-10-30 09:45:21

by Madper Xie

[permalink] [raw]
Subject: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate

From: Madper Xie <[email protected]>

pstore try to find duplicate entries by check both ID, type and psi.
They are not really enough for efi backend. dumped vars always have
the same type, psi and ID. like follows:
dump-type0-9-1-1382511508-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
dump-type0-9-1-1382515661-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0

The "duplicate" entries won't appear in pstorefs. And a complain will be
print -- pstore: failed to load 76 record(s) from 'efi'
So I add two more checks: timespec and count.

Signed-off-by: Madper Xie <[email protected]>
---
fs/pstore/inode.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 1282384..0ae994c 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -47,6 +47,7 @@ static LIST_HEAD(allpstore);
struct pstore_private {
struct list_head list;
struct pstore_info *psi;
+ struct timespec time;
enum pstore_type_id type;
u64 id;
int count;
@@ -290,7 +291,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
list_for_each_entry(pos, &allpstore, list) {
if (pos->type == type &&
pos->id == id &&
- pos->psi == psi) {
+ pos->psi == psi &&
+ pos->count == count &&
+ !timespec_compare(&pos->time, &time)) {
rc = -EEXIST;
break;
}
@@ -312,6 +315,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
private->id = id;
private->count = count;
private->psi = psi;
+ memcpy(&private->time, &time, sizeof(struct timespec));

switch (type) {
case PSTORE_TYPE_DMESG:
--
1.8.4.2

2013-10-30 09:45:27

by Madper Xie

[permalink] [raw]
Subject: [PATCH 2/2] pstore: Differentiating names by adding count and timestamp

From: Madper Xie <[email protected]>

pstore denominates dumped file as type-psname-id. it makes many file have
the same name if there are many entries in backend have the same id.
So adding count and timestamp to file name for differentiating.

Signed-off-by: Madper Xie <[email protected]>
---
fs/pstore/inode.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 0ae994c..36b502f 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -285,7 +285,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
int rc = 0;
char name[PSTORE_NAMELEN];
struct pstore_private *private, *pos;
- unsigned long flags;
+ unsigned long flags, timestamp;

spin_lock_irqsave(&allpstore_lock, flags);
list_for_each_entry(pos, &allpstore, list) {
@@ -316,35 +316,42 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
private->count = count;
private->psi = psi;
memcpy(&private->time, &time, sizeof(struct timespec));
+ timestamp = time.tv_sec;

switch (type) {
case PSTORE_TYPE_DMESG:
- sprintf(name, "dmesg-%s-%lld%s", psname, id,
- compressed ? ".enc.z" : "");
+ sprintf(name, "dmesg-%s-%lld-%d-%ld%s", psname, id, count,
+ timestamp, compressed ? ".enc.z" : "");
break;
case PSTORE_TYPE_CONSOLE:
- sprintf(name, "console-%s", psname);
+ sprintf(name, "console-%s-%d-%ld", psname, count, timestamp);
break;
case PSTORE_TYPE_FTRACE:
- sprintf(name, "ftrace-%s", psname);
+ sprintf(name, "ftrace-%s-%d-%ld", psname, count, timestamp);
break;
case PSTORE_TYPE_MCE:
- sprintf(name, "mce-%s-%lld", psname, id);
+ sprintf(name, "mce-%s-%lld-%d-%ld", psname, id, count,
+ timestamp);
break;
case PSTORE_TYPE_PPC_RTAS:
- sprintf(name, "rtas-%s-%lld", psname, id);
+ sprintf(name, "rtas-%s-%lld-%d-%ld", psname, id, count,
+ timestamp);
break;
case PSTORE_TYPE_PPC_OF:
- sprintf(name, "powerpc-ofw-%s-%lld", psname, id);
+ sprintf(name, "powerpc-ofw-%s-%lld-%d-%ld", psname, id, count,
+ timestamp);
break;
case PSTORE_TYPE_PPC_COMMON:
- sprintf(name, "powerpc-common-%s-%lld", psname, id);
+ sprintf(name, "powerpc-common-%s-%lld-%d-%ld", psname, id,
+ count, timestamp);
break;
case PSTORE_TYPE_UNKNOWN:
- sprintf(name, "unknown-%s-%lld", psname, id);
+ sprintf(name, "unknown-%s-%lld-%d-%ld", psname, id, count,
+ timestamp);
break;
default:
- sprintf(name, "type%d-%s-%lld", type, psname, id);
+ sprintf(name, "type%d-%s-%lld-%d-%ld", type, psname, id, count,
+ timestamp);
break;
}

--
1.8.4.2

2013-10-30 14:35:59

by Seiji Aguchi

[permalink] [raw]
Subject: RE: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate



> -----Original Message-----
> From: Madper Xie [mailto:[email protected]]
> Sent: Wednesday, October 30, 2013 5:45 AM
> To: [email protected]; [email protected]; [email protected]; [email protected]; Seiji Aguchi
> Cc: [email protected]; [email protected]; [email protected]; Madper Xie
> Subject: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate
>
> From: Madper Xie <[email protected]>
>
> pstore try to find duplicate entries by check both ID, type and psi.
> They are not really enough for efi backend. dumped vars always have
> the same type, psi and ID. like follows:
> dump-type0-9-1-1382511508-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
> dump-type0-9-1-1382515661-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>
> The "duplicate" entries won't appear in pstorefs. And a complain will be
> print -- pstore: failed to load 76 record(s) from 'efi'
> So I add two more checks: timespec and count.
>
> Signed-off-by: Madper Xie <[email protected]>

Looks good to me.
Acked-by: Seiji Aguchi <[email protected]>

But, this patch has to be tested by other backend drivers like erst and ramoops.
In my understanding, erst has supported to log multiple events.
So, I'm interested why the same error hasn't happened in the other drivers.

Seiji

> ---
> fs/pstore/inode.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 1282384..0ae994c 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -47,6 +47,7 @@ static LIST_HEAD(allpstore);
> struct pstore_private {
> struct list_head list;
> struct pstore_info *psi;
> + struct timespec time;
> enum pstore_type_id type;
> u64 id;
> int count;
> @@ -290,7 +291,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
> list_for_each_entry(pos, &allpstore, list) {
> if (pos->type == type &&
> pos->id == id &&
> - pos->psi == psi) {
> + pos->psi == psi &&
> + pos->count == count &&
> + !timespec_compare(&pos->time, &time)) {
> rc = -EEXIST;
> break;
> }
> @@ -312,6 +315,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
> private->id = id;
> private->count = count;
> private->psi = psi;
> + memcpy(&private->time, &time, sizeof(struct timespec));
>
> switch (type) {
> case PSTORE_TYPE_DMESG:
> --
> 1.8.4.2

2013-10-30 14:38:37

by Seiji Aguchi

[permalink] [raw]
Subject: RE: [PATCH 2/2] pstore: Differentiating names by adding count and timestamp



> -----Original Message-----
> From: Madper Xie [mailto:[email protected]]
> Sent: Wednesday, October 30, 2013 5:45 AM
> To: [email protected]; [email protected]; [email protected]; [email protected]; Seiji Aguchi
> Cc: [email protected]; [email protected]; [email protected]; Madper Xie
> Subject: [PATCH 2/2] pstore: Differentiating names by adding count and timestamp
>
> From: Madper Xie <[email protected]>
>
> pstore denominates dumped file as type-psname-id. it makes many file have
> the same name if there are many entries in backend have the same id.
> So adding count and timestamp to file name for differentiating.
>
> Signed-off-by: Madper Xie <[email protected]>

It should be tested by other drivers as well..
But, looks good to me.
Acked-by: Seiji Aguchi <[email protected]>

> ---
> fs/pstore/inode.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 0ae994c..36b502f 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -285,7 +285,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
> int rc = 0;
> char name[PSTORE_NAMELEN];
> struct pstore_private *private, *pos;
> - unsigned long flags;
> + unsigned long flags, timestamp;
>
> spin_lock_irqsave(&allpstore_lock, flags);
> list_for_each_entry(pos, &allpstore, list) {
> @@ -316,35 +316,42 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
> private->count = count;
> private->psi = psi;
> memcpy(&private->time, &time, sizeof(struct timespec));
> + timestamp = time.tv_sec;
>
> switch (type) {
> case PSTORE_TYPE_DMESG:
> - sprintf(name, "dmesg-%s-%lld%s", psname, id,
> - compressed ? ".enc.z" : "");
> + sprintf(name, "dmesg-%s-%lld-%d-%ld%s", psname, id, count,
> + timestamp, compressed ? ".enc.z" : "");
> break;
> case PSTORE_TYPE_CONSOLE:
> - sprintf(name, "console-%s", psname);
> + sprintf(name, "console-%s-%d-%ld", psname, count, timestamp);
> break;
> case PSTORE_TYPE_FTRACE:
> - sprintf(name, "ftrace-%s", psname);
> + sprintf(name, "ftrace-%s-%d-%ld", psname, count, timestamp);
> break;
> case PSTORE_TYPE_MCE:
> - sprintf(name, "mce-%s-%lld", psname, id);
> + sprintf(name, "mce-%s-%lld-%d-%ld", psname, id, count,
> + timestamp);
> break;
> case PSTORE_TYPE_PPC_RTAS:
> - sprintf(name, "rtas-%s-%lld", psname, id);
> + sprintf(name, "rtas-%s-%lld-%d-%ld", psname, id, count,
> + timestamp);
> break;
> case PSTORE_TYPE_PPC_OF:
> - sprintf(name, "powerpc-ofw-%s-%lld", psname, id);
> + sprintf(name, "powerpc-ofw-%s-%lld-%d-%ld", psname, id, count,
> + timestamp);
> break;
> case PSTORE_TYPE_PPC_COMMON:
> - sprintf(name, "powerpc-common-%s-%lld", psname, id);
> + sprintf(name, "powerpc-common-%s-%lld-%d-%ld", psname, id,
> + count, timestamp);
> break;
> case PSTORE_TYPE_UNKNOWN:
> - sprintf(name, "unknown-%s-%lld", psname, id);
> + sprintf(name, "unknown-%s-%lld-%d-%ld", psname, id, count,
> + timestamp);
> break;
> default:
> - sprintf(name, "type%d-%s-%lld", type, psname, id);
> + sprintf(name, "type%d-%s-%lld-%d-%ld", type, psname, id, count,
> + timestamp);
> break;
> }
>
> --
> 1.8.4.2

2013-10-30 14:51:25

by Madper Xie

[permalink] [raw]
Subject: Re: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate


[email protected] writes:

>> -----Original Message-----
>> From: Madper Xie [mailto:[email protected]]
>> Sent: Wednesday, October 30, 2013 5:45 AM
>> To: [email protected]; [email protected]; [email protected]; [email protected]; Seiji Aguchi
>> Cc: [email protected]; [email protected]; [email protected]; Madper Xie
>> Subject: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate
>>
>> From: Madper Xie <[email protected]>
>>
>> pstore try to find duplicate entries by check both ID, type and psi.
>> They are not really enough for efi backend. dumped vars always have
>> the same type, psi and ID. like follows:
>> dump-type0-9-1-1382511508-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>> dump-type0-9-1-1382515661-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>>
>> The "duplicate" entries won't appear in pstorefs. And a complain will be
>> print -- pstore: failed to load 76 record(s) from 'efi'
>> So I add two more checks: timespec and count.
>>
>> Signed-off-by: Madper Xie <[email protected]>
>
> Looks good to me.
> Acked-by: Seiji Aguchi <[email protected]>
>
> But, this patch has to be tested by other backend drivers like erst and ramoops.
> In my understanding, erst has supported to log multiple events.
> So, I'm interested why the same error hasn't happened in the other drivers.
>
> Seiji
Thanks for your review. I'll try to test other backends. :-)
>
>> ---
>> fs/pstore/inode.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>> index 1282384..0ae994c 100644
>> --- a/fs/pstore/inode.c
>> +++ b/fs/pstore/inode.c
>> @@ -47,6 +47,7 @@ static LIST_HEAD(allpstore);
>> struct pstore_private {
>> struct list_head list;
>> struct pstore_info *psi;
>> + struct timespec time;
>> enum pstore_type_id type;
>> u64 id;
>> int count;
>> @@ -290,7 +291,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>> list_for_each_entry(pos, &allpstore, list) {
>> if (pos->type == type &&
>> pos->id == id &&
>> - pos->psi == psi) {
>> + pos->psi == psi &&
>> + pos->count == count &&
>> + !timespec_compare(&pos->time, &time)) {
>> rc = -EEXIST;
>> break;
>> }
>> @@ -312,6 +315,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>> private->id = id;
>> private->count = count;
>> private->psi = psi;
>> + memcpy(&private->time, &time, sizeof(struct timespec));
>>
>> switch (type) {
>> case PSTORE_TYPE_DMESG:
>> --
>> 1.8.4.2


--
Best,
Madper Xie.

2013-10-30 21:11:55

by Luck, Tony

[permalink] [raw]
Subject: RE: [PATCH 0/2] make all stored entries accessible.

> 1. checking type, id, psi, count and timespec when finding duplicate entries.
> 2. adding count and timestamp for differentiating.

Ah - I was expecting that the backend driver would have a unique "id" for
each record stored ... but is seems that this isn't true for efivars.

I just tried this patch series out with the erst backend. It seems to work, but
I was confused for a while by the filenames that I see in the pstore filesystem. There is a "--"
in the name that I couldn't quite figure out:

-r--r--r-- 1 root root 17498 Oct 30 13:41 dmesg-erst-5940651313304961025--2129078373-1383165669
-r--r--r-- 1 root root 17511 Oct 30 13:41 dmesg-erst-5940651313304961026--2129078373-1383165669
-r--r--r-- 1 root root 17530 Oct 30 13:41 dmesg-erst-5940651313304961027--2129078373-1383165669
-r--r--r-- 1 root root 17492 Oct 30 13:41 dmesg-erst-5940651313304961028--2129078373-1383165669
-r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669
-r--r--r-- 1 root root 17512 Oct 30 13:41 dmesg-erst-5940651313304961030--2129078373-1383165669
-r--r--r-- 1 root root 17531 Oct 30 13:41 dmesg-erst-5940651313304961031--2129078373-1383165669
-r--r--r-- 1 root root 17488 Oct 30 13:44 dmesg-erst-5940652283967569921--2129078373-1383165895
-r--r--r-- 1 root root 17512 Oct 30 13:44 dmesg-erst-5940652283967569922--2129078373-1383165895
-r--r--r-- 1 root root 17512 Oct 30 13:44 dmesg-erst-5940652283967569923--2129078373-1383165895
-r--r--r-- 1 root root 17500 Oct 30 13:44 dmesg-erst-5940652283967569924--2129078373-1383165895
-r--r--r-- 1 root root 17536 Oct 30 13:44 dmesg-erst-5940652283967569925--2129078373-1383165895
-r--r--r-- 1 root root 17513 Oct 30 13:44 dmesg-erst-5940652283967569926--2129078373-1383165895
-r--r--r-- 1 root root 17513 Oct 30 13:44 dmesg-erst-5940652283967569927--2129078373-1383165895
-r--r--r-- 1 root root 17501 Oct 30 13:44 dmesg-erst-5940652283967569928--2129078373-1383165895

The filename came from:
+ sprintf(name, "dmesg-%s-%lld-%d-%ld%s", psname, id, count,
+ timestamp, compressed ? ".enc.z" : "");

So I guess that "count" is -2129078373 - which is some uninitialized junk from the stack frame
of pstore_get_records() for the "int count" variable ... the erst reader function doesn't touch it.

I'll add an initializer "int count = 0;" there when applying the patches.

-Tony

2013-10-30 23:27:49

by Seiji Aguchi

[permalink] [raw]
Subject: RE: [PATCH 0/2] make all stored entries accessible.

> Ah - I was expecting that the backend driver would have a unique "id" for
> each record stored ... but is seems that this isn't true for efivars.
>

So, do you mean efivars should fix to use the "id" in a proper way?

I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
it is reasonable for users to make multiple numbers visible to the file name.

> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669

Seiji

2013-10-31 00:25:00

by Luck, Tony

[permalink] [raw]
Subject: RE: [PATCH 0/2] make all stored entries accessible.

> So, do you mean efivars should fix to use the "id" in a proper way?

It would avoid the need for all these tests, and additions to the filename to guarantee
uniqueness.

Not sure what options efivars has to create a unique, persistent "id" for each
record. It's a fundamental part of how ERST works (though the unique ID is just
based around a timestamp).

> I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
> it is reasonable for users to make multiple numbers visible to the file name.
>
>> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669

after I added the "count = 0" initialization the filename gets a tiny bit less
scary:

-r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029-0-1383165669

-Tony

2013-10-31 03:00:44

by Madper Xie

[permalink] [raw]
Subject: Re: [PATCH 0/2] make all stored entries accessible.


[email protected] writes:

>> So, do you mean efivars should fix to use the "id" in a proper way?
>
> It would avoid the need for all these tests, and additions to the filename to guarantee
> uniqueness.
>
> Not sure what options efivars has to create a unique, persistent "id" for each
> record. It's a fundamental part of how ERST works (though the unique ID is just
> based around a timestamp).
>
Okay, maybe there are three options here:
1. combine timestamp, count and part into "id".
for now, in efi-pstore.c, *id = part. and we could simply change it
to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.
2. change the id's type. let id become a string.
so every backend could write anything to id. then it will become a
part of filename in pstore filesystem. (but we need fix all backends
since we modified api.)
3. apply the patches I have sent... even if the filename will be ugly
and gory...
Which one do you prefer?
>> I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
>> it is reasonable for users to make multiple numbers visible to the file name.
>>
>>> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669
>
> after I added the "count = 0" initialization the filename gets a tiny bit less
> scary:
>
> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029-0-1383165669
>
> -Tony


--
Best,
Madper Xie.

2013-10-31 14:36:02

by Seiji Aguchi

[permalink] [raw]
Subject: RE: [PATCH 0/2] make all stored entries accessible.



> 1. combine timestamp, count and part into "id".
> for now, in efi-pstore.c, *id = part. and we could simply change it
> to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.

My opinion close to 1.
But, the "*id" should not be the complex one like (timestamp * 100 + part) * 100 + count.
Rather, it should be a simple sequential number beginning with 1.

- Remove "id" member from pstore_read_info struct.
- Introduce a global sequential counter like "static u64 efi_pstore_read_count" (or add the member to pstore_info structure?)
- Initialize to "1" in efi_pstore_open().
- Increment it in efi_pstore_read().

If we can do it, we don't need to touch a code of pstore filesystem and can avoid regressions of
other backend drivers.

Seiji

> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Madper Xie
> Sent: Wednesday, October 30, 2013 11:01 PM
> To: Luck, Tony
> Cc: Seiji Aguchi; Madper Xie; [email protected]; [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 0/2] make all stored entries accessible.
>
>
> [email protected] writes:
>
> >> So, do you mean efivars should fix to use the "id" in a proper way?
> >
> > It would avoid the need for all these tests, and additions to the filename to guarantee
> > uniqueness.
> >
> > Not sure what options efivars has to create a unique, persistent "id" for each
> > record. It's a fundamental part of how ERST works (though the unique ID is just
> > based around a timestamp).
> >
> Okay, maybe there are three options here:
> 1. combine timestamp, count and part into "id".
> for now, in efi-pstore.c, *id = part. and we could simply change it
> to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.
> 2. change the id's type. let id become a string.
> so every backend could write anything to id. then it will become a
> part of filename in pstore filesystem. (but we need fix all backends
> since we modified api.)
> 3. apply the patches I have sent... even if the filename will be ugly
> and gory...
> Which one do you prefer?
> >> I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
> >> it is reasonable for users to make multiple numbers visible to the file name.
> >>
> >>> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669
> >
> > after I added the "count = 0" initialization the filename gets a tiny bit less
> > scary:
> >
> > -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029-0-1383165669
> >
> > -Tony
>
>
> --
> Best,
> Madper Xie.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-10-31 20:22:41

by Luck, Tony

[permalink] [raw]
Subject: RE: [PATCH 0/2] make all stored entries accessible.

>> 1. combine timestamp, count and part into "id".
>> for now, in efi-pstore.c, *id = part. and we could simply change it
>> to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.
>
> My opinion close to 1.
> But, the "*id" should not be the complex one like (timestamp * 100 + part) * 100 + count.
> Rather, it should be a simple sequential number beginning with 1.

I also like option 1 ... but I think the "id" should be a persistent value for
a given saved record. So some func(timestamp, part, count) would be a
good idea. If we try using "sequential" numbers - and don't manage to
clear out /sys/fs/pstore each time - then we may have the same "dmesg"
file show up with different names on each boot.

Right now I have a simple script to save & clear ... not much more
complex than:

cd /sys/fs/pstore
cp * /var/log/save-pstore
rm *

This depends on not re-using filenames (otherwise new files in pstore
might overwrite older saved files in my /var/log/save-pstore area).

-Tony

2013-10-31 23:23:23

by Seiji Aguchi

[permalink] [raw]
Subject: RE: [PATCH 0/2] make all stored entries accessible.

> I also like option 1 ... but I think the "id" should be a persistent value for
> a given saved record. So some func(timestamp, part, count) would be a
> good idea. If we try using "sequential" numbers - and don't manage to
> clear out /sys/fs/pstore each time - then we may have the same "dmesg"
> file show up with different names on each boot.
>
> Right now I have a simple script to save & clear ... not much more
> complex than:
>
> cd /sys/fs/pstore
> cp * /var/log/save-pstore
> rm *
>
> This depends on not re-using filenames (otherwise new files in pstore
> might overwrite older saved files in my /var/log/save-pstore area).

I see.. It is a persuasive use case.

(1) I agree that some func(timestamp, part, count) would be good idea.
This might work, although an overflow will happen some time...
sprintf(id_str, "%lld%d%d", timestamp, part, count)
simple_str_to_ull(id_str, &id, base)

(2) There is a GetNextHighMonotonicCount() runtime service in EFI specification
to get a persistent number across the reboot, but I'm not sure if it is safe to use it..
Also, it would be good if we can create the id by ourselves, rather than using firmware.

(3) Also, (it might not be good idea), if a pstore filesystem expects all backend drivers
to use the persistent id, the pstore should provide it by itself.
(by using time stamp counter or something like that.)

As I looked into the ramoops's code. It seems to use a non-persistent sequential counter,by initializing
read_cnt to "0" in ramoops_pstore_open(), and incrementing it in ramoops_pstore_read().
It doesn't seem to be the pstore's expectation.
And when someone introduces a new driver, they may misunderstand how to create the id as well..

As above, there are mutiple ideas, but (1) is reasonable to me.

Seiji