[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1040832 - Port a fix for a FSFS packing race

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Thu, 02 Dec 2010 15:34:48 +0000

Daniel Shahaf wrote:
> Julian Foad wrote on Thu, Dec 02, 2010 at 14:33:19 +0000:
> > 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.

First step: this patch fixes the comments. Good to commit?

[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1041350)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -7606,8 +7606,8 @@ pack_shard(const char *revs_dir,
   SVN_ERR(svn_io_set_file_read_only(manifest_file_path, FALSE, pool));
 
   /* 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().)
- */
+ * (This doesn't update ffd->min_unpacked_rev. That will be updated by
+ * open_pack_or_rev_file() when necessary.) */
   final_path = svn_dirent_join(fs_path, PATH_MIN_UNPACKED_REV, iterpool);
   SVN_ERR(svn_stream_open_unique(&tmp_stream, &tmp_path, fs_path,
                                    svn_io_file_del_none, iterpool, iterpool));
@@ -7674,9 +7674,8 @@ pack_revprop_shard(svn_fs_t *fs,
       SVN_ERR(svn_sqlite__insert(NULL, stmt));
     }
 
- /* 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().)
- */
+ /* Update the min-unpacked-revprop file to reflect our newly packed shard.
+ * (This doesn't update ffd->min_unpacked_revprop.) */
   final_path = svn_dirent_join(fs_path, PATH_MIN_UNPACKED_REVPROP, iterpool);
   SVN_ERR(svn_stream_open_unique(&tmp_stream, &tmp_path, fs_path,
                                  svn_io_file_del_none, iterpool, iterpool));
]]]

- Julian
Received on 2010-12-02 16:35:30 CET

This is an archived mail posted to the Subversion Dev mailing list.