On Sat, 6 May 2000, Alexander Viro wrote:
>
> a) Network filesystems are, arguably, abusing the pagecache. While the
> data is provided to specific _user_ (RPC signatures, etc.) we store it
> for everyone. Moreover, we can't tell which user had placed the data into
> mmaped page, so the signature/fhandle/fid may very well belong to another
> user.
So the right thing may be to have a way for the filesystem to control
things like that. Adding opaque datastrctures is not the way.
We had a very similar discussion about "struct address_space" when it was
introduced, and it started out having this notion of unions with different
data depending on the type of an address space.
In short, you had a "cookie" in the address space, and I said no.
Did _you_ actually look at the patch? The only thing it did, as far as I
can tell is
- remove "dentry" from the argument list. Fine. Not that I see _why_, as
it just means that anybody who now needs the inode will instead do
- struct inode *inode = dentry->d_inode;
+ struct inode *inode = (struct inode*)page->mapping->host;
whichin my opinion is not exactly making the code prettier.
If "mapping->host" was of type "struct inode *", I might agree with it.
At least it wouldn't need that terminally ugly cast. The code does in
fact apparently enforce the "mapping->host is of type 'struct inode'",
but doesn't make it explicit.
- remove "file" from the argument list, replacing it with "void *
cookie", which actually gets the value "file".
struct file * file = (struct file *)cookie;
which again is not, in my not so d*mn humble opinion, any improvement
at all.
In short, both of the changes resulted in (a) uglier code and (b) loss of
typechecking.
Note that the code did not add any new information - it couldn't do that.
It just hid the information we _did_ have available, and made it uglier.
And people then _applaud_ this move?
Now, a patch that I _would_ find fine is if it just changed readpage() and
writepage() to both just get the "struct file" rather than the current
write that gets both file and dentry. I just don't see why it wants to
hide the fact that yes, it still _does_ pass in "struct file".
Why?
> c) Natural way to deal with that is to store the relevant data in
> file->private_data. However, we don't need the friggin' struct file
> and for many situations we simply don't have one.
And this patch improves the situation exactly =how=?
What's so wrong with just passing in "struct file"? And admitting up front
that that is what we do, instead of calling it "void *cookie"?
Yes, there is a nasty situation. A memory-mapped shared page can have
_many_ "struct file"s associated with it. The patch under discussion does
not help that case at all, and we all know that it's a case that we simply
cannot handle right now - because we simply do not KNOW who did the write.
In the long run, we can introduce concepts like "exclusive access" etc.
The distributed memory guys already did something like that. At that
point, we can uniquely identify the particular "struct file" that did the
write, and that will be a good thing.
But face it - even then we would be a LOT better off just explicitly
passing in "struct file *file" instead of "void *cookie".
I just do not see any reason why this patch should be considered. Maybe it
the plan9fs code were to be shown and explained what the _advantages_ are
(as opposed to just showing a patch that only has disadvantages), I might
see the point. As it is, I think it's stupid.
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun May 07 2000 - 21:00:20 EST