Re: tmpfs status

[Maksym Planeta]

Samuel Thibault <samuel.thibault@gnu.org> writes:
>
>> >> Correct errors in default pager and make it work with tmpfs.
>> >
>> > That needs more explanation about how it works.
>> >
>> 
>> This is quite a big commit and I made several things there:
>
> So it should be broken down in several commits.
>

OK.

>> >> Take into account that pager's map could have NULL value.
>> >
>> > When can that happen?
>> >
>> 
>> It is normal situation that pager's map could have NULL value. This
>> occurs when some parts of object haven't been pageouted.
>
> How so?  Which lines of code make it NULL?  Maybe you are not aware
> that I don't know the mach-defpager code at all, just vague concepts.
> What I currently see is that dpager_t structures get initialized from
> pager_alloc(), where the map field gets initialized to non-zero,
> and that is always called from pager_port_alloc, always called from
> seqnos_memory_object_create.  There is no code that explicitly sets it
> to NULL until deallocation in pager_dealloc().
>
>> As you can see, old_mapptr is supplied as source for new page map, in
>> memcpy, so NULL pointer will be dereferenced.
>
> I fully understand what bad things can happen if map happens to be NULL,
> but for now my reading of the code is that it can't become NULL, and
> thus if it becomes, there is a bug earlier, and it's a bad thing to just
> paper over it.
>

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.

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

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

>> >> Return support of old pageout format.
>> >
>> > AIUI, it's meant to fix the initialization issue that you raised some
>> > time ago?
>> 
>> No, this means that I should support both formats, because old one is
>> used by kernel. And new one is needed because new page attributes are
>> needed. So since this commit both interfaces are supported.
>
> That's actually what I meant. You should probably rather fold it into
> the patch that moves the code from _data_write to _data_return
>

OK.

Regards,
Maksym Planeta.