Make efi-pstore return a unique id
Subsystems (2)
PATCH 1/1 Make efi-pstore return a unique id
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.
--- drivers/firmware/efi/efi-pstore.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
drivers/firmware/efi/efi-pstore.c +18 -4
@@ -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];
Signature · 1 line
1.8.4.2
Quoted 18 lines · 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 <[email protected]>--- drivers/firmware/efi/efi-pstore.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)drivers/firmware/efi/efi-pstore.c +0
-0@@ -39,6 +39,17 @@ struct pstore_read_data {char **buf; };
drivers/firmware/efi/efi-pstore.c struct pstore_read_data { · line 51
Quoted 10 lines
drivers/firmware/efi/efi-pstore.c +10
-0+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.
Quoted 54 lines
drivers/firmware/efi/efi-pstore.c +7
-5static 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
Thanks, //richard
Quoted 1 line
Quoted 1 line
+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.
Quoted 4 lines
Quoted 4 lines
drivers/firmware/efi/efi-pstore.c +4
-0+{ +char id_str[64 ]; +u64 id =0 ; +
Quoted 8 lines
drivers/firmware/efi/efi-pstore.c struct pstore_read_data { · line 51
Quoted 5 lines
drivers/firmware/efi/efi-pstore.c +5
-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
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/firmware/efi/efi-pstore.c +4
-0+{ +char id_str[64 ]; +u64 id =0 ; +Quoted 8 lines
drivers/firmware/efi/efi-pstore.c struct pstore_read_data { · line 51
Quoted 5 lines
drivers/firmware/efi/efi-pstore.c +5
-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.
Quoted 2 lines
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.
drivers/firmware/efi/efi-pstore.c static int efi_pstore_write(enum pstore_type_id type, · line 145
Quoted 11 lines
drivers/firmware/efi/efi-pstore.c +3
-1@@ -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
Quoted 4 lines
drivers/firmware/efi/efi-pstore.c +0
-1for (i =0 ; i <DUMP_NAME_LEN ; i++) efi_name[i] = name[i]; --1.8.4.2
Am 01.11.2013 20:22, schrieb Seiji Aguchi:
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
drivers/firmware/efi/efi-pstore.c struct pstore_read_data { · line 51
Quoted 9 lines
drivers/firmware/efi/efi-pstore.c +9
-0+{ +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.
Quoted 8 lines
Quoted 2 lines
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
Quoted 2 lines
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
Quoted 4 lines · Seiji Aguchi
Quoted 2 lines
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
Quoted 10 lines
How does efivars backend handle "unlink(2)" in the pstore file system. pstore will call the
backend->erasefunction 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
Quoted 23 lines · [email protected] writes:
Am 01.11.2013 20:22, schrieb Seiji Aguchi:
Quoted 21 lines
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/firmware/efi/efi-pstore.c +4
-0+{ +char id_str[64 ]; +u64 id =0 ; +Quoted 8 lines
drivers/firmware/efi/efi-pstore.c struct pstore_read_data { · line 51
Quoted 5 lines
drivers/firmware/efi/efi-pstore.c +5
-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.
Quoted 2 lines
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 ?
Quoted 10 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
Best, Madper Xie.
Quoted 5 lines · richard-/[email protected] writes:
Am 01.11.2013 20:22, schrieb Seiji Aguchi:
Quoted 4 lines
Quoted 3 lines
Agreed. I liked your ((timestamp * 100 + part) * 100 + count function much more than this.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 4 lines
Using this way you get a cheap unique id.
Thanks,
//richard
Quoted 4 lines
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.
Quoted 5 lines
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
Quoted 12 lines · [email protected] writes:
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
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.
Quoted 1 line
Seiji