[prev in list] [next in list] [prev in thread] [next in thread] 

List:       hurd-bug
Subject:    Re: [PATCH] tmpfs: keep a reference to memory objects
From:       Sergio Lopez <sergio.lopez () sinrega ! org>
Date:       2010-05-19 15:02:22
Message-ID: 20100519170222.79e5e375 () sinrega ! org
[Download RAW message or body]

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.


[Attachment #3 (text/x-patch)]

diff -dur a/tmpfs/node.c b/tmpfs/node.c
--- a/tmpfs/node.c	2010-05-19 16:01:43.000000000 +0200
+++ b/tmpfs/node.c	2010-05-19 16:04:27.000000000 +0200
@@ -61,8 +61,10 @@
   switch (np->dn->type)
     {
     case DT_REG:
-      if (np->dn->u.reg.memobj != MACH_PORT_NULL)
+      if (np->dn->u.reg.memobj != MACH_PORT_NULL) {
+	vm_deallocate (mach_task_self (), np->dn->u.reg.memref, 4096);
 	mach_port_deallocate (mach_task_self (), np->dn->u.reg.memobj);
+      }
       break;
     case DT_DIR:
       assert (np->dn->u.dir.entries == 0);
@@ -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);
+
+      /* XXX we need to keep a reference to the object, or GNU Mach
+	 will terminate it when we release the map. */
+      vm_map (mach_task_self (), &np->dn->u.reg.memref, 4096, 0, 1,
+	      np->dn->u.reg.memobj, 0, 0, VM_PROT_NONE, VM_PROT_NONE,
+	      VM_INHERIT_NONE);
     }
 
   /* XXX always writable */
diff -dur a/tmpfs/tmpfs.h b/tmpfs/tmpfs.h
--- a/tmpfs/tmpfs.h	2010-05-19 15:51:44.000000000 +0200
+++ b/tmpfs/tmpfs.h	2010-05-19 16:00:03.000000000 +0200
@@ -47,6 +47,7 @@
     struct
     {
       mach_port_t memobj;
+      vm_address_t memref;
       unsigned int allocpages;	/* largest size while memobj was live */
     } reg;
     struct

[Attachment #4 (text/x-patch)]

diff -dur a/tmpfs/node.c b/tmpfs/node.c
--- a/tmpfs/node.c	2010-05-19 15:51:44.000000000 +0200
+++ b/tmpfs/node.c	2010-05-19 15:53:47.000000000 +0200
@@ -489,7 +489,7 @@
     {
       error_t err = default_pager_object_create (default_pager,
 						 &np->dn->u.reg.memobj,
-						 np->allocsize);
+						 vm_page_size);
       if (err)
 	{
 	  errno = err;


[prev in list] [next in list] [prev in thread] [next in thread]