On 22 May 2015 at 09:40, Branko Čibej <br...@wandisco.com> wrote:
> On 22.05.2015 00:09, Philip Martin wrote:
>> Ivan Zhakov <i...@visualsvn.com> writes:
>>
>>> We considered adding FLUSH_TO_DISK flag to svn_stream_from_apr_file(),
>>> but decided to commit simple fix for now:
>>> 1. The current FSFS code uses explicit flush and this commit makes
>>> code consistent with other parts
>>> 2. Adding flag to svn_stream_from_apr_file() will require revving API
>>> etc, while this fix should be backported to 1.8.x and 1.9.x
>
> The API argument only applies to 1.8.x, which would need a completely
> different fix; not to 1.9.x, because it's not released yet.
Yes, we can introduce new API in 1.9.x but this automatically this fix
1.9.0 release blocker, while I wanted to avoid blocking 1.9.0 due this
fix since it's not regressing.
> The benefit
> of revising the API (which I tentatively support, see [1] below)
> outweighs the pain of having to write a different fix for 1.8.x.
>
>>> 3. It's unclear how FLUSH_TO_DISK flag should interact with DISOWN=TRUE.
>>>
>>> Taking in account all above we decided to commit simple fix for now
>>> and possibly implement FLUSH_TO_DISK flag later applying it to all
>>> FSFS code where needed.
>> Is it simple? It's possible that r1680819 is more complicated than
>> adding flush to the the stream.
>>
>> Exactly what the API should look like is bit unclear. Is there any need
>> to support disown=TRUE and flush=TRUE? When would Subversion use it?
>> If we create svn_stream_from_aprfile3 that takes two booleans then
>> perhaps we change the return type to allow an error for the impossible
>> disown=TRUE and flush=TRUE.
>
> Yup. There's no reason to try to support mutually conflicting arguments.
But how svn_stream_from_aprfile3() should handle disown=TRUE and
flush=TRUE: assertion, disown=TRUE as priority and ignore flush=TRUE?
I don't say we cannot find good solution for this, I just wanted to
make simple fix targeted for 1.8.x and 1.9.x and then improve all FSFS
regarding flushes.
>
>> All the flushing code goes in libsvn_subr
>
> Just one nit here: the flushing code would indeed be localized in
> libsvn_subr, however, the API change would propagate to some 100 places
> in the code [1], including lots of places in libsvn_fs_fs. That's unless
> we decide /not/ to deprecate svn_stream_from_aprfile2 in the 1.9.x
> series and use it in parallel with the prospective svn_stream_from_aprfile3.
>
Yep, revving svn_stream_from_aprfile2() would introduce changing all
callers. But I agree that we can keep svn_stream_from_aprfile2()
without deprecating.
> 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.
>
There are two kinds of flushes:
1) flush all data buffered in stream implementation
2) ensure that data written to disk hardware
Extending stream API with svn_stream_flush() with (1) semantic seems
to be natural extension, but introducing (2) would be leak of
abstraction.
>> and the code change in FSFS is really small, something like:
>>
>> Index: subversion/libsvn_fs_fs/revprops.c
>> ===================================================================
>> --- subversion/libsvn_fs_fs/revprops.c (revision 1680818)
>> +++ subversion/libsvn_fs_fs/revprops.c (working copy)
>> @@ -874,7 +874,7 @@
>> new_filename->data,
>> pool),
>> APR_WRITE | APR_CREATE, APR_OS_DEFAULT, pool));
>> - *stream = svn_stream_from_aprfile2(file, FALSE, pool);
>> + SVN_ERR(svn_stream_from_aprfile3(stream, file, FALSE, TRUE, pool));
>>
>> return SVN_NO_ERROR;
>> }
>> @@ -1205,7 +1205,8 @@
>> 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);
>> + SVN_ERR(svn_stream_from_aprfile3(&stream, pack_file, FALSE, TRUE,
>> + scratch_pool));
>> SVN_ERR(svn_stream_write(stream, compressed->data, &compressed->len));
>> SVN_ERR(svn_stream_close(stream));
>>
>> A patch like that is possibly easier to review than r1680819.
>
> Quite a bit easier, yes.
>
Yes, this patch looks easier to review, the only problem that it's
incomplete. I'm attaching minimal working patch with
svn_stream_from_file3() against trunk@r1680818.
--
Ivan Zhakov
Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h (revision 1679771)
+++ subversion/include/svn_io.h (working copy)
@@ -1087,6 +1087,12 @@
svn_boolean_t disown,
apr_pool_t *pool);
+svn_stream_t *
+svn_stream_from_aprfile3(apr_file_t *file,
+ svn_boolean_t disown,
+ svn_boolean_t flush_on_close,
+ apr_pool_t *pool);
+
/** Similar to svn_stream_from_aprfile2(), except that the file will
* always be disowned.
*
Index: subversion/libsvn_fs_fs/revprops.c
===================================================================
--- subversion/libsvn_fs_fs/revprops.c (revision 1679771)
+++ subversion/libsvn_fs_fs/revprops.c (working copy)
@@ -657,14 +657,16 @@
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_aprfile3(file, FALSE, TRUE, pool);
SVN_ERR(svn_hash_write2(proplist, stream, SVN_HASH_TERMINATOR, pool));
SVN_ERR(svn_stream_close(stream));
@@ -879,7 +881,7 @@
new_filename->data,
pool),
APR_WRITE | APR_CREATE, APR_OS_DEFAULT, pool));
- *stream = svn_stream_from_aprfile2(file, FALSE, pool);
+ *stream = svn_stream_from_aprfile3(file, FALSE, TRUE, pool);
return SVN_NO_ERROR;
}
@@ -903,6 +905,7 @@
packed_revprops_t *revprops;
apr_int64_t generation = 0;
svn_stream_t *stream;
+ apr_file_t *file;
svn_stringbuf_t *serialized;
apr_off_t new_total_size;
int changed_index;
@@ -933,8 +936,10 @@
*final_path = svn_dirent_join(revprops->folder, revprops->filename,
pool);
- SVN_ERR(svn_stream_open_unique(&stream, tmp_path, revprops->folder,
- svn_io_file_del_none, pool, pool));
+ SVN_ERR(svn_io_open_unique_file3(&file, tmp_path, revprops->folder,
+ svn_io_file_del_none, pool, pool));
+ stream = svn_stream_from_aprfile3(file, FALSE, TRUE, pool);
+
SVN_ERR(repack_revprops(fs, revprops, 0, revprops->sizes->nelts,
changed_index, serialized, new_total_size,
stream, pool));
@@ -1016,8 +1021,9 @@
/* write the new manifest */
*final_path = svn_dirent_join(revprops->folder, PATH_MANIFEST, pool);
- SVN_ERR(svn_stream_open_unique(&stream, tmp_path, revprops->folder,
- svn_io_file_del_none, pool, pool));
+ SVN_ERR(svn_io_open_unique_file3(&file, tmp_path, revprops->folder,
+ svn_io_file_del_none, pool, pool));
+ stream = svn_stream_from_aprfile3(file, FALSE, TRUE, pool);
for (i = 0; i < revprops->manifest->nelts; ++i)
{
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1679771)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -808,6 +808,7 @@
struct baton_apr {
apr_file_t *file;
apr_pool_t *pool;
+ svn_boolean_t flush_on_close;
};
/* svn_stream_mark_t for streams backed by APR files. */
@@ -1031,10 +1032,10 @@
return SVN_NO_ERROR;
}
-
svn_stream_t *
-svn_stream_from_aprfile2(apr_file_t *file,
+svn_stream_from_aprfile3(apr_file_t *file,
svn_boolean_t disown,
+ svn_boolean_t flush_on_close,
apr_pool_t *pool)
{
struct baton_apr *baton;
@@ -1046,6 +1047,7 @@
baton = apr_palloc(pool, sizeof(*baton));
baton->file = file;
baton->pool = pool;
+ baton->flush_on_close = flush_on_close;
stream = svn_stream_create(baton, pool);
svn_stream_set_read2(stream, read_handler_apr, read_full_handler_apr);
svn_stream_set_write(stream, write_handler_apr);
@@ -1056,12 +1058,22 @@
svn_stream__set_is_buffered(stream, is_buffered_handler_apr);
stream->file = file;
- if (! disown)
- svn_stream_set_close(stream, close_handler_apr);
+ /* ## TODO: How to handle DISOWN = TRUE and FLUSH_ON_CLOSE=TRUE? */
+ if (!disown)
+ svn_stream_set_close(stream, close_handler_apr);
return stream;
}
+
+svn_stream_t *
+svn_stream_from_aprfile2(apr_file_t *file,
+ svn_boolean_t disown,
+ apr_pool_t *pool)
+{
+ return svn_stream_from_aprfile3(file, disown, FALSE, pool);
+}
+
apr_file_t *
svn_stream__aprfile(svn_stream_t *stream)
{
