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.
> > --This line, and those below, will be ignored--
> >
> > Index: subversion/libsvn_fs_fs/fs_fs.c
> > ===================================================================
> > --- subversion/libsvn_fs_fs/fs_fs.c (revision 1041339)
> > +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> > @@ -246,8 +246,6 @@ path_rev(svn_fs_t *fs, svn_revnum_t rev,
> > apr_psprintf(pool, "%ld", rev), NULL);
> > }
> >
> > -/* Returns the path of REV in FS, whether in a pack file or not.
> > - Allocate in POOL. */
> > svn_error_t *
> > svn_fs_fs__path_rev_absolute(const char **path,
> > svn_fs_t *fs,
> > @@ -256,45 +254,16 @@ svn_fs_fs__path_rev_absolute(const char
> > {
> > fs_fs_data_t *ffd = fs->fsap_data;
> >
> > - if (ffd->format < SVN_FS_FS__MIN_PACKED_FORMAT)
> > + if (ffd->format < SVN_FS_FS__MIN_PACKED_FORMAT
> > + || ! is_packed_rev(fs, rev))
> > {
> > *path = path_rev(fs, rev, pool);
> > - return SVN_NO_ERROR;
> > }
> > -
> > - if (! is_packed_rev(fs, rev))
> > + else
> > {
> > - svn_node_kind_t kind;
> > -
> > - /* Initialize the return variable. */
> > - *path = path_rev(fs, rev, pool);
> > -
> > - SVN_ERR(svn_io_check_path(*path, &kind, pool));
> > - if (kind == svn_node_file)
> > - {
> > - /* *path is already set correctly. */
> > - return SVN_NO_ERROR;
> > - }
> > - else
> > - {
> > - /* Someone must have run 'svnadmin pack' while this fs object
> > - * was open. */
> > -
> > - SVN_ERR(update_min_unpacked_rev(fs, pool));
> > -
> > - /* The rev really should be present now. */
> > - if (! is_packed_rev(fs, rev))
> > - return svn_error_createf(APR_ENOENT, NULL,
> > - _("Revision file '%s' does not exist, "
> > - "and r%ld is not packed"),
> > - svn_dirent_local_style(*path, pool),
> > - rev);
> > - /* Fall through. */
> > - }
> > + *path = path_rev_packed(fs, rev, "pack", pool);
> > }
> >
> > - *path = path_rev_packed(fs, rev, "pack", pool);
> > -
> > return SVN_NO_ERROR;
> > }
> >
>
> This part looks good. I'm more concerned about a bug in the "All
> callers use the write lock, ..." analysis than about a bug in the
> above hunk.
Right.
> 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?
> (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.
Do you have an idea whether something should be done differently? I
don't, not without studying it further.
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.
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.
> > 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.
- Julian
> > svn_error_t *
> > svn_fs_fs__path_rev_absolute(const char **path,
> > svn_fs_t *fs,
>
Received on 2010-12-02 15:34:09 CET