You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2015/06/03 18:02:11 UTC
RE: svn commit: r1683378 - in /subversion/trunk/subversion/libsvn_fs_fs: pack.c revprops.c
> -----Original Message-----
> From: ivan@apache.org [mailto:ivan@apache.org]
> Sent: woensdag 3 juni 2015 17:49
> To: commits@subversion.apache.org
> Subject: svn commit: r1683378 - in /subversion/trunk/subversion/libsvn_fs_fs:
> pack.c revprops.c
>
> Author: ivan
> Date: Wed Jun 3 15:48:35 2015
> New Revision: 1683378
>
> URL: http://svn.apache.org/r1683378
> Log:
> Prevent a possible FSFS repository corruption with power or network disk
> failures during 'svnadmin pack'.
>
> * subversion/libsvn_fs_fs/pack.c
> (close_pack_context): Call svn_io_file_flush_to_disk() for pack file.
> (pack_phys_addressed): Use svn_io_file_open() to open pack and manifest
> file and call svn_io_file_flush_to_disk() before closing them.
>
> * subversion/libsvn_fs_fs/revprops.c
> (svn_fs_fs__copy_revprops): Use apr_file_t to write pack file and flush
> changes to disk before close.
> (svn_fs_fs__pack_revprops_shard): Use svn_io_file_open() to packed revision
> properties manifest file and call svn_io_file_flush_to_disk()
> before closing it.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/pack.c
> subversion/trunk/subversion/libsvn_fs_fs/revprops.c
>
<snip>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/revprops.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/revprop
> s.c?rev=1683378&r1=1683377&r2=1683378&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/libsvn_fs_fs/revprops.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/revprops.c Wed Jun 3 15:48:35
> 2015
> @@ -1168,7 +1168,6 @@ svn_fs_fs__copy_revprops(const char *pac
> apr_file_t *pack_file;
> svn_revnum_t rev;
> apr_pool_t *iterpool = svn_pool_create(scratch_pool);
> - svn_stream_t *stream;
>
> /* create empty data buffer and a write stream on top of it */
> svn_stringbuf_t *uncompressed
> @@ -1192,6 +1191,7 @@ svn_fs_fs__copy_revprops(const char *pac
> for (rev = start_rev; rev <= end_rev; rev++)
> {
> const char *path;
> + svn_stream_t *stream;
>
> svn_pool_clear(iterpool);
>
> @@ -1213,9 +1213,10 @@ svn_fs_fs__copy_revprops(const char *pac
> 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_write(stream, compressed->data, &compressed->len));
> - SVN_ERR(svn_stream_close(stream));
> + SVN_ERR(svn_io_file_write_full(pack_file, compressed->data, compressed-
> >len,
> + NULL, scratch_pool));
> + SVN_ERR(svn_io_file_flush_to_disk(pack_file, scratch_pool));
> + SVN_ERR(svn_io_file_close(pack_file, scratch_pool));
Minor optimization: you could have used iterpool as the shortest living scratch pool here, like how it is used in the last part of the patch.
>
> svn_pool_destroy(iterpool);
>
> @@ -1234,6 +1235,7 @@ svn_fs_fs__pack_revprops_shard(const cha
> 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;
> @@ -1250,8 +1252,12 @@ svn_fs_fs__pack_revprops_shard(const cha
>
> /* 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);
> @@ -1318,8 +1324,10 @@ svn_fs_fs__pack_revprops_shard(const cha
> compression_level, cancel_func,
> cancel_baton, iterpool));
>
> - /* flush the manifest file and update permissions */
> + /* flush the manifest file to disk 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);
>
Bert
Re: svn commit: r1683378 - in /subversion/trunk/subversion/libsvn_fs_fs:
pack.c revprops.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 3 June 2015 at 19:02, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: ivan@apache.org [mailto:ivan@apache.org]
>> Sent: woensdag 3 juni 2015 17:49
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1683378 - in /subversion/trunk/subversion/libsvn_fs_fs:
>> pack.c revprops.c
>>
>> Author: ivan
>> Date: Wed Jun 3 15:48:35 2015
>> New Revision: 1683378
>>
>> URL: http://svn.apache.org/r1683378
>> Log:
>> Prevent a possible FSFS repository corruption with power or network disk
>> failures during 'svnadmin pack'.
>>
>> * subversion/libsvn_fs_fs/pack.c
>> (close_pack_context): Call svn_io_file_flush_to_disk() for pack file.
>> (pack_phys_addressed): Use svn_io_file_open() to open pack and manifest
>> file and call svn_io_file_flush_to_disk() before closing them.
>>
>> * subversion/libsvn_fs_fs/revprops.c
>> (svn_fs_fs__copy_revprops): Use apr_file_t to write pack file and flush
>> changes to disk before close.
>> (svn_fs_fs__pack_revprops_shard): Use svn_io_file_open() to packed revision
>> properties manifest file and call svn_io_file_flush_to_disk()
>> before closing it.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_fs_fs/pack.c
>> subversion/trunk/subversion/libsvn_fs_fs/revprops.c
>>
>
> <snip>
>
>> Modified: subversion/trunk/subversion/libsvn_fs_fs/revprops.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/revprop
>> s.c?rev=1683378&r1=1683377&r2=1683378&view=diff
>> ================================================================
>> ==============
>> --- subversion/trunk/subversion/libsvn_fs_fs/revprops.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/revprops.c Wed Jun 3 15:48:35
>> 2015
>> @@ -1168,7 +1168,6 @@ svn_fs_fs__copy_revprops(const char *pac
>> apr_file_t *pack_file;
>> svn_revnum_t rev;
>> apr_pool_t *iterpool = svn_pool_create(scratch_pool);
>> - svn_stream_t *stream;
>>
>> /* create empty data buffer and a write stream on top of it */
>> svn_stringbuf_t *uncompressed
>> @@ -1192,6 +1191,7 @@ svn_fs_fs__copy_revprops(const char *pac
>> for (rev = start_rev; rev <= end_rev; rev++)
>> {
>> const char *path;
>> + svn_stream_t *stream;
>>
>> svn_pool_clear(iterpool);
>>
>> @@ -1213,9 +1213,10 @@ svn_fs_fs__copy_revprops(const char *pac
>> 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_write(stream, compressed->data, &compressed->len));
>> - SVN_ERR(svn_stream_close(stream));
>> + SVN_ERR(svn_io_file_write_full(pack_file, compressed->data, compressed-
>> >len,
>> + NULL, scratch_pool));
>> + SVN_ERR(svn_io_file_flush_to_disk(pack_file, scratch_pool));
>> + SVN_ERR(svn_io_file_close(pack_file, scratch_pool));
>
Hi Bert,
Thanks for review.
> Minor optimization: you could have used iterpool as the shortest living scratch pool here,
> like how it is used in the last part of the patch.
Personally I find using iterpool out of loop slightly confusing, so I
prefer don't use such practice unless it's necessary.
--
Ivan Zhakov