Re: tmpfs status

[Samuel Thibault]

Maksym Planeta, le Wed 28 Mar 2012 22:40:22 +0300, a écrit :
> Samuel Thibault <samuel.thibault@gnu.org> writes:
> 
> > Thomas Schwinge, le Mon 26 Mar 2012 22:24:55 +0200, a écrit :
> >> > 1. 
> >> > http://git.savannah.gnu.org/cgit/hurd/hurd.git/log/?h=mplaneta/tmpfs/master
> >> >  
> >> 
> >> Someone to review the patches...  :-/
> >
> > Just a few comments.
> >
> >> Fix auto-terminating of tmpfs due to idle.
> >
> > Err, really?  I'd rather not see my tmpfs fly away just because I've
> > left it dormant for some time.
> 
> I meant that now it doesn't auto-terminate.

Ah, ok :) I would understand "Fix auto-terminating" by "Fix the
autotermination", i.e. make auto-termination actually work.  I have
applied it with fixed changelog.

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

> >> Correct handling of object size.
> >
> > I guess why the first hooks are needed, but the last hook needs
> > explanation (dropping the call to default_pager_object_set_size).
> >
> 
> Object size is set in default_pager_object_create.

OK. I've applied it.

> >> Update comments.
> >
> > Why moving the comment and not duplicating it?
> 
> I moved comment because old function was just stub and didn't do
> anything. But now comment should be returned. I'll fix it.

I have applied the comment addition.

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

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

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.

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

Samuel