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

[Madper Xie]

Howdy Tony,
  Does this patch still need some rework?
  
cxie@xxxxxxxxxx writes:

> 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/