Re: [PATCH] tmpfs: now working

[Roland McGrath]

> Well, we have to do something, otherwise, when diskfs_drop_node is
> called, will trigger an assert.
> 
> Consider the following:  when diskfs_drop_node is called, if there is
> space allocated, it adds a reference and attempts to truncate the file
> to zero and happens to sets np->allocsize to 0.  diskfs_drop_node then
> drops its reference causing it to, eventually, restart.  This time,
> since np->allocsize is 0 it does not try to truncate the file, however,
> it asserts that the file size is zero.

Ah, I see.  That code is reasonable because it is attempting to handle the
case where diskfs_truncate(np,0) fails to set np->allocsize=0 because of
something like a disk IO error. 

Despite what Thomas has said, I believe that it is reasonable for
diskfs_truncate to return with allocsize arbitrarily higher than what was
asked for.  (It's up to the filesystem how it does allocation, and if its
method overallocates a truncated node--for whatever reason--allocsize
should reflect that.)

However, it is simply a bug for diskfs_truncate to return success without
setting st_size to exactly what was asked for.  This simple change fixes
that, anyway.  I just checked this in, though I can't test tmpfs now.

2001-05-05  Roland McGrath  <roland@frob.com>

            * node.c (diskfs_truncate): Set st_size before rounding it up,
            and do set it when there is a memory object.

Index: node.c
===================================================================
RCS file: /cvs/hurd/tmpfs/node.c,v
retrieving revision 1.7
diff -u -b -p -r1.7 node.c
--- node.c 2001/04/15 22:30:07  1.7
+++ node.c 2001/05/06 00:38:41
@@ -394,6 +394,8 @@ diskfs_truncate (struct node *np, off_t 
   if (default_pager == MACH_PORT_NULL)
     return EIO;
 
+  np->dn_stat.st_size = size;
+
   size = round_page (size);
 
   if (np->dn->u.reg.memobj != MACH_PORT_NULL)
@@ -405,7 +407,6 @@ diskfs_truncate (struct node *np, off_t 
 
   adjust_used (size - np->allocsize);
   np->dn_stat.st_blocks += (size - np->allocsize) / 512;
-  np->dn_stat.st_size = size;
   np->allocsize = size;
 
   return 0;


[From Thomas:]
> You can hardly expect it to do otherwise.  It asked you to truncate
> the file, you returned success, but failed to truncate it.  

I heartily agree with that, and think that the bug fix above addresses that
complaint per se.

> There are a couple confusions here.

At least.

> Diskfs_drop_node is called only when there are no outstanding
> references to the file: including memory objects.  If there is a
> memory object reference of any kind, and diskfs_drop_node is being
> called, you have a serious bug.  

I think Neal is aware of the meaning.  You seem to be overlooking the
discussion we've had here already about tmpfs.  As we have discussed, it
does some things we know not to be kosher, but we are trying to get it
working as close as possible to right within the constraints of the current
implementation that uses the unmodified defualt_pager_object interface.

It is certainly a dubious situation that users of the memory object for the
file are not tracked as references by diskfs.  However, I don't think that
needs to be considered a first-class assumption of diskfs, but rather a
secondary consequent assumption of how things must usually interact to get
the filesystem semantics right.  

Fortunately, in tmpfs diskfs_drop_node with st_nlink=0 is the one case
where it is really safe to lose track of the fact that users might still
have the memory object.  By definition, there is no way and never will
again be any way to try to access the same file again so as to detect 
whether or not you're sharing with that memory object.

> Diskfs_drop_node clamps the allocsize to zero as a safety check.  It is
> fundamentally not allowed for diskfs_truncate ever to fail to set
> NP->allocsize (unless there's an error).

I see no reason to make such a requirement in this interface, and I know of
no place before this here email of yours that ever said that was a
requirement of the interface.  It does indeed seem a proper requirement
that on a successful return from diskfs_truncate, st_size is set to exactly
the value requested.

> You don't have to bother communicating with the default pager; I'd
> just set allocsize and dn_stat.st_size and return.  Users using pages
> past the truncation will get data, but we don't care; we make no
> promises about what they see (importantly, we don't promise that they
> will get faults).

As we have previously discussed here, I think that POSIX.1 may require that
they do get faults.  It is certainly a desireable behavior.

> (Ideally we'd ask the default pager to revoke the pages from the
> kernel and provide empty pages on the next pagein fault.)

That has always been my plan.  I just went for getting tmpfs working as
fully as possible with semantics as close as possible to right and testing
it, before considering touching the default pager code.