Re: tmpfs status

[Maksym Planeta]

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.

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

  1. Macros for debugging purposes were slightly changed, so using them
  became more handy.
  2. Was handled properly case when size of object could be not rounded
  to page size. Therefor new field was added to struct pdager to store
  exact size of object in bytes.
  3. Was added support for zero-sized objects.
  4. Was disabled hack that relies on specifics of algorithm of function
  for allocation memory.
  5. Was taken into account that there could be "holes" in pager map.
  6. Default pager was moved to new pageout interface.
  7. Function kfree does nothing if it is asked to free NULL address.
  8. Removed some bugs in function pager_truncate.

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

>> Don't panic in default_pager_object_set_size.
>
> Ditto.  Why is it normal that it happens?
>

This don't seem to be normal. I'll correct this.

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

>
>> 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. But when object
is truncated, map should be truncated too. So, lets consider, for
example, this code snippet:

  pager->map = (dp_map_t) kalloc (PAGEMAP_SIZE (new_size));
  memcpy (pager->map, old_mapptr, PAGEMAP_SIZE (new_size));
  kfree ((char *) old_mapptr, PAGEMAP_SIZE (old_size));

As you can see, old_mapptr is supplied as source for new page map, in
memcpy, so NULL pointer will be dereferenced.

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

>> Update '..' link for directory when moving it.
>
> It's a bit odd to have to use a special NULL value. ext2fs rather stores
> the lookup_type in a type field of struct dirstat.  That looks much
> better.
>

Ok. I'll add new field in dirstat that stores if diskfs_lookup_hard was
called with type RENAME for '..' directory.

> Thanks for your work, and looking forward your updated patches.
> Samuel

How have I make updated patches? Amend old ones, revert them and then
make new patches, but already right or just make patches that correct
present errors?

Thank you for review,
Maksym Planeta.