You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ramkumar Ramachandra <ar...@gmail.com> on 2010/10/01 16:07:55 UTC
[Merge request] Merge r985477 from performance branch
Hi,
I would like to get r985477 merged into trunk. I've applied and used
it successfully and checked that all tests pass.
Warning: I have no background knowledge. I'm just reviewing the patch
as-it-is, because Stefan asked me to.
> [[[
> r985477 | stefan2 | 2010-08-14 18:02:04 +0530 (Sat, 14 Aug 2010) | 9 lines
>
> Eliminate all file system access in get_default_file_perms() for all but the first execution.
>
> The only downside is that we don't detect FS permission changes made while the
> (client) process runs. Because such changes would cause race conditions and file I/O
> errors anyway, we are not make things worse by omitting those tests.
>
> * subversion/libsvn_subr/io.c
> (get_default_file_perms): store result in a singleton in the first run and bypass
> the FS access in all later runs
I looked at this after reading the patch, and I must admit that I
didn't get the idea from the log message: how about using "static
variable" instead of "singleton".
> ]]]
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 985476)
> +++ subversion/libsvn_subr/io.c (revision 985477)
> @@ -1297,30 +1297,47 @@ reown_file(const char *path,
> static svn_error_t *
> get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
> {
> - apr_finfo_t finfo;
> - apr_file_t *fd;
> + /* the default permissions as read from the temp folder */
> + static apr_fileperms_t default_perms = 0;
>From the APR documentation, this is just an int32. Hm, you've got rid
of the file description and finfo -- let's see how this works out.
> - /* Get the perms for a newly created file to find out what bits
> - should be set.
> + /* Technically, this "racy": Multiple threads may use enter here and
> + try to figure out the default permisission concurrently. That's fine
> + since they will end up with the same results. Even more technical,
> + apr_fileperms_t is an atomic type on 32+ bit machines.
> + */
> + if (default_perms == 0)
> + {
> + apr_finfo_t finfo;
> + apr_file_t *fd;
Ah, so you didn't remove them.
> - NOTE: normally del_on_close can be problematic because APR might
> - delete the file if we spawned any child processes. In this case,
> - the lifetime of this file handle is about 3 lines of code, so
> - we can safely use del_on_close here.
> + /* Get the perms for a newly created file to find out what bits
> + should be set.
>
> - NOTE: not so fast, shorty. if some other thread forks off a child,
> - then the APR cleanups run, and the file will disappear. sigh.
> + NOTE: normally del_on_close can be problematic because APR might
> + delete the file if we spawned any child processes. In this case,
> + the lifetime of this file handle is about 3 lines of code, so
> + we can safely use del_on_close here.
Bogus diff due to re-indent. The only real addition is "Get the perms
for a newly created file to find out what bits should be set".
> - Using svn_io_open_uniquely_named() here because other tempfile
> - creation functions tweak the permission bits of files they create.
> - */
> - SVN_ERR(svn_io_open_uniquely_named(&fd, NULL, NULL, "svn-tempfile", ".tmp",
> - svn_io_file_del_on_pool_cleanup,
> - scratch_pool, scratch_pool));
> - SVN_ERR(svn_io_file_info_get(&finfo, APR_FINFO_PROT, fd, scratch_pool));
> - SVN_ERR(svn_io_file_close(fd, scratch_pool));
> + NOTE: not so fast, shorty. if some other thread forks off a child,
> + then the APR cleanups run, and the file will disappear. sigh.
Ok. You've moved this comment down to say why the del_on_close idea in
the previous comment is a bad idea.
> + *perms = finfo.protection;
> + default_perms = finfo.protection;
Very simple patch. Basically, if the permissions were found in the
last function call, simply return the found value, using a static
variable for remembering it. Otherwise, re-calculate permissions.
-- Ram
Re: [Merge request] Merge r985477 from performance branch
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Stefan,
Stefan Sperling writes:
> > Can I get an explicit +1 to commit this? I just want to get as many of
> > Stefan's changes merged in quickly so that there's enough time before
> > the 1.7 release to test them.
> >
> > Technical detail: How do I merge?
>
> cd svn-trunk-working-copy
> svn merge -c r985477 ^/subversion/branches/performance
> # edit if necessary
> svn commit
>
> # Take note of committed revision, e.g. rN.
> # I'd recommend doing the following to avoid a cyclic (aka "reflective") merge.
> # The commit just made should not bounce back to the performance branch
> # when someone runs svn merge ^/subversion/trunk on that branch
>
> cd svn-performance-branch-working-copy
> svn merge --record-only -rN ^/subversion/trunk
> svn commit -m "Block rN from being merged into the performance branch to
> avoid a cyclic merge"
>
> If you don't understand why the record-only merge is necessary, see
> http://mail-archives.apache.org/mod_mbox/subversion-dev/201004.mbox/%3C20100406152724.GM19641@noel.stsp.name%3E
Thanks for the elaborate explanation! :)
> > I want to make some modifications to
> > this patch before committing
>
> What kinds of modifications do you want to make?
> You'll need a +1 for those, too. Please mail the diff which shows the
> results of the merge plus your modifications.
Just minor indentation/ comment changes (some mentioned in my
review). Nothing functional.
> > but I want to preserve authorship.
>
> r985477 lists stefan2 as author. There is no concept of authorship in svn
> beyond that. You'll be listed as author of the merge commit, and your log
> message should say which change you're merging (the mergeinfo will also
> say it, but it doesn't hurt to mention it in the commit message anyway).
Ok, so when the build on trunk breaks, I'll be blamed, not stefan2- I
think I can live with that. I'll also need to merge 5~10 more commits:
will do shortly.
My objective is to get whatever I think I can evaluate into trunk as
quickly as possible so that they're well-tested and shipped with
Subversion 1.7. I'm inexperienced, and will only attempt to evaluate a
subset of patches that directly affect svnrdump: I would request
someone else to pick up the other patches.
-- Ram
Re: [Merge request] Merge r985477 from performance branch
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Oct 03, 2010 at 07:02:01PM +0530, Ramkumar Ramachandra wrote:
> Hi Julian,
>
> Julian Foad writes:
> > Looks good to me.
> >
> > I wondered if it is safe in a long-running Subversion process, like
> > TortoiseSvn or a Linux equivalent.
> >
> > It seems to me that it won't really matter much in practice. If someone
> > changes their umask and finds that Subversion carries on creating files
> > with the 'old' permissions that were in effect when Subversion was
> > started... that's fine, as far as I'm concerned.
>
> Can I get an explicit +1 to commit this? I just want to get as many of
> Stefan's changes merged in quickly so that there's enough time before
> the 1.7 release to test them.
>
> Technical detail: How do I merge?
cd svn-trunk-working-copy
svn merge -c r985477 ^/subversion/branches/performance
# edit if necessary
svn commit
# Take note of committed revision, e.g. rN.
# I'd recommend doing the following to avoid a cyclic (aka "reflective") merge.
# The commit just made should not bounce back to the performance branch
# when someone runs svn merge ^/subversion/trunk on that branch
cd svn-performance-branch-working-copy
svn merge --record-only -rN ^/subversion/trunk
svn commit -m "Block rN from being merged into the performance branch to
avoid a cyclic merge"
If you don't understand why the record-only merge is necessary, see
http://mail-archives.apache.org/mod_mbox/subversion-dev/201004.mbox/%3C20100406152724.GM19641@noel.stsp.name%3E
> I want to make some modifications to
> this patch before committing
What kinds of modifications do you want to make?
You'll need a +1 for those, too. Please mail the diff which shows the
results of the merge plus your modifications.
> but I want to preserve authorship.
r985477 lists stefan2 as author. There is no concept of authorship in svn
beyond that. You'll be listed as author of the merge commit, and your log
message should say which change you're merging (the mergeinfo will also
say it, but it doesn't hurt to mention it in the commit message anyway).
Thanks,
Stefan
Re: [Merge request] Merge r985477 from performance branch
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Julian,
Julian Foad writes:
> Hi Ram. I wasn't comfortable with giving a +1 for this change just
> then, but now I've satisfied myself. The only potential negative impact
> I can imagine is if a user has a very long-running instance of
> Subversion and is accustomed to Subversion tracking changes of umask.
> To such a user, this might be seen as a regression, but the impact on
> such a user is low. In all other respects, determining the permissions
> just once per execution is more correct as well as more efficient.
>
> +1 to merge, with minor tweaks if you wish.
Thanks. Committed in r1004286.
(Sorry for the delay: classes and labs)
-- Ram
Re: [Merge request] Merge r985477 from performance branch
Posted by Julian Foad <ju...@wandisco.com>.
On Sun, 2010-10-03, Ramkumar Ramachandra wrote:
> Hi Julian,
>
> Julian Foad writes:
> > Looks good to me.
> >
> > I wondered if it is safe in a long-running Subversion process, like
> > TortoiseSvn or a Linux equivalent.
> >
> > It seems to me that it won't really matter much in practice. If someone
> > changes their umask and finds that Subversion carries on creating files
> > with the 'old' permissions that were in effect when Subversion was
> > started... that's fine, as far as I'm concerned.
>
> Can I get an explicit +1 to commit this? I just want to get as many of
> Stefan's changes merged in quickly so that there's enough time before
> the 1.7 release to test them.
Hi Ram. I wasn't comfortable with giving a +1 for this change just
then, but now I've satisfied myself. The only potential negative impact
I can imagine is if a user has a very long-running instance of
Subversion and is accustomed to Subversion tracking changes of umask.
To such a user, this might be seen as a regression, but the impact on
such a user is low. In all other respects, determining the permissions
just once per execution is more correct as well as more efficient.
+1 to merge, with minor tweaks if you wish.
- Julian
Re: [Merge request] Merge r985477 from performance branch
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Julian,
Julian Foad writes:
> Looks good to me.
>
> I wondered if it is safe in a long-running Subversion process, like
> TortoiseSvn or a Linux equivalent.
>
> It seems to me that it won't really matter much in practice. If someone
> changes their umask and finds that Subversion carries on creating files
> with the 'old' permissions that were in effect when Subversion was
> started... that's fine, as far as I'm concerned.
Can I get an explicit +1 to commit this? I just want to get as many of
Stefan's changes merged in quickly so that there's enough time before
the 1.7 release to test them.
Technical detail: How do I merge? I want to make some modifications to
this patch before committing, but I want to preserve authorship.
-- Ram
Re: [Merge request] Merge r985477 from performance branch
Posted by Julian Foad <ju...@wandisco.com>.
Ramkumar Ramachandra wrote:
> I would like to get r985477 merged into trunk. I've applied and used
> it successfully and checked that all tests pass.
>
> Warning: I have no background knowledge. I'm just reviewing the patch
> as-it-is, because Stefan asked me to.
>
> > [[[
> > r985477 | stefan2 | 2010-08-14 18:02:04 +0530 (Sat, 14 Aug 2010) | 9 lines
> >
> > Eliminate all file system access in get_default_file_perms() for all but the first execution.
> >
> > The only downside is that we don't detect FS permission changes made while the
> > (client) process runs. Because such changes would cause race conditions and file I/O
> > errors anyway, we are not make things worse by omitting those tests.
Looks good to me.
I wondered if it is safe in a long-running Subversion process, like
TortoiseSvn or a Linux equivalent.
It seems to me that it won't really matter much in practice. If someone
changes their umask and finds that Subversion carries on creating files
with the 'old' permissions that were in effect when Subversion was
started... that's fine, as far as I'm concerned.
To make it easier for others to see, here's the patch, ignoring
indentation changes:
[[[
$ svn diff -x -upw -c985477 http://svn.apache.org/repos/asf/subversion
Index: branches/performance/subversion/libsvn_subr/io.c
===================================================================
--- branches/performance/subversion/libsvn_subr/io.c (revision 985476)
+++ branches/performance/subversion/libsvn_subr/io.c (revision 985477)
@@ -1297,6 +1297,16 @@
static svn_error_t *
get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
{
+ /* the default permissions as read from the temp folder */
+ static apr_fileperms_t default_perms = 0;
+
+ /* Technically, this "racy": Multiple threads may use enter here and
+ try to figure out the default permisission concurrently. That's fine
+ since they will end up with the same results. Even more technical,
+ apr_fileperms_t is an atomic type on 32+ bit machines.
+ */
+ if (default_perms == 0)
+ {
apr_finfo_t finfo;
apr_file_t *fd;
@@ -1321,6 +1331,13 @@ get_default_file_perms(apr_fileperms_t *
SVN_ERR(svn_io_file_close(fd, scratch_pool));
*perms = finfo.protection;
+ default_perms = finfo.protection;
+ }
+ else
+ {
+ *perms = default_perms;
+ }
+
return SVN_NO_ERROR;
}
]]]
- Julian
Re: [Merge request] Merge r985477 from performance branch
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@xbc.nu> writes:
> Got into this a bit late, sorry, but I'm not at all happy about this
> change.
>
> If we ignore the issue with long-running SVN processes ... ok, let's
> assume that changing umask requires that you restart daemons ...
>
> I cannot ignore two issues:
>
> * The default perms will come from the temp directory, not the WC
> directory. These perms may not be the same, defaults may be
> different on different volumes.
> * Even if we took the perms off of the current WC and stored them, a
> long-running process can work with multiple WCs at the same times
> and there's no guarantee that the default file perms are the same
> on all of them.
Those are valid concerns but not really an issue with patch which
merely caches the permissions and doesn't change the value that gets
cached. Also it was introduced as a server optimisation so repository
permissions are affected as well as working copy permissions.
--
Philip
Re: [Merge request] Merge r985477 from performance branch
Posted by Branko Čibej <br...@xbc.nu>.
Got into this a bit late, sorry, but I'm not at all happy about this
change.
If we ignore the issue with long-running SVN processes ... ok, let's
assume that changing umask requires that you restart daemons ...
I cannot ignore two issues:
* The default perms will come from the temp directory, not the WC
directory. These perms may not be the same, defaults may be
different on different volumes.
* Even if we took the perms off of the current WC and stored them, a
long-running process can work with multiple WCs at the same times
and there's no guarantee that the default file perms are the same
on all of them.
-- Brane