Re: [PATCH] tmpfs: keep a reference to memory objects

[Sergio Lopez]

El Wed, 19 May 2010 10:37:21 +0200
<olafBuddenhagen@gmx.net> escribió:
> On Tue, May 18, 2010 at 04:21:14PM +0200, Sergio Lopez wrote:
> 
> > This patch modifies tmpfs to keep a reference (by mapping it into
> > its own space) to each memory object created by the user, so they
> > don't get inmediately terminated at the end of the current
> > operation.
> 
> Hm... Do I get it right, that when the client closes a file (or
> otherwise drops the mapping), the object holding the data was getting
> lost? Ouch...
>

Yes, but Mach is doing what we've told him, since the default pager is
creating the object with can_cache=FALSE. Anyway, if we change default
pager to create external objects with can_cache=TRUE, we'll still have
this problem when objects get expeled from cache.

The real problem is that Mach thinks that every object can be recreated
at any time. This is true for a translator like ext2fs, as we can
locate the contents of a file a provide its contents to Mach every time
we're requested to. But with the default pager, even if its contents
are currently written to swap space, if a object is terminated we lost
all our connections with actual data.

I think that, in future, in parallel with a new cache policy for memory
objects, we should implement a special treatment for named memory
objects without a backing store.
 
> (The comment in the code says something slightly different though. So
> it doesn't happen immediately, only when cleaning the cache?)
> 

You're right, I've wrote that comment when I was testing it with a
default pager which created objects with can_cache=TRUE. I've changed
it now.

> > @@ -489,7 +491,7 @@
> >      {
> >        error_t err = default_pager_object_create (default_pager,
> >                                              &np->dn->u.reg.memobj,
> > -                                            np->allocsize);
> > +                                            vm_page_size);
> >        if (err)
> >     {
> >       errno = err;
> 
> Hm... This change doesn't look like it's related to the issue you
> described above... Am I missing something, or did you sneak in an
> unrelated fix here? ;-)
>

Yes, it's unrelated. I've splitted it up in other patch.
 
> > @@ -500,6 +502,13 @@
> >      past the specified size of the file.  */
> >        err = default_pager_object_set_size (np->dn->u.reg.memobj,
> >                                        np->allocsize);
> > +      assert_perror (err);
> 
> Again, seems unrelated?...

No, this _is_ related :-). I want to make sure that the call to
default_pager_object_set_size didn't return an error before calling
vm_map().

> 
> > +      /* XXX we need to keep a reference to the object, or GNU Mach
> > +    could try to terminate it while cleaning object cache */
> 
> Why the XXX? Do you think this should be fixed another way in the
> long run?
> 

Yes, as I wrote above, IMHO there should be a special treatment for
objects without a backing store.

Here is a little explanation of these patches:

  - 01tmpfs_objectsize.patch: Create objects with size == vm_page_size,
    since the default_pager doesn't properly support other values.
    (object size will be virtually increased with successive calls to
    default_pager_set_size).

  - 02tmpfs_memref.patch: Keep a reference to the memory object. This
    is better explained above.

02tmpfs_memref.patch
Description: Text Data

01tmpfs_objectsize.patch
Description: Text Data