Re: [PATCH] tmpfs: now working

[Thomas Bushnell, BSG]

Neal H Walfield <neal@cs.uml.edu> writes:

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

You can hardly expect it to do otherwise.  It asked you to truncate
the file, you returned success, but failed to truncate it.  There are
a couple confusions here.

First confusion:

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.  

Now, the pagers here are weird, because tmpfs doesn't have control
over them.  What you are supposed to do is have the memory object port
hold a reference to the node (see ufs/pager.c and ext2fs/pager.c and
their implementation of diskfs_get_filemap [where they allocate a
reference] and pager_clear_user_data [where they release it]).

You can't do that, because you don't have a clue when the pager has
lost all its references.  It might work to just ignore the references,
but then you are outside the diskfs paradigm, and you can't blame
diskfs for assuming there were no references when in fact there were
some, and thus attempting diskfs_drop_node while there is still a live
user somewhere.

Ok, now Second Confusion:

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).  That means the code in
tmpfs/node.c:diskfs_truncate is thoroughly wrong.  Totally bogusly
wrong.  In my opinion, diskfs_truncate in tmpfs should do nothing at
all with the underlying object (because we basically can't).  It
should *always* succeed or return an error; to return zero but not
change allocsize is totally wrong.

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).  It *might* be a good idea to zero the pages here
for security concerns.  If you don't zero them in truncate, you have
to do it in grow.  I would just bzero the region by hand if necessary,
since you don't have control over the default pager's actions.
(Ideally we'd ask the default pager to revoke the pages from the
kernel and provide empty pages on the next pagein fault.)

Thomas