subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: svn commit: r1040832 - Port a fix for a FSFS packing race
Date Thu, 02 Dec 2010 12:56:48 GMT
Julian Foad wrote on Thu, Dec 02, 2010 at 12:15:19 +0000:
> On Wed, 2010-12-01, Daniel Shahaf wrote: 
> > Julian Foad wrote on Wed, Dec 01, 2010 at 12:32:45 +0000:
> > > (2) Doesn't the exact same race exist in *all* uses of
> > > svn_fs_fs__path_rev_absolute()?  There are five other calls to it,
> > 
> > As far as I can tell, apart from open_pack_or_rev_file(), all callers
> > always[1] run under the write lock.  Since pack operation take the write
> > lock too, it's not possible for the result of svn_fs_fs__path_rev_absolute()
> > to become outdated for those callers.
> 
> OK, that's good.  No change needed then.
> 
> 
> > > -/* Sets *PATH to the path of REV in FS, whether in a pack file or not.
> > > +/* Set *PATH to the path of REV in FS, whether in a pack file or not.
> > > +   The path is not guaranteed to remain correct after the function returns,
> > > +   because it is possible that the revision will become packed just after
> > > +   this call, so the caller should re-try once if the path is not found.
> > >     Allocate in POOL. */
> > 
> > Sounds right.
> > 
> > Bordering on bikeshed, I would suggest some changes:
> > 
> > * Separate describing what the function does ("Sets *PATH to FOO and
> >   allocates in POOL") from describing the conditions the caller should
> >   beware of / check for ("sometimes the return value will be out-of-date
> >   by the time you receive it").
> > 
> > * Mention that the non-guarantee is not in effect if the caller has the
> >   write lock.
> 
> OK, committed in r1041056 with these improvements.
> 

+1, thanks.

> > >  svn_error_t *
> > >  svn_fs_fs__path_rev_absolute(const char **path,
> > >                               svn_fs_t *fs,
> > >                               svn_revnum_t rev,
> > >                               apr_pool_t *pool);
> > 
> > Also, svn_fs_fs__path_rev_absolute() also does, internally, a single
> > repeat loop.  Theoretically this isn't needed any more now (since all
> > callers either run under the write lock or retry)...
> 
> Can you check the attached patch for this, please?
> 
> - Julian
> 
> 

> 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.

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?  (Otherwise we don't know whether there
is a "concurrent" packer, or just ffd->min_unpacked_rev is stale.)

> 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.

>  svn_error_t *
>  svn_fs_fs__path_rev_absolute(const char **path,
>                               svn_fs_t *fs,


Mime
View raw message