Skip to contentSkip to conversation

pstore: avoid incorrectly mark entry as duplicate

PATCH In review Madper Xie 3 participants 6 messages Oct 23, 2013 – Oct 28, 2013

Subsystems (2)
EFI Pstore filesystem
lore
Share & tools
lore

Madper Xie

author
#0 lore raw

PATCH 1/1 pstore: avoid incorrectly mark entry as duplicate

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 one more check: timespec.

Signed-off-by Madper Xie <[email protected]>
---
 fs/pstore/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/pstore/inode.c +4 -1
index 1282384..f70f1e5 100644
@@ -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,8 @@ 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 &&
+		    !timespec_compare(&pos->time, &time)) {
 			rc = -EEXIST;
 			break;
 		}

@@ -312,6 +314,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:
Signature · 1 line
1.8.4.1
#1 lore raw
Quoted 38 lines · Madper Xie

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 one more check: timespec.

Signed-off-by Madper Xie <[email protected]>
---
 fs/pstore/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/pstore/inode.c +3 -1
index 1282384..f70f1e5 100644
@@ -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,8 @@ 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 &&
+                   !timespec_compare(&pos->time, &time)) {
                        rc = -EEXIST;
                        break;
                }

fs/pstore/inode.c int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count, · line 320

Quoted 7 lines
fs/pstore/inode.c +1 -0
@@ -312,6 +314,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:

As discussed on IRC, why don't you compare the variable names?

Signature · 2 lines
Thanks,
//richard
author
#2 lore raw
Quoted 44 lines · [email protected] writes:
Quoted 43 lines · Madper Xie

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 one more check: timespec.

Signed-off-by Madper Xie <[email protected]>
---
 fs/pstore/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/pstore/inode.c +4 -1
index 1282384..f70f1e5 100644
@@ -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,8 @@ 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 &&
+                   !timespec_compare(&pos->time, &time)) {
                        rc = -EEXIST;
                        break;
                }

@@ -312,6 +314,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));
Quoted 5 lines

fs/pstore/inode.c int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count, · line 320

