Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
From: "Bert Huijben" <bert@qqmail.nl>
To: <dev@subversion.apache.org>,
	"'Stefan Fuhrman'" <stefan2@apache.org>
References: <20150520113242.709CCAC0A57@hades.apache.org>
In-Reply-To: <20150520113242.709CCAC0A57@hades.apache.org>
Subject: RE: svn commit: r1680528 -
 /subversion/branches/fsx-1.10/subversion/libsvn_fs_x/transaction.c
Date: Wed, 20 May 2015 13:40:00 +0200
Message-ID: <026f01d092f1$b641e5d0$22c5b170$@qqmail.nl>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQKEAsMCnllgQQGAwReDrUaOvrNLlpwd3G8Q
Content-Language: nl


> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: woensdag 20 mei 2015 13:33
> To: commits@subversion.apache.org
> Subject: svn commit: r1680528 - /subversion/branches/fsx-
> 1.10/subversion/libsvn_fs_x/transaction.c
>=20
> Author: stefan2
> Date: Wed May 20 11:32:42 2015
> New Revision: 1680528
>=20
> URL: http://svn.apache.org/r1680528
> Log:
> On the fsx-1.10 branch:
> Don't FSYNC changes to FSX txn properties.  FSYNC future revprops
> only after their final contents has been written.  This is how we
> already treat the revision file data.
>=20
> * subversion/libsvn_fs_x/transaction.c
>   (set_txn_proplist): Don't FSYNC any txn prop change.
>   (commit_body): Flush the revprop contents here.
>=20
> Modified:
>     subversion/branches/fsx-1.10/subversion/libsvn_fs_x/transaction.c
>=20
> Modified: =
subversion/branches/fsx-1.10/subversion/libsvn_fs_x/transaction.c
> URL: http://svn.apache.org/viewvc/subversion/branches/fsx-
> =
1.10/subversion/libsvn_fs_x/transaction.c?rev=3D1680528&r1=3D1680527&r2=3D=
1680
> 528&view=3Ddiff
> =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- subversion/branches/fsx-1.10/subversion/libsvn_fs_x/transaction.c =
(original)
> +++ subversion/branches/fsx-1.10/subversion/libsvn_fs_x/transaction.c =
Wed
> May 20 11:32:42 2015
> @@ -1344,23 +1344,27 @@ set_txn_proplist(svn_fs_t *fs,
>                   svn_boolean_t final,
>                   apr_pool_t *scratch_pool)
>  {
> -  svn_stringbuf_t *buf;
>    svn_stream_t *stream;
> +  const char *temp_path;
>=20
> -  /* Write out the new file contents to BUF. */
> -  buf =3D svn_stringbuf_create_ensure(1024, scratch_pool);
> -  stream =3D svn_stream_from_stringbuf(buf, scratch_pool);
> +  /* Write the new contents into a temporary file. */
> +  SVN_ERR(svn_stream_open_unique(&stream, &temp_path,
> +                                 svn_fs_x__path_txn_dir(fs, txn_id,
> +                                                        =
scratch_pool),
> +                                 svn_io_file_del_none,
> +                                 scratch_pool, scratch_pool));
>    SVN_ERR(svn_hash_write2(props, stream, SVN_HASH_TERMINATOR,
> scratch_pool));
>    SVN_ERR(svn_stream_close(stream));
>=20
> -  /* Open the transaction properties file and write new contents to =
it. */
> -  SVN_ERR(svn_io_write_atomic((final
> +  /* Replace the old file with the new one. */
> +  SVN_ERR(svn_io_file_rename(temp_path,
> +                             (final
>                                 ? svn_fs_x__path_txn_props_final(fs, =
txn_id,
>                                                                  =
scratch_pool)
>                                 : svn_fs_x__path_txn_props(fs, txn_id,
>                                                            =
scratch_pool)),
> -                              buf->data, buf->len,
> -                              NULL /* copy_perms_path */, =
scratch_pool));
> +                              scratch_pool));
> +
>    return SVN_NO_ERROR;
>  }
>=20
> @@ -3316,7 +3320,7 @@ commit_body(void *baton,
>    const char *revprop_filename, *final_revprop;
>    svn_fs_x__id_t root_id, new_root_id;
>    svn_revnum_t old_rev, new_rev;
> -  apr_file_t *proto_file;
> +  apr_file_t *proto_file, *revprop_file;
>    void *proto_file_lockcookie;
>    apr_off_t initial_offset, changed_path_offset;
>    svn_fs_x__txn_id_t txn_id =3D svn_fs_x__txn_get_id(cb->txn);
> @@ -3437,6 +3441,12 @@ commit_body(void *baton,
>    /* Move the revprops file into place. */
>    SVN_ERR_ASSERT(! svn_fs_x__is_packed_revprop(cb->fs, new_rev));
>    SVN_ERR(write_final_revprop(&revprop_filename, cb->txn, txn_id, =
subpool));
> +
> +  SVN_ERR(svn_io_file_open(&revprop_file, revprop_filename, APR_READ,
> +                           APR_OS_DEFAULT, subpool));
> +  SVN_ERR(svn_io_file_flush_to_disk(revprop_file, subpool));
> +  SVN_ERR(svn_io_file_close(revprop_file, subpool));

Perhaps this works on some platforms, but it really depends on which =
layer the information is cached before flushing.

[[
  (commit_body): Flush the revprop contents here.
]]
Opening a file, flushing it and closing it isn't guaranteed to flush the =
information written when the file was opened earlier on.

I would be surprised if this actually performed a hardware flush on =
Windows, as generally things are attached to handles there... And if =
nothing is written to the handle, nor can be written, there is not much =
to flush to lower layers.

On top of that close, directly followed by open is generally slow on =
Windows, because virusscanners will access the file between these =
operations. Closing the file just once should perform much better and =
will avoid retry loops.

	Bert

> +
>    final_revprop =3D svn_fs_x__path_revprops(cb->fs, new_rev, =
subpool);
>    SVN_ERR(svn_fs_x__move_into_place(revprop_filename, final_revprop,
>                                      old_rev_filename, subpool));
>=20