You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Sander Striker <st...@apache.org> on 2003/02/28 08:48:25 UTC
RE: svn commit: rev 5136 - trunk/subversion/libsvn_fs
> From: sussman@tigris.org [mailto:sussman@tigris.org]
> Sent: Thursday, February 27, 2003 8:41 PM
> New Revision: 5136
>
> Modified:
> trunk/subversion/libsvn_fs/reps-strings.c
> Log:
>
> A fix for Sander's checksum caching patch (r4908), which itself was a
> rewrite of the original r4882 fix.
>
> * reps-strings.c (struct rep_write_baton): add a 'finalized' boolean,
> indicating if we've final digest cache has been filled in yet.
>
> (rep_write_close_contents): use the flag, so we don't repeatedly
> finalize the md5_context. (Finalizing destroys the context.)
I really, really don't get this. Are you saying that rep_write_close_contents()
is being called multiple times on retry?! I thought only
txn_body_write_close_rep() was being called repeatedly on retry. Something
else seems broken here.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: code flow in svn_fs
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Mar 01, 2003 at 02:18:23PM +0100, Sander Striker wrote:
>...
> svn_fs_apply_textdelta
> svn_fs__retry_txn(txn_body_apply_textdelta)
> svn_fs__dag_get_edit_stream
> svn_fs__rep_contents_write_stream
> baton = rep_write_get_baton
> stream = svn_stream_create(baton)
> svn_stream_set_write(stream, rep_write_contents)
> svn_stream_set_close(stream, rep_write_close_contents)
>
>
> What happens with the stream from here on? Is it really
> possible that the stream is closed _more than once_?
> The answer lies in the main window handler returned by
> svn_fs_apply_textdelta:
>
> window_consumer(stream)
> svn_fs__retry_txn(txn_body_txdelta_finalize_edits(stream))
> svn_stream_close(stream) <-- !!!
> svn_fs__dag_finalize_edits
Woah! That doesn't look like Good Behavior. Why don't we lose the closer
function on the stream, and just call rep_write_close_contents manually, and
then move the stream closing out to window_consumer() (or simply ignore
closing the thing).
Closing a stream twice isn't doc'd in the streams functions as safe. But we
can define rep_write_close_contents as safe to call multiple times.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
code flow in svn_fs, WAS: RE: svn commit: rev 5136 - trunk/subversion/libsvn_fs
Posted by Sander Striker <st...@apache.org>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
> Sent: Friday, February 28, 2003 9:01 PM
[...]
> When this is all done, svn_fs_apply_textdelta() has returned a window
> consumer that expects that stream to still be available for writes!
Yes, I can see that. What I don't get is why the stream is being closed
more than once.
[...]
> Anyway, when that stream is finally closed, years later, there's no
> way for us to know how many retries (that is, the retry loop inside
> svn_fs__retry_txn) it will take. Like any other function that happens
> inside a trail, rep_write_close_contents() must be be reinvokable --
> not necessarily reentrant, just redoable, since there's no telling how
> many times the whole trail might get run before it succeeds.
>
> We were violating the retryability rule, since an md5 context can only
> stand to be finalized once.
>
> Does this help?
Well, I didn't fully get it until I went through the code myself, so for
educational reasons I'll post my findings here so others can benefit.
This is mostly simplified code and callstacks, but it should be readable.
svn_fs_apply_textdelta
svn_fs__retry_txn(txn_body_apply_textdelta)
svn_fs__dag_get_edit_stream
svn_fs__rep_contents_write_stream
baton = rep_write_get_baton
stream = svn_stream_create(baton)
svn_stream_set_write(stream, rep_write_contents)
svn_stream_set_close(stream, rep_write_close_contents)
What happens with the stream from here on? Is it really
possible that the stream is closed _more than once_?
The answer lies in the main window handler returned by
svn_fs_apply_textdelta:
window_consumer(stream)
svn_fs__retry_txn(txn_body_txdelta_finalize_edits(stream))
svn_stream_close(stream) <-- !!!
svn_fs__dag_finalize_edits
So, apparently yes it can. Now I see why you'd need a flag to
indicate we already have finalized the checksum, since you might
revisit this code aswell (sigh).
rep_write_contents(baton)
svn_fs__retry_txn(txn_body_write_rep(baton))
apr_md5_update is called after the operations that might fail, so
you never update the checksum twice for the same data. That is,
unless the operation trying to write one window fails after
rep_write_contents and is itself wrapped in svn_fs__retry_txn.
rep_write_close_contents(baton)
if (!finalized)
apr_md5_finalize
finalized = true
svn_fs__retry_txn(txn_body_write_close_rep(baton))
Make sense?
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5136 - trunk/subversion/libsvn_fs
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Sander Striker" <st...@apache.org> writes:
> >> I really, really don't get this. Are you saying that
> >> rep_write_close_contents() is being called multiple times on retry?!
> >
> > Yes, it's all part of a very large trail.
>
> Sorry for being so ignorant, but without an explanation of how this
> works (the trail and the reason for calling rep_write_close_contents twice)
> it comes across as broken :(.
>
> Can you please elaborate so I can feel warm and fuzzy about the fs code
> again? ;) :)
Notice that svn_fs__rep_contents_write_stream() doesn't actually
*call* rep_write_close_contents(). It just sets it as the close
function on a stream. Now, that stream might ultimately outlive the
particular public fs function call that created it. For example:
svn_fs_apply_textdelta
CALLS svn_fs__retry_txn(txn_body_apply_textdelta)
CALLS svn_fs__rep_contents_write_stream
CALLS svn_fs__dag_get_edit_stream
When this is all done, svn_fs_apply_textdelta() has returned a window
consumer that expects that stream to still be available for writes!
This is why, in svn_fs__rep_contents_write_stream(), the write baton
is not allocated in trail->pool, but in an explicitly passed pool.
So the stream must be useable outside the particular trail in which it
was created, that is, the trail in svn_fs__retry_txn() in the call
stack above. That's why rep_write_close_contents() and
rep_read_contents() are special, in that they can create their own
trail internally (though that isn't directly relevant to your
question, it's just an interesting side note. And the reason they
have the option to use the passed-in trail is that that's what you
always want to do when possible, to avoid deadlock).
Anyway, when that stream is finally closed, years later, there's no
way for us to know how many retries (that is, the retry loop inside
svn_fs__retry_txn) it will take. Like any other function that happens
inside a trail, rep_write_close_contents() must be be reinvokable --
not necessarily reentrant, just redoable, since there's no telling how
many times the whole trail might get run before it succeeds.
We were violating the retryability rule, since an md5 context can only
stand to be finalized once.
Does this help?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn commit: rev 5136 - trunk/subversion/libsvn_fs
Posted by Sander Striker <st...@apache.org>.
> From: sussman@collab.net [mailto:sussman@collab.net]
> Sent: Friday, February 28, 2003 6:41 PM
> "Sander Striker" <st...@apache.org> writes:
>> I really, really don't get this. Are you saying that
>> rep_write_close_contents() is being called multiple times on retry?!
>
> Yes, it's all part of a very large trail.
Sorry for being so ignorant, but without an explanation of how this
works (the trail and the reason for calling rep_write_close_contents twice)
it comes across as broken :(.
Can you please elaborate so I can feel warm and fuzzy about the fs code
again? ;) :)
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5136 - trunk/subversion/libsvn_fs
Posted by Ben Collins-Sussman <su...@collab.net>.
"Sander Striker" <st...@apache.org> writes:
> > From: sussman@tigris.org [mailto:sussman@tigris.org]
> > Sent: Thursday, February 27, 2003 8:41 PM
>
>
> > New Revision: 5136
> >
> > Modified:
> > trunk/subversion/libsvn_fs/reps-strings.c
> > Log:
> >
> > A fix for Sander's checksum caching patch (r4908), which itself was a
> > rewrite of the original r4882 fix.
> >
> > * reps-strings.c (struct rep_write_baton): add a 'finalized' boolean,
> > indicating if we've final digest cache has been filled in yet.
> >
> > (rep_write_close_contents): use the flag, so we don't repeatedly
> > finalize the md5_context. (Finalizing destroys the context.)
>
> I really, really don't get this. Are you saying that
> rep_write_close_contents() is being called multiple times on retry?!
Yes, it's all part of a very large trail.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org