Re: [PATCH] Make efi-pstore return a unique id
rich...@nod.at 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Make efi-pstore return a unique id
rich...@nod.at 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] Make efi-pstore return a unique id
> 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
N�Р骒r��yb�X�肚�v�^�)藓{.n�+�伐�{��赙zXФ�≤�}��财�z�:+v�����赙zZ+��+zf＂�h���~i���z� �wア�?�ㄨ��&�)撷f��^j谦y�m��@A�a囤�
0鹅h���i
RE: [PATCH] Make efi-pstore return a unique id
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
N�Р骒r��yb�X�肚�v�^�)藓{.n�+�伐�{��赙zXФ�≤�}��财�z�j:+v�����赙zZ+��+zf＂�h���~i���z� �wア�?�ㄨ���)撷f��^j谦y�m��@A�a囤�
0鹅h���i
Re: [PATCH] Make efi-pstore return a unique id
rich...@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, ))
> + 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 majord...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Make efi-pstore return a unique id
On Fri, Nov 1, 2013 at 1:57 PM, Seiji Aguchi 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] Make efi-pstore return a unique id
> 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
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, ))
+ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] Make efi-pstore return a unique id
> >> +{
> >> + char id_str[64];
> >> + u64 id = 0;
> >> +
> >> + sprintf(id_str, "%lu%u%d", timestamp, part, count);
> >> + if (kstrtoull(id_str, 10, ))
> >> + 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
>> +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, ))
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Make efi-pstore return a unique id
On Fri, Nov 1, 2013 at 5:14 PM, Madper Xie 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
> ---
> 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, ))
> + 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, , , , _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, , , ) == 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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.
Signed-off-by: Madper Xie
---
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, ))
+ 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, , , , _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, , , ) == 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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.
Signed-off-by: Madper Xie c...@redhat.com
---
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Make efi-pstore return a unique id
On Fri, Nov 1, 2013 at 5:14 PM, Madper Xie c...@redhat.com 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 c...@redhat.com
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
Thanks,
//richard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Make efi-pstore return a unique id
+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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] Make efi-pstore return a unique id
+{
+ 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
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] Make efi-pstore return a unique id
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
On Fri, Nov 1, 2013 at 1:57 PM, Seiji Aguchi seiji.agu...@hds.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Make efi-pstore return a unique id
rich...@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 majord...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
