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.
> 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.
>
> 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.
> 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.
Alan
>
> Bert
>
Received on 2008-10-05 22:05:23 CEST