Re: [PATCH] pstore: avoid incorrectly mark entry as duplicate

[Madper Xie]

tony.luck@xxxxxxxxx writes:

> On Wed, Oct 23, 2013 at 7:55 AM, Madper Xie <cxie@xxxxxxxxxx> wrote:
>> 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.

> 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

>
> 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.
> -Tony

-- 
Best,
Madper Xie.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/