On Fri, Aug 7, 2015 at 1:25 AM, <bert_at_qqmail.nl> wrote:
> [And now to the proper list]
>
>
>
> On the buildbots I see
>
> [[[
>
> ..\..\..\subversion\tests\libsvn_fs_x\fs-x-pack-test.c:873,
>
> ..\..\..\subversion\libsvn_fs_x\batch_fsync.c:386,
>
> ..\..\..\subversion\libsvn_fs_x\batch_fsync.c:343,
>
> ..\..\..\subversion\libsvn_subr\io.c:3515: (apr_err=720005)
>
> svn_tests: E720005: Can't open file 'E:\svn-local\tests\subversion\tests\libsvn_fs_x': Access is denied.
>
> FAIL: fs-x-pack-test 13: test batch fsync
>
> ]]]
>
> after this commit
>
>
>
> It looks like the batch fsync introduced in this patch is trying to open a
> directory as a file?
>
> That is not going to work on Windows, and probably on more platforms.
> Opening a directory requires other functions.
>
> Note that this code is called inside a 'SVN_ON_POSIX' block, which I would
> assume shouldn't be active on Windows.
>
Yup, that's where the bug is / was. It must be "#if SVN_ON_POSIX"
instead of "#ifdef SVN_ON_POSIX". Fixed in r1694669.
> Last time we discussed batch syncing you told me/us, that you would use
> the original handles to perform the flush... This code appears to tell a
> different story?
>
No, it doesn't. The svn_fs_x__batch_fsync_new_path call only
informs the batch that this file is actually new, implying that the
parent folder needs fsync-ing on POSIX. When used as intended,
the file itself has been opened through / in the batch container.
Only if it hadn't, the container would need to open it.
> Reopening files may, and commonly will cause a huge performance hit on
> Windows... because virusscanners are designed to delay those operations,
> until they are sure the file doesn’t contain a virus; especially if the
> file was just written.
>
The FSX code should not reopen files. The tests might - just to
cover as many cases as they can.
-- Stefan^2.
Received on 2015-08-07 15:02:39 CEST