Re: tmpfs status

[Maksym Planeta]

Samuel Thibault <samuel.thibault@gnu.org> writes:

> Maksym Planeta, le Sat 07 Apr 2012 19:51:56 +0300, a écrit :
>> Here is initialization code from pager_alloc():
>>              if (INDIRECT_PAGEMAP(size)) {
>>                      alloc_size = INDIRECT_PAGEMAP_SIZE(size);
>>                      init_value = (dp_map_t)0;
>> 
>> And from pager_extend():
>>          for (; i < INDIRECT_PAGEMAP_ENTRIES(new_size); i++)
>>              new_mapptr[i].indirect = (dp_map_t)0;
>> 
>> As you can see instead of NULL, (dp_map_t)0 is used.
>
> And can be put into pager->map in pager_truncate, ok. I'm however not
> sure we really want to put ifs everywhere. The comment in the truncation
> says
>
>         /* We are truncating to a size small enough that it goes to using
>            a one-level map.  We already have that map, as the first and only
>            nonempty element in our indirect map.  */
>
> i.e. the code assumes that map[0].indirect is not NULL. I'd say we
> should rather allocate an empty map in such case, to keep the rest of
> the code simple.
>

And what is the alternative for ifs? longjumps and setjumps? Purpose of
this conditions is checking whether map (or submap) is already empty.

>> > There is also an issue with
>> >
>> > +           if (!pager->map) {
>> > +                invalidate_block (pager_offset);
>> > +                goto done;
>> > +            }
>> >             pager_offset = pager->map[f_page];
>> >
>> > at that point, pager_offset is not initialized yet...
>> >
>> 
>> invalidate_block is a macro that sets pager_offset, so, really,
>> pager_offset shouldn't been initialized yet.
>
> It sets the *content* pointed by pager_offset. It does not set the
> pager_offset pointer.
>

pager_offset is not a pointer, it is a union. So setting pager_offset
is the same as setting its content.

>> > Also I don't understand the use of
>> >
>> > +               if (no_block (*pager->map))
>> > +                   goto done;
>> >                 entry = pager->map[offset];
>> >                 invalidate_block(pager->map[offset]);
>> >
>> > *pager->map would be pager->map[0], I'd believe that it is independent
>> > from pager->map[offset], and clearing it can only bring issues.
>> >
>> 
>> Sorry, didn't get you.
>
> *pager->map is the same as pager->map[0]. AIUI, it has nothing to do
> with pager->map[offset]. So why testing it at all?
>

Hmm... Looks odd, indeed. I think that I should add assertion before
condition, which determines whether mapping indirect or not. Assertion
will check if pager->map is not NULL (I looked through code and it
called only in one function, that should supply pager to
pager_release_offset with non empty map). Finally, I'll  remove the
check, you've pointed at.