[PATCH] Make efi-pstore return a unique id

[PATCH] Make efi-pstore return a unique id

[Madper Xie]

Pstore fs expects that backends provide a uniqued id which could avoid
pstore making entries as duplication or denominating entries the same
name. So I combine the timestamp, part and count into id.

Signed-off-by: Madper Xie <cxie-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/efi-pstore.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 5002d50..0de9179 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -39,6 +39,17 @@ struct pstore_read_data {
 	char **buf;
 };
 
+static u64 efi_generate_id(unsigned long timestamp, unsigned int part, int count)
+{
+	char id_str[64];
+	u64 id = 0;
+
+	sprintf(id_str, "%lu%u%d", timestamp, part, count);
+	if (kstrtoull(id_str, 10, &id))
+		pr_warn("efi-pstore: failed to generate id\n");
+	return id;
+}
+
 static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 {
 	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
@@ -57,17 +68,18 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 
 	if (sscanf(name, "dump-type%u-%u-%d-%lu-%c",
 		   cb_data->type, &part, &cnt, &time, &data_type) == 5) {
-		*cb_data->id = part;
+		*cb_data->id = efi_generate_id(time, part, cnt);
 		*cb_data->count = cnt;
 		cb_data->timespec->tv_sec = time;
 		cb_data->timespec->tv_nsec = 0;
+
 		if (data_type == 'C')
 			*cb_data->compressed = true;
 		else
 			*cb_data->compressed = false;
 	} else if (sscanf(name, "dump-type%u-%u-%d-%lu",
 		   cb_data->type, &part, &cnt, &time) == 4) {
-		*cb_data->id = part;
+		*cb_data->id = efi_generate_id(time, part, cnt);
 		*cb_data->count = cnt;
 		cb_data->timespec->tv_sec = time;
 		cb_data->timespec->tv_nsec = 0;
@@ -79,7 +91,7 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 		 * which doesn't support holding
 		 * multiple logs, remains.
 		 */
-		*cb_data->id = part;
+		*cb_data->id = efi_generate_id(time, part, 0);
 		*cb_data->count = 0;
 		cb_data->timespec->tv_sec = time;
 		cb_data->timespec->tv_nsec = 0;
@@ -125,9 +137,11 @@ static int efi_pstore_write(enum pstore_type_id type,
 	efi_char16_t efi_name[DUMP_NAME_LEN];
 	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	int i, ret = 0;
+	unsigned long timestamp;
 
+	timestamp = get_seconds();
 	sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
-		get_seconds(), compressed ? 'C' : 'D');
+		timestamp, compressed ? 'C' : 'D');
 
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = name[i];
-- 
1.8.4.2

Re: [PATCH] Make efi-pstore return a unique id

[Richard Weinberger]

On Fri, Nov 1, 2013 at 5:14 PM, Madper Xie <cxie-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Pstore fs expects that backends provide a uniqued id which could avoid
> pstore making entries as duplication or denominating entries the same
> name. So I combine the timestamp, part and count into id.
>
> Signed-off-by: Madper Xie <cxie-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/firmware/efi/efi-pstore.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index 5002d50..0de9179 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -39,6 +39,17 @@ struct pstore_read_data {
>         char **buf;
>  };
>
> +static u64 efi_generate_id(unsigned long timestamp, unsigned int part, int count)
> +{
> +       char id_str[64];
> +       u64 id = 0;
> +
> +       sprintf(id_str, "%lu%u%d", timestamp, part, count);
> +       if (kstrtoull(id_str, 10, &id))
> +               pr_warn("efi-pstore: failed to generate id\n");
> +       return id;
> +}

This is just odd. You make a string from three ints and then a parse
it to a int again.

>  static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
>  {
>         efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> @@ -57,17 +68,18 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
>
>         if (sscanf(name, "dump-type%u-%u-%d-%lu-%c",
>                    cb_data->type, &part, &cnt, &time, &data_type) == 5) {
> -               *cb_data->id = part;
> +               *cb_data->id = efi_generate_id(time, part, cnt);
>                 *cb_data->count = cnt;
>                 cb_data->timespec->tv_sec = time;
>                 cb_data->timespec->tv_nsec = 0;
> +
>                 if (data_type == 'C')
>                         *cb_data->compressed = true;
>                 else
>                         *cb_data->compressed = false;
>         } else if (sscanf(name, "dump-type%u-%u-%d-%lu",
>                    cb_data->type, &part, &cnt, &time) == 4) {
> -               *cb_data->id = part;
> +               *cb_data->id = efi_generate_id(time, part, cnt);
>                 *cb_data->count = cnt;
>                 cb_data->timespec->tv_sec = time;
>                 cb_data->timespec->tv_nsec = 0;
> @@ -79,7 +91,7 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
>                  * which doesn't support holding
>                  * multiple logs, remains.
>                  */
> -               *cb_data->id = part;
> +               *cb_data->id = efi_generate_id(time, part, 0);
>                 *cb_data->count = 0;
>                 cb_data->timespec->tv_sec = time;
>                 cb_data->timespec->tv_nsec = 0;
> @@ -125,9 +137,11 @@ static int efi_pstore_write(enum pstore_type_id type,
>         efi_char16_t efi_name[DUMP_NAME_LEN];
>         efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
>         int i, ret = 0;
> +       unsigned long timestamp;
>
> +       timestamp = get_seconds();
>         sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
> -               get_seconds(), compressed ? 'C' : 'D');
> +               timestamp, compressed ? 'C' : 'D');
>
>         for (i = 0; i < DUMP_NAME_LEN; i++)
>                 efi_name[i] = name[i];
> --
> 1.8.4.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Thanks,
//richard

Re: [PATCH] Make efi-pstore return a unique id

[Tony Luck]

>> +static u64 efi_generate_id(unsigned long timestamp, unsigned int part, int count)

I don't think the "efi_" prefix is needed here. For one thing the
function is static, so
no name space pollution worries. For another - it makes it look like
this is some thing
defined in EFI standard.  If "generate_id()" is too generic for your
tastes ... then a
"pstore_" prefix might be more appropriate.

>> +{
>> +       char id_str[64];
>> +       u64 id = 0;
>> +
>> +       sprintf(id_str, "%lu%u%d", timestamp, part, count);
>> +       if (kstrtoull(id_str, 10, &id))
>> +               pr_warn("efi-pstore: failed to generate id\n");
>> +       return id;
>> +}
>
> This is just odd. You make a string from three ints and then a parse
> it to a int again.

Agreed.  I liked your ((timestamp * 100 + part) * 100 + count function much
more than this.

-Tony

RE: [PATCH] Make efi-pstore return a unique id

[Seiji Aguchi]

> >> +{
> >> +       char id_str[64];
> >> +       u64 id = 0;
> >> +
> >> +       sprintf(id_str, "%lu%u%d", timestamp, part, count);
> >> +       if (kstrtoull(id_str, 10, &id))
> >> +               pr_warn("efi-pstore: failed to generate id\n");
> >> +       return id;
> >> +}
> >
> > This is just odd. You make a string from three ints and then a parse
> > it to a int again.
> 
> Agreed.  I liked your ((timestamp * 100 + part) * 100 + count function much
> more than this.

I was worried that the part and count could be more than 100.
If it happens, the id may not be unique...

But, currently, size of nvram storage is limited, so it is a corner case.
I respect your opinion.

> @@ -125,9 +137,11 @@ static int efi_pstore_write(enum pstore_type_id type,
>  	efi_char16_t efi_name[DUMP_NAME_LEN];
>  	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
>  	int i, ret = 0;
> +	unsigned long timestamp;
> 
> +	timestamp = get_seconds();
>  	sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
> -		get_seconds(), compressed ? 'C' : 'D');
> +		timestamp, compressed ? 'C' : 'D');
> 

I don't think you need to change efi_pstore_write().
It is just add a local timestamp variable.

Seiji

>  	for (i = 0; i < DUMP_NAME_LEN; i++)
>  		efi_name[i] = name[i];
> --
> 1.8.4.2

Re: [PATCH] Make efi-pstore return a unique id

[Richard Weinberger]

Am 01.11.2013 20:22, schrieb Seiji Aguchi:
>>>> +{
>>>> +       char id_str[64];
>>>> +       u64 id = 0;
>>>> +
>>>> +       sprintf(id_str, "%lu%u%d", timestamp, part, count);
>>>> +       if (kstrtoull(id_str, 10, &id))
>>>> +               pr_warn("efi-pstore: failed to generate id\n");
>>>> +       return id;
>>>> +}
>>>
>>> This is just odd. You make a string from three ints and then a parse
>>> it to a int again.
>>
>> Agreed.  I liked your ((timestamp * 100 + part) * 100 + count function much
>> more than this.
> 
> I was worried that the part and count could be more than 100.
> If it happens, the id may not be unique...
> 
> But, currently, size of nvram storage is limited, so it is a corner case.
> I respect your opinion.

What about feeding the bytes of all three integers into a non-cryptographic hash function?
Using this way you get a cheap unique id.

Thanks,
//richard

Re: [PATCH] Make efi-pstore return a unique id

[Madper Xie]

richard@nod.at writes:

> Am 01.11.2013 20:22, schrieb Seiji Aguchi:
>>>>> +{
>>>>> +       char id_str[64];
>>>>> +       u64 id = 0;
>>>>> +
>>>>> +       sprintf(id_str, "%lu%u%d", timestamp, part, count);
>>>>> +       if (kstrtoull(id_str, 10, &id))
>>>>> +               pr_warn("efi-pstore: failed to generate id\n");
>>>>> +       return id;
>>>>> +}
>>>>
>>>> This is just odd. You make a string from three ints and then a parse
>>>> it to a int again.
>>>
>>> Agreed.  I liked your ((timestamp * 100 + part) * 100 + count function much
>>> more than this.
>> 
>> I was worried that the part and count could be more than 100.
>> If it happens, the id may not be unique...
>> 
>> But, currently, size of nvram storage is limited, so it is a corner case.
>> I respect your opinion.
>
Is it really safe? for now I have more than 100 entries:
[root@dhcp-13-41 rhel6]# ls -l /dev/pstore/ | wc -l
124
The maximum part of my records is 16. But I not sure if overflow will
happen in some special case, like a very long dmesg output. or a server
never reboot, and too many warnings make count++...
So is it necessary to check count < 100 or 100 =< count < 1000 ?
> What about feeding the bytes of all three integers into a non-cryptographic hash function?
> Using this way you get a cheap unique id.
>
> Thanks,
> //richard
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


-- 
Best,
Madper Xie.

RE: [PATCH] Make efi-pstore return a unique id

[Seiji Aguchi]

> What about feeding the bytes of all three integers into a non-cryptographic hash function?
> Using this way you get a cheap unique id.

It is reasonable to me.

Seiji

Re: [PATCH] Make efi-pstore return a unique id

[Tony Luck]

On Fri, Nov 1, 2013 at 1:57 PM, Seiji Aguchi <seiji.aguchi-7rDLJAbr9SE@public.gmane.org> wrote:
>> What about feeding the bytes of all three integers into a non-cryptographic hash function?
>> Using this way you get a cheap unique id.
>
> It is reasonable to me.

How does efivars backend handle "unlink(2)" in the pstore file system.
pstore will call the backend->erase function passing the "id".  The
backend should then erase the right record from persistent storage.

With the  ((timestamp * 100 + part) * 100 + count function - you can
easily reverse it to find timestamp, part and count - would that make life
easier for the backend to find the record to be erased?  If you use a
hash function you will need to check each record and compute the
hash to see if it matches (probably not a big deal because the backend
will generally only hold a handful of records).

-Tony

RE: [PATCH] Make efi-pstore return a unique id

[Seiji Aguchi]

> How does efivars backend handle "unlink(2)" in the pstore file system.
> pstore will call the backend->erase function passing the "id".  The
> backend should then erase the right record from persistent storage.
> 
> With the  ((timestamp * 100 + part) * 100 + count function - you can
> easily reverse it to find timestamp, part and count - would that make life
> easier for the backend to find the record to be erased?  If you use a
> hash function you will need to check each record and compute the
> hash to see if it matches (probably not a big deal because the backend
> will generally only hold a handful of records).

By generating the id in  efi_pstore_write(), and using it to a variable name,
It works at an erasing time as well.

The root cause of this problem is that efivars used "part" as id.
It was a wrong way. So, we should not keep it.

Seiji

Re: [PATCH] Make efi-pstore return a unique id

[Madper Xie]

richard-/L3Ra7n9ekc@public.gmane.org writes:

> Am 01.11.2013 20:22, schrieb Seiji Aguchi:
>>>
>>> Agreed.  I liked your ((timestamp * 100 + part) * 100 + count function much
>>> more than this.
>> 
>> I was worried that the part and count could be more than 100.
>> If it happens, the id may not be unique...
>> 
>> But, currently, size of nvram storage is limited, so it is a corner case.
>> I respect your opinion.
>
> What about feeding the bytes of all three integers into a
> non-cryptographic hash function?

Then will lost the sequence of our log. We will get lots of entries like
"dmesg-efi-`unique but meaningless number here`" in pstore fs. Who will
know which file is the latest record?
A possible way is sort them by created time. But pstore splits large
messages into many parts. So they will have the same created-time. Like
the following case:
[root@dhcp-13-41 ~]# ls -rtl /dev/pstore/
total 0
-r--r--r--. 1 root root  930 Nov 15 08:42 dmesg-efi-9
-r--r--r--. 1 root root 1017 Nov 15 08:42 dmesg-efi-8
-r--r--r--. 1 root root  993 Nov 15 08:42 dmesg-efi-7
-r--r--r--. 1 root root  984 Nov 15 08:42 dmesg-efi-6
-r--r--r--. 1 root root 1008 Nov 15 08:42 dmesg-efi-5
-r--r--r--. 1 root root  909 Nov 15 08:42 dmesg-efi-10
-r--r--r--. 1 root root 1003 Nov 15 08:42 dmesg-efi-11
-r--r--r--. 1 root root  980 Nov 18 00:41 dmesg-efi-4
-r--r--r--. 1 root root  990 Nov 18 00:41 dmesg-efi-3
-r--r--r--. 1 root root  966 Nov 18 00:41 dmesg-efi-2
-r--r--r--. 1 root root 1010 Nov 18 00:41 dmesg-efi-1

or more intuitive:
ls /sys/firmware/efi/efivars/ | grep -i "dump" | cut -d'-' -f5 | sort |wc -l
103
ls /sys/firmware/efi/efivars/ | grep -i "dump" | cut -d'-' -f5 | sort |uniq |wc -l
26

So if we using a hashed unique number for uniqueness, we will lose the
sequence. We must sort them manually.

And another side, the combin of timestamp, count and part is unique. Why
we generate a unique number from a unique number?
if you think "making a string from three ints and then a parse it to a
int again" is odd, i'd like to use ((timestamp * 100 + part) * 100 +
count.

> Using this way you get a cheap unique id.
>
> Thanks,
> //richard

RE: [PATCH] Make efi-pstore return a unique id

[Seiji Aguchi]

> Then will lost the sequence of our log. We will get lots of entries like
> "dmesg-efi-`unique but meaningless number here`" in pstore fs. Who will
> know which file is the latest record?

Ah, that's good point.

> And another side, the combin of timestamp, count and part is unique. Why
> we generate a unique number from a unique number?
> if you think "making a string from three ints and then a parse it to a
> int again" is odd, i'd like to use ((timestamp * 100 + part) * 100 +
> count.

How about calculating the number of digit of part /count, instead of using fixed "100"?
We can ensure the uniqueness of each id by doing it.

digit_num = log2(part)/log2(10) + 1
timestamp * 10^(digit_num + 1)

Seiji

Re: [PATCH] Make efi-pstore return a unique id

[Madper Xie]

seiji.aguchi-7rDLJAbr9SE@public.gmane.org writes:

>> Then will lost the sequence of our log. We will get lots of entries like
>> "dmesg-efi-`unique but meaningless number here`" in pstore fs. Who will
>> know which file is the latest record?
>
> Ah, that's good point.
>
>> And another side, the combin of timestamp, count and part is unique. Why
>> we generate a unique number from a unique number?
>> if you think "making a string from three ints and then a parse it to a
>> int again" is odd, i'd like to use ((timestamp * 100 + part) * 100 +
>> count.
>
> How about calculating the number of digit of part /count, instead of using fixed "100"?
> We can ensure the uniqueness of each id by doing it.
>
> digit_num = log2(part)/log2(10) + 1
> timestamp * 10^(digit_num + 1)
>
Then we must implement a log10 and a pow func...
And reverting id to timestamp, part and count will hard if we using
unfixed length of count and id.

So i'd like to using fixed length. My dell xps has 128kb nvram
space. and it at most store 126 pstore entries. So I think 1000 will be
a safe value for count. For part, 100 should be okay. I don't think
there will be a large kmsg and we must split it into more than 100 parts.

> Seiji