> From: Alan Wood [mailto:alan.wood_at_clear.net.nz]
> Sent: zondag 5 oktober 2008 22:04
> To: Bert Huijben; dev_at_subversion.tigris.org
> Subject: RE: [PATCH] Working copy on Samba share
>
> On 5 Oct 2008 at 14:56, Bert Huijben wrote:
>
> > > -----Original Message-----
> > > From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
> > > Sent: zondag 5 oktober 2008 13:50
> > > To: alan.wood_at_clear.net.nz
> > > Cc: dev_at_subversion.tigris.org
> > > Subject: Re: [PATCH] Working copy on Samba share
> > >
> > > Ping, anyone has an opinion on this patch?
> > >
> > > Alan: using empty lines between paragraphs helps people read what you
> > > write (increasing the chance to get a response).
> >
> > -1 on applying this patch without further review per case and a stricter
set
> > of errors to retry on.
>
> Review is the name of the game, thanks for looking into it.
Thanks.
>
> > This patch just hides problems without resolving them.. and makes
subversion
> > error later without good reason.
> > If a file is locked for a good reason that should stop the subversion
> > command, this patch will just delay subversion for several seconds.
> >
>
> It would be nice not to but not having the ability to change all parts of
the system, in this case the Samba client code, means we end up with code in
subversion that is just trying to make life a little nicer for the user.
>
I agree. I don't disagree with that we should retry in several occasions,
but the current code retries too many times and on too many error
conditions. I would like to see that fixed first
> >
> > Just retrying on every error condition is not a solution.
> > (Read http://blogs.msdn.com/oldnewthing/archive/2005/11/07/489807.aspx
for
> > some of the reasoning)
>
> Interesting link, I could add some of my own to that one.
:-)
>
> >
> > Just last week we got a report from someone who found out a case in
which we
> > retry file creation 100 times over 15 seconds to just create a file with
a
> > unique name. In this case the filename happens to be the same as an
existing
> > directory.
> >
> > The retry loop in svn_io_file_open doesn't test for this error condition
and
> > retries 100 times in +- 15 seconds.
> >
> > The real solution in this case is of course to give a new name in the
outer
> > loop instead of retrying the same not-unique name a hundred times.
> > This takes a few milliseconds instead of the 15 seconds waiting for
someone
> > to delete the directory before checking the next name, that potentially
has
> > the same issue.
> >
> >
> > Just retrying is not a solution. Adding more retries without looking for
the
> > real cause is just asking for more problems. Another reply on the
original
> > thread suggested that it is probably an issue in specific samba
versions. (I
> > think it was somebody from the TortoiseSVN project)
>
> It was, but I had already tested with the latest version of Samba that
worked on RHEL, and I got no reply when asking for an actual version number.
>
> >
> > I'm working on reviewing the existing retry loops as part of looking why
> > Windows fs access is called slow on this list.
>
> I would vote for reducing the WIN32_RETRY_LOOP loop by a factor of 10, but
I haven't seen it being used except under real error conditions.
Reducing the number of retries is an option and limiting the amount of time
spent on retrying could be another option. (Think of network delays to a
server).
> > If there are specific cases that need a specific retry that I should
look
> > at, please let me know.
>
> The one I found was a very rare case so you are right to question it. It
was a case of two processes accessing the same working copy at the same
time, which I would say is bad news anyway. I went after it because it had
the same error response as the problem that I really want to solve ( the
rename error ) so I thought they were related.
>
> Actually further investigation has shown that this patch does not solve
the main rename problem, I now have another patch for that, should be
available for the list on Wednesday, after a bit more testing.
>
> If I really wanted to place this issue in the correct camp, I would say
that the Samba server code that I was using should not have returned an
error ( it was "delete in progress") and also the Samba client code (
Windows XP ) should not have changed an error code from one that could
sensible be retried to one that would normally not be retried ( access
denied ). I suspect that Microsoft smb server never uses this error code and
thus it is not a problem to them.
>
> Also, in the case that I saw, the error was only temporary and with my
testing the error was cleared in a single retry. I would be happy if the
retry loop macro changed to only loop twice for the case of the apr_stat()
and apr_file_attrs_set() calls that I was investigating.
>
> If this patch does get rejected then I would still like to see a comment
added to the code warning that these two apr_ routines can give misleading
error returns on Samba systems, just so we have an easy record of it.
>
I'll see what I can do
> [[[
> Make working copies ( and other file transactions ) more robust on Samba.
> * subversion/libsvn_subr/io.c
> Wrap all calls to apr_stat() and apr_file_attrs_set() in
WIN32_RETRY_LOOP()
> (io_check_path)(svn_io_open_unique_file2)(svn_io_filesizes_different_p)
> (svn_io_set_file_read_only)(svn_io_set_file_read_write)
> (svn_io_detect_mimetype2)(svn_io_stat)(dir_make)
> Patch By: Alan Wood <Alan.Wood_at_clear.net.nz>
> ]]]
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- io.c (revision 32851)
> +++ io.c (working copy)
> @@ -141,7 +141,7 @@
>
> flags = resolve_symlinks ? APR_FINFO_MIN : (APR_FINFO_MIN |
APR_FINFO_LINK);
> apr_err = apr_stat(&finfo, path_apr, flags, pool);
> -
> + WIN32_RETRY_LOOP(apr_err,apr_stat(&finfo, path_apr, flags, pool));
> if (APR_STATUS_IS_ENOENT(apr_err))
> *kind = svn_node_none;
> else if (APR_STATUS_IS_ENOTDIR(apr_err))
> @@ -382,7 +382,7 @@
> continue;
> else if (apr_err)
> {
> - /* On Win32, CreateFile failswith an "Access Denied" error
> + /* 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))
> @@ -1075,6 +1075,8 @@
>
> /* Stat both files */
> status = apr_stat(&finfo1, file1_apr, APR_FINFO_MIN, pool);
> + WIN32_RETRY_LOOP(status,apr_stat(&finfo1, file1_apr,
> + APR_FINFO_MIN, pool));
> if (status)
> {
> /* If we got an error stat'ing a file, it could be because the
> @@ -1086,6 +1088,8 @@
> }
>
> status = apr_stat(&finfo2, file2_apr, APR_FINFO_MIN, pool);
> + WIN32_RETRY_LOOP(status,apr_stat(&finfo2, file2_apr,
> + APR_FINFO_MIN, pool));
> if (status)
> {
> /* See previous comment. */
> @@ -1377,7 +1381,9 @@
> APR_FILE_ATTR_READONLY,
> APR_FILE_ATTR_READONLY,
> pool);
> -
> + WIN32_RETRY_LOOP(status,apr_file_attrs_set(path_apr,
> + APR_FILE_ATTR_READONLY,
> + APR_FILE_ATTR_READONLY,pool));
For the record:
When I test this from Vista to my FreeBSD samba server I see the mask
changing from -rw-rw-r-- to -r--r--r--.
> if (status && status != APR_ENOTIMPL)
> if (!ignore_enoent || !APR_STATUS_IS_ENOENT(status))
> return svn_error_wrap_apr(status,
> @@ -1409,7 +1415,9 @@
> 0,
> APR_FILE_ATTR_READONLY,
> pool);
> -
> + WIN32_RETRY_LOOP(status,apr_file_attrs_set(path_apr,0,
> + APR_FILE_ATTR_READONLY,
> +
pool));
This changes the mask from -r--r--r--. To -rw-r--r--.
With a non-read-only -> read only -> non-read-only you will lose your group
writable bit.
> if (status && status != APR_ENOTIMPL)
> if (!ignore_enoent || !APR_STATUS_IS_ENOENT(status))
> return svn_error_wrap_apr(status,
> @@ -2572,7 +2580,7 @@
> if (amt_read > 0)
> {
> apr_size_t i;
> - int binary_count = 0;
> + apr_size_t binary_count = 0;
>
> /* Run through the data we've read, counting the 'binary-ish'
> bytes. HINT: If we see a 0x00 byte, we'll set our count to its
> @@ -2828,6 +2836,7 @@
> SVN_ERR(svn_path_cstring_from_utf8(&fname_apr, fname, pool));
>
> status = apr_stat(finfo, fname_apr, wanted, pool);
> + WIN32_RETRY_LOOP(status,apr_stat(finfo, fname_apr, wanted, pool));
> if (status)
> return svn_error_wrap_apr(status, _("Can't stat '%s'"),
> svn_path_local_style(fname, pool));
> @@ -2946,7 +2955,11 @@
> APR_FILE_ATTR_HIDDEN,
> APR_FILE_ATTR_HIDDEN,
> pool);
> - if (status)
> + WIN32_RETRY_LOOP(status,
> + apr_file_attrs_set(path_apr,
> + APR_FILE_ATTR_HIDDEN,
> + APR_FILE_ATTR_HIDDEN,pool));
Setting a file to hidden is ignored on my samba version as the hidden status
can't be expressed in the umask.
> + if (status)
> return svn_error_wrap_apr(status, _("Can't hide directory '%s'"),
> svn_path_local_style(path, pool));
> }
> @@ -2959,6 +2972,7 @@
> /* Per our contract, don't do error-checking. Some filesystems
> * don't support the sgid bit, and that's okay. */
> status = apr_stat(&finfo, path_apr, APR_FINFO_PROT, pool);
> + WIN32_RETRY_LOOP(status,apr_stat(&finfo, path_apr, APR_FINFO_PROT,
pool));
>
> if (!status)
> apr_file_perms_set(path_apr, finfo.protection | APR_GSETID);
This code path is disabled for WIN32 since r33178 (after you created your
initial patch). It did a lot of disk IO in an attempt to set the sgid (g+s
bit), but setting the bit was a no-op.
Thanks
Bert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-05 22:45:17 CEST