On 27 May 2015 at 19:56, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 27 May 2015 at 19:48, Philip Martin <philip.mar...@wandisco.com> wrote:
>> Julian Foad <julianf...@btopenworld.com> writes:
>>
>>> Evgeny Kotkov wrote:
>>>> Philip, Julian, thank you for giving this patch a look.
>>>>
>>>> As I said in my previous e-mails, I think that the committed fix is a
>>>> better
>>>> choice here, as opposed to attempts to achieve the same behavior using the
>>>> stream API. [...]
>>>
>>> That sounds good to me -- leaving the code using file APIs.
>>
> +1.
>
>> It only fixes writing revprops during commit, it still leaves the
>> problem that the pack operation doesn't flush. We need to add flush
>> when writing the revision pack file, the packed indices, the revprop
>> pack file and the revprop manifest file. That means replacing more, if
>> not all, of the stream code in revprops.c.
>>
> I'm currently working on patch to fix packing and hotcopy code.
>
Please find patch that adds disk flushes to FSFS packing code in attachment.
I'm currently testing it and going to commit it later. This patch
should also slightly improve packing performance as a bonus, since we
can avoid APR buffered IO for some cases.
--
Ivan Zhakov
Index: subversion/libsvn_fs_fs/pack.c
===================================================================
--- subversion/libsvn_fs_fs/pack.c (revision 1681799)
+++ subversion/libsvn_fs_fs/pack.c (working copy)
@@ -384,6 +384,8 @@
SVN_ERR(svn_io_remove_file2(proto_l2p_index_path, FALSE, pool));
SVN_ERR(svn_io_remove_file2(proto_p2l_index_path, FALSE, pool));
+ /* Ensure that packed file is written to disk.*/
+ SVN_ERR(svn_io_file_flush_to_disk(context->pack_file, pool));
SVN_ERR(svn_io_file_close(context->pack_file, pool));
return SVN_NO_ERROR;
@@ -1654,7 +1656,9 @@
apr_pool_t *pool)
{
const char *pack_file_path, *manifest_file_path;
- svn_stream_t *pack_stream, *manifest_stream;
+ apr_file_t *pack_file;
+ apr_file_t *manifest_file;
+ svn_stream_t *manifest_stream;
svn_revnum_t end_rev, rev;
apr_off_t next_offset;
apr_pool_t *iterpool;
@@ -1663,13 +1667,18 @@
pack_file_path = svn_dirent_join(pack_file_dir, PATH_PACKED, pool);
manifest_file_path = svn_dirent_join(pack_file_dir, PATH_MANIFEST, pool);
- /* Create the new directory and pack file. */
- SVN_ERR(svn_stream_open_writable(&pack_stream, pack_file_path, pool,
- pool));
+ /* Create the new directory and pack file.
+ * Use unbuffered apr_file_t since we're going to write using big
+ * chunks. */
+ SVN_ERR(svn_io_file_open(&pack_file, pack_file_path,
+ APR_WRITE | APR_CREATE | APR_EXCL,
+ APR_OS_DEFAULT, pool));
/* Create the manifest file. */
- SVN_ERR(svn_stream_open_writable(&manifest_stream, manifest_file_path,
- pool, pool));
+ SVN_ERR(svn_io_file_open(&manifest_file, manifest_file_path,
+ APR_WRITE | APR_BUFFERED | APR_CREATE | APR_EXCL,
+ APR_OS_DEFAULT, pool));
+ manifest_stream = svn_stream_from_aprfile2(manifest_file, TRUE, pool);
end_rev = start_rev + max_files_per_dir - 1;
next_offset = 0;
@@ -1696,16 +1705,23 @@
/* Copy all the bits from the rev file to the end of the pack file. */
SVN_ERR(svn_stream_open_readonly(&rev_stream, path, iterpool, iterpool));
- SVN_ERR(svn_stream_copy3(rev_stream, svn_stream_disown(pack_stream,
- iterpool),
+ SVN_ERR(svn_stream_copy3(rev_stream,
+ svn_stream_from_aprfile2(pack_file, TRUE, pool),
cancel_func, cancel_baton, iterpool));
}
+ /* Currently stream over file doesn't buffer data, but it may in future. */
+ SVN_ERR(svn_stream_close(manifest_stream));
+ /* Ensure that pack file is written to disk. */
+ SVN_ERR(svn_io_file_flush_to_disk(manifest_file, pool));
+ SVN_ERR(svn_io_file_close(manifest_file, pool));
+
/* disallow write access to the manifest file */
- SVN_ERR(svn_stream_close(manifest_stream));
SVN_ERR(svn_io_set_file_read_only(manifest_file_path, FALSE, iterpool));
- SVN_ERR(svn_stream_close(pack_stream));
+ /* Ensure that pack file is written to disk. */
+ SVN_ERR(svn_io_file_flush_to_disk(pack_file, pool));
+ SVN_ERR(svn_io_file_close(pack_file, pool));
svn_pool_destroy(iterpool);
Index: subversion/libsvn_fs_fs/revprops.c
===================================================================
--- subversion/libsvn_fs_fs/revprops.c (revision 1682076)
+++ subversion/libsvn_fs_fs/revprops.c (working copy)
@@ -1215,10 +1215,13 @@
SVN_ERR(svn__compress(uncompressed, compressed, compression_level));
/* write the pack file content to disk */
- stream = svn_stream_from_aprfile2(pack_file, FALSE, scratch_pool);
+ stream = svn_stream_from_aprfile2(pack_file, TRUE, scratch_pool);
SVN_ERR(svn_stream_write(stream, compressed->data, &compressed->len));
SVN_ERR(svn_stream_close(stream));
+ SVN_ERR(svn_io_file_flush_to_disk(pack_file, scratch_pool));
+ SVN_ERR(svn_io_file_close(pack_file, scratch_pool));
+
svn_pool_destroy(iterpool);
return SVN_NO_ERROR;
@@ -1236,6 +1239,7 @@
apr_pool_t *scratch_pool)
{
const char *manifest_file_path, *pack_filename = NULL;
+ apr_file_t *manifest_file;
svn_stream_t *manifest_stream;
svn_revnum_t start_rev, end_rev, rev;
apr_off_t total_size;
@@ -1252,9 +1256,13 @@
/* Create the new directory and manifest file stream. */
SVN_ERR(svn_io_dir_make(pack_file_dir, APR_OS_DEFAULT, scratch_pool));
- SVN_ERR(svn_stream_open_writable(&manifest_stream, manifest_file_path,
- scratch_pool, scratch_pool));
+ SVN_ERR(svn_io_file_open(&manifest_file, manifest_file_path,
+ APR_WRITE | APR_BUFFERED | APR_CREATE | APR_EXCL,
+ APR_OS_DEFAULT, scratch_pool));
+ manifest_stream = svn_stream_from_aprfile2(manifest_file, TRUE,
+ scratch_pool);
+
/* revisions to handle. Special case: revision 0 */
start_rev = (svn_revnum_t) (shard * max_files_per_dir);
end_rev = (svn_revnum_t) ((shard + 1) * (max_files_per_dir) - 1);
@@ -1322,6 +1330,8 @@
/* flush the manifest file and update permissions */
SVN_ERR(svn_stream_close(manifest_stream));
+ SVN_ERR(svn_io_file_flush_to_disk(manifest_file, iterpool));
+ SVN_ERR(svn_io_file_close(manifest_file, iterpool));
SVN_ERR(svn_io_copy_perms(shard_path, pack_file_dir, iterpool));
svn_pool_destroy(iterpool);
