Re: svn commit: r1680819 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c

On 22 May 2015 at 11:50, Philip Martin <philip.martin_at_wandisco.com> wrote:
> Branko Čibej <brane_at_wandisco.com> writes:
>
>> FWIW, I think in this case revving the API without deprecating the old
>> one is justified. Another option would be to invent a different API name
>> for the flushing stream, e.g., svn_stream_from_aprfile_flushed or some
>> such. This way we'd also avoid the dilemma about disown by simply not
>> having that flag in the new function.
>
> Are there bits missing from r1680819? svn_fs_fs__pack_revprops_shard()
> still uses a stream to write the manifest file and it has not been
> modified to flush.
I intentionally kept packing code unchanged: it seems that now we
don't use hardware flush when packing repository: so we need apply fix
to all pack code, which is separate issue IMO.

> write_non_packed_revprop() also uses a stream that is not flushed.
What do you mean? Here is hunk changing write_non_packed_revprop() to
flush file before close.
[[
@@ -652,17 +652,23 @@ write_non_packed_revprop(const char **fi
                          apr_hash_t *proplist,
                          apr_pool_t *pool)
 {
+ apr_file_t *file;
   svn_stream_t *stream;
   *final_path = svn_fs_fs__path_revprops(fs, rev, pool);

   /* ### do we have a directory sitting around already? we really shouldn't
      ### have to get the dirname here. */
- SVN_ERR(svn_stream_open_unique(&stream, tmp_path,
- svn_dirent_dirname(*final_path, pool),
- svn_io_file_del_none, pool, pool));
+ SVN_ERR(svn_io_open_unique_file3(&file, tmp_path,
+ svn_dirent_dirname(*final_path, pool),
+ svn_io_file_del_none, pool, pool));
+ stream = svn_stream_from_aprfile2(file, TRUE, pool);
   SVN_ERR(svn_hash_write2(proplist, stream, SVN_HASH_TERMINATOR, pool));
   SVN_ERR(svn_stream_close(stream));

+ /* Flush temporary file to disk and close it. */
+ SVN_ERR(svn_io_file_flush_to_disk(file, pool));
+ SVN_ERR(svn_io_file_close(file, pool));
+
   return SVN_NO_ERROR;
 }

@@ -745,7 +751,7 @@ serialize_revprops_header(svn_stream_t *
   return SVN_NO_ERROR;
 }
]]]

> We could create a "flushing stream" that gets chained:
>
> struct baton_flush_t {
> svn_stream_t *stream;
> apr_pool_t *pool;
> };
>
> static svn_error_t *
> close_handler_flush(void *baton)
> {
> struct baton_flush_t *b = baton;
>
> SVN_ERR(svn_io_file_flush_to_disk(svn_stream__aprfile(b->stream), b->pool));
> SVN_ERR(svn_stream_close(b->stream));
> return SVN_NO_ERROR;
> }
>
> svn_error_t *
> svn_stream_flush(svn_stream_t **flush,
> svn_stream_t *stream,
> apr_pool_t *pool)
I think that using svn_stream_flush() as wrapper
svn_io_file_flush_to_disk() is very confusing since it will be mixed
up with svn_io_file_flush().

[...]

>
> Another approach would be to have a function to enable flushing directly
> on the stream created by svn_stream_from_aprfile2() without creating a
> new stream. This is probably the smallest/simplest patch. After
> reverting r1680819:
>
[...]

This approach introduce dependency what kind of stream returned from
svn_stream_open_unique() which reduce advantage of using abstract
stream_t object.

Given all feedback I suggest do the following:
1. Commit patch introducing svn_stream_from_aprfile3() with
FLUSH_ON_CLOSE argument, without deprecating
svn_stream_from_aprfile2().
2. I revert r1680819
3. Switch revprop change code to use svn_stream_from_aprfile3() with
FLUSH_ON_CLOSE=TRUE.

Then I'll nominate alternative revisions from (1) and (3) to 1.9.x.

Does it work for you?

-- 
Ivan Zhakov