subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julian.f...@wandisco.com>
Subject Re: svn commit: r1040832 - Port a fix for a FSFS packing race
Date Mon, 06 Dec 2010 17:36:35 GMT
I'm going to drop this "Remove the re-try logic from
svn_fs_fs__path_rev_absolute()" follow-up patch, as I don't want to get
into checking or messing with the txn-correctness of FSFS now.  If
Daniel or anyone else wants to pursue it, I'd be glad to help.

- Julian


On Thu, 2010-12-02, Daniel Shahaf wrote:
> Julian Foad wrote on Thu, Dec 02, 2010 at 14:33:19 +0000:
> > On Thu, 2010-12-02 at 14:56 +0200, Daniel Shahaf wrote:
> > > Julian Foad wrote on Thu, Dec 02, 2010 at 12:15:19 +0000:
> > > > Remove the re-try logic from svn_fs_fs__path_rev_absolute().  Since
> > > > r1040832, all its callers correctly account for the possibility of an
> > > > out-of-date result due to a concurrent packing operation.
> > > > 
> > > > The re-try logic was introduced in r875097 and reduced but did not eliminate
> > > > the window of opportunity for the caller to use an out-of-date result.
> > > > 
> > > > See the email thread <http://svn.haxx.se/dev/archive-2010-12/0019.shtml>,
> > > > subject "Re: svn commit: r1040832 - Port a fix for a FSFS packing race".
> > > > 
> > > > * subversion/libsvn_fs_fs/fs_fs.c
> > > >   (svn_fs_fs__path_rev_absolute): Remove the re-try logic.
> > > > 
> > > > * subversion/libsvn_fs_fs/fs_fs.h
> > > >   (svn_fs_fs__path_rev_absolute): Update the doc string accordingly.
> > > >  
> > > 
> > > In fact, doesn't the correctness of this change depend on the fact that
> > > svn_fs_fs__with_write_lock() calls update_min_unpacked_rev() before
> > > executing the body() callback?
> > 
> > Yes, it does.  Do you think it shouldn't?
> > 
> 
> I'm not sure whether it "should" or "shouldn't" depend.  The change IS
> correct, but it is only correct because the ffd->min_unpacked_rev is
> known to be up-to-date whenever the write lock is held.
> 
> That's a bit of a spaghetti effect there.  Now, is there another such
> effect that means the patch is wrong?  (I haven't come up with one yet,
> but that doesn't mean there isn't any.)
> 
> > >   (Otherwise we don't know whether there
> > > is a "concurrent" packer, or just ffd->min_unpacked_rev is stale.)
> > 
> > You know, I'm not too clear on the design intention here.  I would have
> > assumed it would be the job of all the fs_fs.c code to ensure that
> > ffd->min_unpacked_rev is never stale (due to its own actions) at any
> > point where it matters.  But the way it does that seems to be to re-read
> > the file in several places, instead of simply updating
> > ffd->min_unpacked_rev when it writes the file.  Maybe the idea was that
> > that method would pick up both internal and concurrent (external)
> > changes in the same way - I don't know.  Seems odd.
> > 
> 
> Yes, that was the idea.  If two processes access the filesystem at the
> same time, and one of them calls svn_fs_pack(), then the other's
> ffd->min_unpacked_rev would be stale.
> 
> It's the same story with ffd->youngest_rev, by the way; compare
> ensure_revision_exists(), which I believe predates the packing logic.
> 
> > Do you have an idea whether something should be done differently?  I
> > don't, not without studying it further.
> > 
> 
> I'm not sure.  Ultimately, ffd->min_unpacked_rev is just a cache, so we
> have to update it when we're about to do something that depends on its
> value.
> 
> In the meantime, let me at least attempt to define the "something" which
> perhaps should be done differently:
> 
> Given the way packing works today, it's possible for a revision file to
> stop existing where you expected it to exist.  Today the code informs
> itself of that situation by re-reading min-packed-rev and retrying.
> (Further up the stack, the code calculating the offset to seek to also
> needs to know whether it's seeking a pack file or a revision file.)
> 
> > I note that the following comment in pack_shard() is not quite right:
> > 
> >   /* Update the min-unpacked-rev file to reflect our newly packed shard.
> >    * (ffd->min_unpacked_rev will be updated by open_pack_or_rev_file().)
> > 
> > That may or may not be true, but it doesn't seem like this function has
> > any right to make that assertion.
> > 
> 
> We could remove the comment and have that function call update_min_unpacked_rev().
> (This would also save us a failed disk access later.)
> 
> But is it strictly *necessary* to do so?  I think not: by not calling
> update_min_unpacked_rev(), we cause ffd->min_unpacked_rev to become
> stale --- a situation which the code has to account for anyway.
> 
> > And in pack_revprop_shard(), it's the same, verbatim:
> > 
> >   /* Update the min-unpacked-rev file to reflect our newly packed shard.
> > 
> > should say 'the min-unpacked-revprop file';
> > 
> >    * (ffd->min_unpacked_rev will be updated by open_pack_or_rev_file().)
> > 
> > and that second line looks completely bogus in this context.
> > 
> 
> Yes, whoever did the copy-paste forgot to update some places.
> 
> > 
> > > > Index: subversion/libsvn_fs_fs/fs_fs.h
> > > > ===================================================================
> > > > --- subversion/libsvn_fs_fs/fs_fs.h	(revision 1041339)
> > > > +++ subversion/libsvn_fs_fs/fs_fs.h	(working copy)
> > > > @@ -411,8 +411,10 @@ svn_error_t *svn_fs_fs__move_into_place(
> > > >     Allocate *PATH in POOL.
> > > >  
> > > >     Note: If the caller does not have the write lock on FS, then the path
is
> > > > -   not guaranteed to remain correct after the function returns, because
the
> > > > -   revision might become packed just after this call. */
> > > > +   not guaranteed to be correct or to remain correct after the function
> > > > +   returns, because the revision might become packed before or after
this
> > > > +   call.  If a file exists at that path, then it is correct; if not,
then
> > > > +   the caller should call update_min_unpacked_rev() and re-try once.
*/
> > > 
> > > +1 semantically.  However, update_min_unpacked_rev() is private to
> > > fs_fs.c, so technically you aren't supposed to mention it in this
> > > context.
> > 
> > Good point.  Really that means any external caller of this API has to
> > hold the write lock.  Calling update_min_unpacked_rev() and re-trying is
> > only an option for internal callers (within fs_fs.c).  I wonder if this
> > should give us a clue about the questions above.
> > 
> 
> First, I still think having the update_min_unpacked_rev() reference
> there is useful.
> 
> Would it be a layering violation for code in libsvn_fs_fs outside of
> fs_fs.c to call update_min_unpacked_rev()?  I'm not sure.
> 
> > - Julian
> > 
> > 
> > > >  svn_error_t *
> > > >  svn_fs_fs__path_rev_absolute(const char **path,
> > > >                               svn_fs_t *fs,
> > > 
> > 
> > 



Mime
View raw message