On Jul 20, 2006, at 3:17 PM, Julian Martin Kunkel wrote:
Hi,
Maybe if we could differentiate between user errors (ENOENT) and
unexpected system errors (ENOMEM, etc.), we could do that.
Unfortunately, right now we don't differentiate, so some fellow might
come along and do an ls on a directory or file that doesn't exist,
get an error, causing a flush of the dbs, and stalling the other
file creates that another user or processing is trying to do. I
would argue for not flushing for now.
ok. I agree. I thought about the case that a operation does multiple
modifications to the db but fails at the end and reverts the
changes. I
haven't found such a op in pvfs2, yet. But in case there are some
the db has
to be flushed to guarantee consistency. Are there such ops ?
I don't think so, no.
Sorry if I missed previous conversations about this, but doesn't
unlink just free the inode? Wouldn't rename take just as long if not
longer..
I don't think so, in think in case the file is still open it is not
deleted,
but in our case it is. Mh.. maybe we could fake the same behavior
by closing
the file later ?
I think phil talked about the problem during our meeting ?
Anyway that changes are only a couple of LOC...
Yes I forgot about Phil's comments during the meeting. It looks like
others have implemented changes in ext3 to fix this:
http://ext2.sourceforge.net/2005-ols/paper-html/node32.html
Those changes haven't made it into the everyday code though.
AFAICT, the only case where id_gen_safe_register _should_ be used, is
in returning an id back through the sysint calls. This is especially
needed for pvfs2-client-core, where the id is then passed to the
kernel module, so it must be an opaque 64 bit value. In all the
other cases (pvfs internal use), we are really just passing around
pointers. If the pointers somehow become invalid, using gen_safe
instead of gen_fast isn't going to help us. I would argue that the
other internal uses (it looks like there are a couple) of gen_safe
should be changed to gen_fast.
I think gen_safe register and lookup deal with the fact that a
object is
freed. Isn't the pointer still valid in case an object is
unregistered ? Thus
a free operation from test or testcontext can not interfere with
the other
test* ops, same with the cancel op.
The modification would make the interface
more stable and understandable and in case we later wan't to add
some test
functions functionable. I know currently these functions are called
in a way
that these cases can not happen. But, you never know what changes
happen to
the upper layers.
gen_safe just stores the pointer in a hashtable and returns a hash
key in replace of it. There's no reference counting done on the
pointer, so its not a way of implementing smart pointers.
There are already mutex locks around the dspace_test* calls, so they
can't be called simultaneously anyway.
In essence what you have is code that fails cleanly if someone tries
to call dspace_testcontext with an id, and then later call
dspace_test with the _same_ id, after dspace_testcontext had already
removed the id from the hashtable (because the operation had
completed). So its an awefully expensive way to show the developer
that he has bugs in his code.
What I would be in favor of is a modification to the id_gen_fast
calls that checks if debugging is enabled, and calls id_gen_safe if
so. That will help solve the safety concerns you have and still
allow the non-debug compiled code to use the faster version.
I would also change the checks in the dspace_test* functions to
asserts, since a null value from an id lookup is clearly a
programming error.
-sam
Overall this does cleanup the dbpf code nicely, so good work Julian.
Hopefully we can get some really good performance numbers out of all
this. There's still some style quirks in there in different places,
but those will go away over time, right? ;-)
Thanks, I still try to eliminate a bug in the code. Style quirks =>
*hehehe*
I try to adapt to your styles :)
See you tommorow,
Julian
_______________________________________________
Pvfs2-developers mailing list
[email protected]
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
_______________________________________________
Pvfs2-developers mailing list
[email protected]
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
