Re: tmpfs status

[Maksym Planeta]

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

> Maksym Planeta, le Sun 08 Apr 2012 00:17:43 +0300, a écrit :
>> Samuel Thibault <samuel.thibault@gnu.org> writes:
>> > Maksym Planeta, le Sat 07 Apr 2012 23:20:13 +0300, a écrit :
>> >> > No: as I said, allocate an empty map, so that the existing code can poke
>> >> > at it without testing for its presence or not.
>> >> >
>> >> >> Purpose of this conditions is checking whether map (or submap) is
>> >> >> already empty.
>> >> >
>> >> > Not empty, but allocated.
>> >> 
>> >> So, if, for instance, only one page of large object (that needs indirect
>> >> mapping) was evicted, the whole map would be allocated? And what the
>> >> purpose of two-level system in this case?
>> >
>> > I'm not saying to fill the whole two-level system.
>> >
>> > I'm saying to allocate the one-level system when truncation brings from
>> > two-level to one-level while the first map of the two-level happens to
>> > be NULL.
>> >
>> 
>> But thus you will not avoid conditions, because most of them are applied
>> to second level.
>
> Existing conditions, agreed.  But the conditions you introduce are about
> one-level only.
>

Really, but I can replace 3 conditions from the beginning of commit with
one. Would it fit?

>> Also you will need to allocate memory map for every object is created
>> even it would not ever use pageout.
>
> Well, that's already what we do in pager_alloc, don't we?
>

Than I'd rather removed initialization of map from pager_alloc, because,
I think, there is no sense to consume space for map if object doesn't
use it.

>> >> >> >> > 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.
>> >> >
>> >> > Oops, indeed, sorry about that. I'm still wondering, however: rather
>> >> > than a goto, why not just putting pager_offset = pager->map[f_page] in
>> >> > the else part?
>> >> >
>> >> 
>> >> pager_offset = pager->map[f_page] could be just put in else block, but I
>> >> did so because "goto done" shows clearer that everything was done and
>> >> execution could be move to finalization part. Just a matter of style. Do
>> >> you think that this should be changed?
>> >
>> > In both cases pager_offset is filled with something, so I don't see why
>> > it should be different. In the indirect map case, it's a mere else.
>> 
>> But pager_offset is filled anyway: either in condition or after.
>
> Yes, so why using a "goto" and not a mere "else"?  AIUI, the whole point
> of that part of the code is to set pager_offset.
>

I just wanted to emphasis that everything was done... But, never mind,
I'll remake this.

Regards,
Maksym Planeta.