Quoted 2 lines
fs/pstore/inode.c +0 -0
        switch (type) {
        case PSTORE_TYPE_DMESG:

As discussed on IRC, why don't you compare the variable names?

Howdy Richard,
Let's analyze the name of "duplicate" entries:

Quoted 2 lines
Quoted 2 lines

dump-type0-9-1-1382511508-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0 dump-type0-9-1-1382515661-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0

The have the same name except timestamp. In this case, there is no difference between compare names and compare timestamps.
( Yeah, I'm pretty sure the guid will be the same one -- LINUX_EFI_CRASH_GUID )
But efi is only one backend. For other backends, we don't sure if they will add timestamp to name. So...

Signature · 2 lines
Best,
Madper Xie.
#3 lore raw
In reply to Madper Xie
Quoted 2 lines · Madper Xie

The "duplicate" entries won't appear in pstorefs. And a complain will be print -- pstore: failed to load 76 record(s) from 'efi'

Maybe I don't quite get this - but it sounds like you have a whole lot of entries using up space in efivars that have similar names - differing just in the timestamp - that won't show up in the pstore filesystem - because we'd try to name them all the same thing.

How did all those things end up in efivars?

Wouldn't the right fix be to make pstore allow them all to appear (using the timestamp to differentiate names?) so that we could see them, log them, and then remove them from pstore (in turn freeing up efivars space - which people keep telling me is in short supply).

-Tony

author
#4 lore raw
Quoted 9 lines · [email protected] writes:
Quoted 2 lines · Madper Xie

The "duplicate" entries won't appear in pstorefs. And a complain will be print -- pstore: failed to load 76 record(s) from 'efi'

Maybe I don't quite get this - but it sounds like you have a whole lot of entries using up space in efivars that have similar names - differing just in the timestamp - that won't show up in the pstore filesystem - because we'd try to name them all the same thing.

Maybe I misunderstand you...

Sure pstore try to name them all the same thing, but it's another issue. and it doesn't prevent entries showing up in pstore fs.

Consider the following case: (after efi-pstore support append mode, it always like this case):

I choose four dumped efivars from my DELL XPS:
dump-type0-9-1-1380441690-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0 dump-type0-9-1-1380448560-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0 dump-type0-9-1-1380460890-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0 dump-type0-9-1-1382496073-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0

         ^        ^       [              ^                   ]
         !        !                      !

type timestamp GUID

When pstore load them from efivars, pstore incorrectly assuming that efivars with the same TYPE, ID and GUID are duplicate.

list_for_each_entry(pos, &allpstore, list) {
	if (pos->type == type &&    /---
	    pos->id == id &&     <-| as I said, it only check type,id,psi
	    pos->psi == psi) {      \---
		rc = -EEXIST;    <- then set -EEXIST, and ignore *dup* entry
		break;
	}
}

You can see the code above, for those four entries, only one could be showed in pstorefs, all others will get a -EEXIST. So I add a timestamp check here, it's the only different part.

Quoted 1 line

How did all those things end up in efivars?

before the patch I can see

dmesg-efi-1   dmesg-efi-11  dmesg-efi-3  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-10  dmesg-efi-2   dmesg-efi-4  dmesg-efi-6  dmesg-efi-8
after apply the patch:
[root@dhcp-13-41 vars]# ls /dev/pstore/
dmesg-efi-1  dmesg-efi-1   dmesg-efi-11  dmesg-efi-2  dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
dmesg-efi-1  dmesg-efi-10  dmesg-efi-11  dmesg-efi-2  dmesg-efi-3  dmesg-efi-5  dmesg-efi-7  dmesg-efi-8
dmesg-efi-1  dmesg-efi-10  dmesg-efi-12  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-8
dmesg-efi-1  dmesg-efi-10  dmesg-efi-13  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-14  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-15  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-16  dmesg-efi-3  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-8  dmesg-efi-9
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-8  dmesg-efi-9
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-8  dmesg-efi-9
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
Quoted 6 lines

Wouldn't the right fix be to make pstore allow them all to appear (using the timestamp to differentiate names?) so that we could see them, log them, and then remove them from pstore (in turn freeing up efivars space - which people keep telling me is in short supply).

Yeah, many file have the same name, just like my case above. But it not really block the file shows up and should be solved in another patch. And I'm trying fix it.

Quoted 1 line

-Tony

Signature · 2 lines
Best,
Madper Xie.
author
#5 lore raw

Howdy Tony,
Does this patch still need some rework?

Quoted 75 lines · [email protected] writes:
Quoted 9 lines · [email protected] writes:
Quoted 2 lines · Madper Xie

The "duplicate" entries won't appear in pstorefs. And a complain will be print -- pstore: failed to load 76 record(s) from 'efi'

Maybe I don't quite get this - but it sounds like you have a whole lot of entries using up space in efivars that have similar names - differing just in the timestamp - that won't show up in the pstore filesystem - because we'd try to name them all the same thing.

Maybe I misunderstand you...

Sure pstore try to name them all the same thing, but it's another issue. and it doesn't prevent entries showing up in pstore fs.

Consider the following case: (after efi-pstore support append mode, it always like this case):

I choose four dumped efivars from my DELL XPS:
dump-type0-9-1-1380441690-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0 dump-type0-9-1-1380448560-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0 dump-type0-9-1-1380460890-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0 dump-type0-9-1-1382496073-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0

         ^        ^       [              ^                   ]
         !        !                      !

type timestamp GUID

When pstore load them from efivars, pstore incorrectly assuming that efivars with the same TYPE, ID and GUID are duplicate.

list_for_each_entry(pos, &allpstore, list) {
	if (pos->type == type &&    /---
	    pos->id == id &&     <-| as I said, it only check type,id,psi
	    pos->psi == psi) {      \---
		rc = -EEXIST;    <- then set -EEXIST, and ignore *dup* entry
		break;
	}
}

You can see the code above, for those four entries, only one could be showed in pstorefs, all others will get a -EEXIST. So I add a timestamp check here, it's the only different part.

Quoted 1 line

How did all those things end up in efivars?

before the patch I can see

dmesg-efi-1   dmesg-efi-11  dmesg-efi-3  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-10  dmesg-efi-2   dmesg-efi-4  dmesg-efi-6  dmesg-efi-8
after apply the patch:
[root@dhcp-13-41 vars]# ls /dev/pstore/
dmesg-efi-1  dmesg-efi-1   dmesg-efi-11  dmesg-efi-2  dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
dmesg-efi-1  dmesg-efi-10  dmesg-efi-11  dmesg-efi-2  dmesg-efi-3  dmesg-efi-5  dmesg-efi-7  dmesg-efi-8
dmesg-efi-1  dmesg-efi-10  dmesg-efi-12  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-8
dmesg-efi-1  dmesg-efi-10  dmesg-efi-13  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-14  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-15  dmesg-efi-2  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-16  dmesg-efi-3  dmesg-efi-4  dmesg-efi-5  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-7  dmesg-efi-9
dmesg-efi-1  dmesg-efi-10  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-8  dmesg-efi-9
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-8  dmesg-efi-9
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-4  dmesg-efi-6  dmesg-efi-8  dmesg-efi-9
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
dmesg-efi-1  dmesg-efi-11  dmesg-efi-2   dmesg-efi-3  dmesg-efi-5  dmesg-efi-6  dmesg-efi-8
Quoted 6 lines

Wouldn't the right fix be to make pstore allow them all to appear (using the timestamp to differentiate names?) so that we could see them, log them, and then remove them from pstore (in turn freeing up efivars space - which people keep telling me is in short supply).

Yeah, many file have the same name, just like my case above. But it not really block the file shows up and should be solved in another patch. And I'm trying fix it.

Quoted 1 line

-Tony

Signature · 2 lines
Best,
Madper Xie.