Philip Martin <philip.martin_at_wandisco.com> writes:
> I like this patch better. It puts the flush into 2 extra places, 4 in
> total, in FSFS, and the corresponding 4 places in FSX. For 1.8 we would
> make the new API private: svn_stream__flush_to_disk_on_close.
>
> Index: subversion/include/svn_io.h
> ===================================================================
> --- subversion/include/svn_io.h (revision 1680818)
> +++ subversion/include/svn_io.h (working copy)
> @@ -1087,6 +1087,17 @@ svn_stream_from_aprfile2(apr_file_t *file,
> svn_boolean_t disown,
> apr_pool_t *pool);
>
> +/** Arrange for the APR file underlying @a stream to be flushed to
> + * disk before being closed. Returns SVN_ERR_STREAM_NOT_SUPPORTED for
> + * streams that cannot be flushed. This can be applied to streams
> + * created by svn_stream_from_aprfile2(), svn_stream_open_unique() and
> + * svn_stream_open_writable().
> + *
> + * @since New in 1.9.
> + */
> +svn_error_t *
> +svn_stream_flush_to_disk_on_close(svn_stream_t *stream);
Isn't this an abstraction leak?
This function accepts a generic stream as an argument, but makes assumptions
about what the stream should look like. You can't use it with an arbitrary
stream, because you have to be sure that it was opened using a function like
svn_stream_from_aprfile2() with some constraints, i.e., disown set to FALSE
and non-NULL file. As another example, you cannot use it with a file stream
wrapped with svn_stream_compressed() due to a close handler mismatch.
From my point of view, this means that svn_stream_flush_to_disk_on_close()
is only useful when you have full control over the stream that you're about to
wrap, probably, only in sequences like "open a stream and immediately install
a flush-on-close handler". Hence, an API like svn_stream_from_aprfile3() with
a flush_on_close argument would probably be a better choice.
[...]
>
> @@ -875,6 +876,7 @@ repack_stream_open(svn_stream_t **stream,
> pool),
> APR_WRITE | APR_CREATE, APR_OS_DEFAULT, pool));
> *stream = svn_stream_from_aprfile2(file, FALSE, pool);
> + SVN_ERR(svn_stream_flush_to_disk_on_close(*stream));
This is something that we were trying to avoid in the original patch, as this
makes the open() function decide if the file should receive a hardware flush
when closed or not. It's the caller who knows more about the details, not the
open() function. Personally I believe that this is an example of when explicit
is better than implicit — i.e., explicitly stating that we're going to do a
hardware flush and then close the file is better than juggling with streams
that keep the knowledge that they must flush the underlying file before
closing.
With that in mind, I see two possible options. Both of these options assume
that we handle the absence of flushing during packing and other operations
separately. From what I witness, the same issue can happen with, for example,
hotcopy due to how svn_io_copy_file() works. So, we could:
1) Leave this fix unchanged. I see the benefit of being explicit, but if we're
striving to get away from files in favor of streams, then the fix does not
really follow the pattern.
2) Introduce svn_stream_from_aprfile3(), svn_stream_open_unique2(), revert the
r1680819 fix and go again with these new functions. There could be a couple
of caveats in how we treat different combinations of svn_io_file_del_t and
disown and flush_on_close, but I suppose we could work them out.
Personally, I don't see anything fundamentally wrong with 1), because it makes
it easy to tell whether the flush is going to happen or not — without the need
to find the chunk of code that opened the stream. Is the patch hard to review?
Or is it just a gut feeling that we should be using streams here?
Regards,
Evgeny Kotkov
Received on 2015-05-22 17:15:16 CEST