Samuel Thibault <samuel.thiba...@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.
