Around about 08/02/11 10:01, Neil Bird typed ...
Around about 08/02/11 09:03, Daniel Shahaf typed ...
Thanks. However, to clarify, I'm not specifically interested in the
"give us N revisions" form; I'm just interested in seeing a coherent
patch at the end, and wanted to ensure you haven't forgotten that in
depth of coding.
It's just going to take longer that I planned :)
OK, I tried back-porting (svn merge) actual trunk revisions to get a
working fix in place, but I stopped when I hit 7 of them (just for this one
change) and knew (by the volume of conflicts I was having to patch) that it
was a no-goer.
Even if I had got it compiling and working, there would have been so many
tertiary changes that it would have been difficult to prove that it wasn't
breaking more than it fixed (there are an awful lot of secondary & important
fixes in trunk relating to the initial re-working; I'd have almost
certainly missed one).
All that added to the fact that the revision numbers seem to have all
changed (the move from Tigris to Apache?), so even when the comments *do*
say that they're fixing blah from rev. xxx, the xxx is wrong.
So, I'm going back to my initial change. It's not terribly elegant, but
it is at least clean, does the job, and passes the tests.
Just to re-iterate, what I've done is change svn_io_open_unique_file3()
so that instead of just calling svn_io_open_uniquely_named() it is in fact a
verbatim copy of svn_io_open_uniquely_named(), except that it uses rand()
(after a fashion) to generate iterative temporary file names instead of
counting from 1.
This time, I believe I have catered for the case where RAND_MAX is only
32767 (my Windows at least) without being OS-specific. I have tested it on
Linux on both ext3 and onto a shared NTFS drive (which used to exhibit the
fault).
I have not compiled it for Windows (I don't have a Windows subversion
build environment), but I have compiled both sides of the ifdef I added.
I'll include the latest patch here, and put it on the bug entry for
posterity as well.
If you're happy with it, great, if not, well, so be it.
[[[
Fix issue #3719: prevent logarithmic slow down when checking out large
directories onto Windows NTFS
* subversion/libsvn_subr/io.c
svn_io_open_unique_file3: don't call svn_io_open_uniquely_named(), but
instead use a copy of that routine that uses rand() to get the next
potential free number instead if simply incrementing, minimising clashes on
repeated calls.
]]]
--
[neil@fnx ~]# rm -f .signature
[neil@fnx ~]# ls -l .signature
ls: .signature: No such file or directory
[neil@fnx ~]# exit
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1075316)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -466,9 +466,146 @@
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
- return svn_io_open_uniquely_named(file, temp_path,
- dirpath, "tempfile", ".tmp",
- delete_when, result_pool, scratch_pool);
+ const char *path;
+ unsigned int i;
+ struct temp_file_cleanup_s *baton = NULL;
+ char *filename = "tempfile";
+ char *suffix = ".tmp";
+ unsigned int randmax = RAND_MAX;
+#if RAND_MAX < 0x8000
+ /* RAND_MAX isn't big enough (e.g,. Windows has 0x7FFF), so use 4 times as many.
+ * Linux has RAND_MAX = 0x7FFFFFFF which is enough ...
+ */
+#define USING_4_TIMES_RAND_MAX
+ randmax *= 4;
+#endif
+
+ SVN_ERR_ASSERT(file || temp_path);
+
+ if (dirpath == NULL)
+ SVN_ERR(svn_io_temp_dir(&dirpath, scratch_pool));
+ if (filename == NULL)
+ filename = "tempfile";
+ if (suffix == NULL)
+ suffix = ".tmp";
+
+ path = svn_path_join(dirpath, filename, scratch_pool);
+
+ if (delete_when == svn_io_file_del_on_pool_cleanup)
+ {
+ baton = apr_palloc(result_pool, sizeof(*baton));
+
+ baton->pool = result_pool;
+ baton->name = NULL;
+
+ /* Because cleanups are run LIFO, we need to make sure to register
+ our cleanup before the apr_file_close cleanup:
+
+ On Windows, you can't remove an open file.
+ */
+ apr_pool_cleanup_register(result_pool, baton,
+ temp_file_plain_cleanup_handler,
+ temp_file_child_cleanup_handler);
+ }
+
+ for (i = 1; i <= randmax; i++)
+ {
+ const char *unique_name;
+ const char *unique_name_apr;
+ apr_file_t *try_file;
+ apr_status_t apr_err;
+ apr_int32_t flag = (APR_READ | APR_WRITE | APR_CREATE | APR_EXCL
+ | APR_BUFFERED | APR_BINARY);
+
+ if (delete_when == svn_io_file_del_on_close)
+ flag |= APR_DELONCLOSE;
+
+ /* Special case the first attempt -- if we can avoid having a
+ generated numeric portion at all, that's best. So first we
+ try with just the suffix; then future tries add a number
+ before the suffix. (A do-while loop could avoid the repeated
+ conditional, but it's not worth the clarity loss.) */
+ if (i == 1)
+ unique_name = apr_psprintf(scratch_pool, "%s%s", path, suffix);
+ else
+ {
+ unsigned int ir = rand();
+#ifdef USING_4_TIMES_RAND_MAX
+ ir += rand() + rand() + rand();
+#endif
+ unique_name = apr_psprintf(scratch_pool, "%s.%u%s", path, ir, suffix);
+ }
+
+ /* Hmmm. Ideally, we would append to a native-encoding buf
+ before starting iteration, then convert back to UTF-8 for
+ return. But I suppose that would make the appending code
+ sensitive to i18n in a way it shouldn't be... Oh well. */
+ SVN_ERR(cstring_from_utf8(&unique_name_apr, unique_name, scratch_pool));
+
+ apr_err = file_open(&try_file, unique_name_apr, flag,
+ APR_OS_DEFAULT, FALSE, result_pool);
+
+ if (APR_STATUS_IS_EEXIST(apr_err))
+ continue;
+ else if (apr_err)
+ {
+ /* On Win32, CreateFile fails with an "Access Denied" error
+ code, rather than "File Already Exists", if the colliding
+ name belongs to a directory. */
+ if (APR_STATUS_IS_EACCES(apr_err))
+ {
+ apr_finfo_t finfo;
+ apr_status_t apr_err_2 = apr_stat(&finfo, unique_name_apr,
+ APR_FINFO_TYPE, scratch_pool);
+
+ if (!apr_err_2 && finfo.filetype == APR_DIR)
+ continue;
+
+#if WIN32
+ apr_err_2 = APR_TO_OS_ERROR(apr_err);
+
+ if (apr_err_2 == ERROR_ACCESS_DENIED ||
+ apr_err_2 == ERROR_SHARING_VIOLATION)
+ {
+ /* The file is in use by another process or is hidden;
+ create a new name, but don't do this 99999 times in
+ case the folder is not writable */
+ i += 797;
+ continue;
+ }
+#endif
+
+ /* Else fall through and return the original error. */
+ }
+
+ if (file) *file = NULL;
+ if (temp_path) *temp_path = NULL;
+ return svn_error_wrap_apr(apr_err, _("Can't open '%s'"),
+ svn_path_local_style(unique_name,
+ scratch_pool));
+ }
+ else
+ {
+ if (delete_when == svn_io_file_del_on_pool_cleanup)
+ baton->name = apr_pstrdup(result_pool, unique_name_apr);
+
+ if (file)
+ *file = try_file;
+ else
+ apr_file_close(try_file);
+ if (temp_path)
+ *temp_path = apr_pstrdup(result_pool, unique_name);
+
+ return SVN_NO_ERROR;
+ }
+ }
+
+ if (file) *file = NULL;
+ if (temp_path) *temp_path = NULL;
+ return svn_error_createf(SVN_ERR_IO_UNIQUE_NAMES_EXHAUSTED,
+ NULL,
+ _("Unable to make name for '%s'"),
+ svn_path_local_style(path, scratch_pool));
}
svn_error_t *
@@ -1717,7 +1854,7 @@
}
}
-
+
/* XXX: We should check the incoming data for being of type binary. */
res = svn_stringbuf_create_ensure(res_initial_len, pool);
@@ -1833,11 +1970,11 @@
{
apr_finfo_t finfo;
- if (apr_stat(&finfo, path_apr, APR_FINFO_TYPE, pool) == APR_SUCCESS
+ if (apr_stat(&finfo, path_apr, APR_FINFO_TYPE, pool) == APR_SUCCESS
&& finfo.filetype == APR_REG)
{
WIN32_RETRY_LOOP(apr_err, apr_file_remove(path_apr, pool));
- }
+ }
}
/* Just return the delete error */
@@ -1904,10 +2041,10 @@
{
/* if the directory doesn't exist, our mission is accomplished */
if (ignore_enoent && APR_STATUS_IS_ENOENT(err->apr_err))
- {
- svn_error_clear(err);
- return SVN_NO_ERROR;
- }
+ {
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+ }
return err;
}
