You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2012/10/29 17:36:36 UTC
Race in svn_atomic_namespace__create
I'm getting random regression test fails in parallel mode due to a race
in svn_atomic_namespace__create:
W: ERROR: dump failed: ../src/subversion/libsvn_fs_fs/fs_fs.c:3220: (apr_err=160052)
W: ERROR: dump failed: svnadmin: E160052: Revprop caching for 'svn-test-work/local_tmp/repos/db' disabled because SHM infrastructure for revprop caching failed to initialize.
W: ERROR: dump failed: ../src/subversion/libsvn_fs_fs/fs_fs.c:3171: (apr_err=17)
W: ERROR: dump failed: ../src/subversion/libsvn_subr/named_atomic.c:430: (apr_err=17)
W: ERROR: dump failed: svnadmin: E000017: Can't get shared memory for named atomics: File exists
The code does
/* First, look for an existing shared memory object. If it doesn't
* exist, create one.
*/
apr_err = apr_shm_attach(&shared_mem, shm_name, result_pool);
if (apr_err)
{
apr_err = apr_shm_create(&shared_mem,
sizeof(*new_ns->data),
shm_name,
result_pool);
if (apr_err)
return unlock(&new_ns->mutex,
svn_error_wrap_apr(apr_err,
_("Can't get shared memory for named atomics")));
and between the failed attach and the create another process could
create the segment.
I can't see any order in which we can do attach/create that doesn't have
a similar race. I think the best solution is a short loop trying
attach-create a few times before giving up.
--
Join us this October at Subversion Live 2012
http://www.wandisco.com/svn-live-2012
Re: Race in svn_atomic_namespace__create
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Oct 30, 2012 at 7:18 PM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, Oct 30, 2012 at 05:58:20PM +0000, Philip Martin wrote:
> > Another approach would be to create the shared memory created from some
> > other, long-lived, process. The user would have to run this process to
> > enable caching. To handle a large number of repositories this would
> > probably have to be some sort of daemon.
>
> Can we require an auxilliary process such as memcached to hold
> cached revprop data instead of caching it in-process?
> Or would that perform worse or equal to no caching at all?
>
Latency would probably be a major issue here even
if we used some RPC or socket interface. In that case,
we might as well read the file content from the OS'es
file cache. It is at least unclear whether we could still
be much faster than that.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Race in svn_atomic_namespace__create
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 30, 2012 at 05:58:20PM +0000, Philip Martin wrote:
> Another approach would be to create the shared memory created from some
> other, long-lived, process. The user would have to run this process to
> enable caching. To handle a large number of repositories this would
> probably have to be some sort of daemon.
Can we require an auxilliary process such as memcached to hold
cached revprop data instead of caching it in-process?
Or would that perform worse or equal to no caching at all?
Re: Race in svn_atomic_namespace__create
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Nov 06, 2012 at 11:47:51AM +0100, Stefan Fuhrmann wrote:
> I'd like to see all that solved and SHM being used
> for membuffer - which has been designed with that
> goal in mind. It's the robustness part that makes it
> so much harder to do than I thought back then.
Right. I see. Thanks for the explanation!
Re: Race in svn_atomic_namespace__create
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Nov 5, 2012 at 3:16 PM, Stefan Sperling <st...@elego.de> wrote:
> On Mon, Nov 05, 2012 at 02:54:07PM +0100, Stefan Fuhrmann wrote:
> > On Sun, Nov 4, 2012 at 10:40 AM, Stefan Sperling <st...@elego.de> wrote:
> > > I just came across something that reminded me of this thread.
> > > It seems PostgreSQL is doing something quite similar to what we
> > > want to do here:
> > >
> > > When the first PostgreSQL process attaches to the shared memory
> segment,
> > > it
> > > checks how many processes are attached. If the result is anything
> other
> > > than
> > > "one", it knows that there's another copy of PostgreSQL running which
> is
> > > pointed at the same data directory, and it bails out.
> > > http://rhaas.blogspot.nl/2012/06/absurd-shared-memory-limits.html
> > >
> >
> > IIUIC, the problems they are trying to solve are:
> >
> > * have only one process open / manage a given data base
> > * have SHM of arbitrary size
> >
> > Currently, we use named SHM to make the value of
> > two 64 bit numbers per repo visible to all processes.
> > Also, we don't have a master process that would
> > channel access to a given repository.
> >
> > The "corruption" issue is only about how to behave
> > if someone wrote random data to one of our repo
> > files. That's being addressed now (don't crash, have
> > a predictable behavior in most cases).
> >
> > > If this works for postgres I wonder why it wouldn't work for us.
> > > Is this something we cannot do because APR doesn't provide the
> > > necessary abstractions?
> > >
> >
> > The postgres code / approach may be helpful when
> > we try to move the whole membuffer cache into a
> > SHM segment.
>
> Ah, I see.
>
> Next question: Why don't we use a single SHM segment for the revprop cache?
>
> Revprop values are usually small so mapping a small amount of memory
> would suffice. And using a single SHM segment would make updated values
> immediately visible in all processes, wouldn't it? And we wouldn't need the
> generation number dance to make sure all processes see up-to-date values.
> Whichever process updates a revprop value would update the corresponding
> section of shared memory.
>
First of all, I want to point out that we now have a
working implementation for 1.8 and what we are
discussing here is probably targeted at future releases.
If we want revprop-only caches (to keep things simple),
we still need to handle the following basic trade-off:
Lifetime (effectiveness) ./. size. To be effective with
e.g. serf, the cache content should survive single
requests i.e. live longer than an fs_t. We also need
several MB (~200B/rev) per repo for decent hit rates.
OTOH, there may be hundreds of repositories on a
server and it is very hard to re-size the revprop cache
when the number of revs in a repo grows. It is thus
not quite feasible to keep fairly-sized per-repository
caches around indefinitely - even if they only contain
revprops.
That means that we should have one (or some small
number) shared cache for all repositories and let e.g.
some external process manage its lifetime etc. But
that is technically no different from having our membuffer
cache use shared memory instead of being process
local - which is a good thing.
The downside is that we need to address the following
3 issues when moving membuffer to SHM. From the
easiest to the hardest:
* make generations an integral feature of the cache
(e.g. by tagging index entries and bumping the
values upon "replace")
This is necessary to get rid of the revprop generations.
Race between revprop readers 1 and writer 2:
1: lookup revprop in cache -> miss
1: read revprop from disk -> "old content"
2: store new revprop on disk and in cache
1: store "old content" in cache
Note that after the 3rd step, the new content may or
may not be cached, i.e. we can't check for it in step 4.
* Have some SHM not bound to a repository or a
parent / child (fork) process relationship. Make it
work on most platforms.
* Portable, robust (lock owners may die), very low
overhead (~1musec) many-readers-one-writer locks
on the cache content. I have some ideas on how
to do that but this will be very hard to do correctly.
I'd like to see all that solved and SHM being used
for membuffer - which has been designed with that
goal in mind. It's the robustness part that makes it
so much harder to do than I thought back then.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Race in svn_atomic_namespace__create
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 05, 2012 at 02:54:07PM +0100, Stefan Fuhrmann wrote:
> On Sun, Nov 4, 2012 at 10:40 AM, Stefan Sperling <st...@elego.de> wrote:
> > I just came across something that reminded me of this thread.
> > It seems PostgreSQL is doing something quite similar to what we
> > want to do here:
> >
> > When the first PostgreSQL process attaches to the shared memory segment,
> > it
> > checks how many processes are attached. If the result is anything other
> > than
> > "one", it knows that there's another copy of PostgreSQL running which is
> > pointed at the same data directory, and it bails out.
> > http://rhaas.blogspot.nl/2012/06/absurd-shared-memory-limits.html
> >
>
> IIUIC, the problems they are trying to solve are:
>
> * have only one process open / manage a given data base
> * have SHM of arbitrary size
>
> Currently, we use named SHM to make the value of
> two 64 bit numbers per repo visible to all processes.
> Also, we don't have a master process that would
> channel access to a given repository.
>
> The "corruption" issue is only about how to behave
> if someone wrote random data to one of our repo
> files. That's being addressed now (don't crash, have
> a predictable behavior in most cases).
>
> > If this works for postgres I wonder why it wouldn't work for us.
> > Is this something we cannot do because APR doesn't provide the
> > necessary abstractions?
> >
>
> The postgres code / approach may be helpful when
> we try to move the whole membuffer cache into a
> SHM segment.
Ah, I see.
Next question: Why don't we use a single SHM segment for the revprop cache?
Revprop values are usually small so mapping a small amount of memory
would suffice. And using a single SHM segment would make updated values
immediately visible in all processes, wouldn't it? And we wouldn't need the
generation number dance to make sure all processes see up-to-date values.
Whichever process updates a revprop value would update the corresponding
section of shared memory.
Re: Race in svn_atomic_namespace__create
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Nov 4, 2012 at 10:40 AM, Stefan Sperling <st...@elego.de> wrote:
> On Wed, Oct 31, 2012 at 03:24:10PM +0100, Stefan Fuhrmann wrote:
> > On Wed, Oct 31, 2012 at 2:54 PM, Philip Martin
> > <ph...@wandisco.com>wrote:
> >
> > > Philip Martin <ph...@wandisco.com> writes:
> > >
> > > > Stefan Fuhrmann <st...@wandisco.com> writes:
> > > >
> > > >> Excellent analysis, Philip! With r1404112, we use "plain"
> > > >> APR mmap code with almost no coding overhead.
> > > >> The only downside is that we now have a temporary
> > > >> file sitting in the db folder.
> > > >
> > > > Error handling needs attention:
> > > >
> > > > $ svnadmin create repo
> > > > $ svnadmin dump repo > /dev/null
> > > > $ chmod -rw repo/db/rev-prop-atomicsShm
> > > > $ svnadmin dump repo > /dev/null
> > > > Segmentation fault
> > >
> > > We are mmaping a 64k file, that's bigger than a disk block on lots of
> > > filesystems so updates are not atomic. Do we have to consider
> > > corruption:
> > >
> > > $ svnadmin create repo
> > > $ dd if=/dev/urandom of=repo/db/rev-prop-atomicsShm bs=64k count=1
> > > $ svnadmin verify repo
> > > Segmentation fault
> > > $ svnadmin recover repo
> > > Repository lock acquired.
> > > Please wait; recovering the repository may take some time...
> > >
> > > Recovery completed.
> > > The latest repos revision is 0.
> > > $ svnadmin verify repo
> > > Segmentation fault
> > >
> > > Perhaps recover should delete the file?
> > >
> >
> > Done. Also, random data should no longer result in segfaults.
>
> I just came across something that reminded me of this thread.
> It seems PostgreSQL is doing something quite similar to what we
> want to do here:
>
> When the first PostgreSQL process attaches to the shared memory segment,
> it
> checks how many processes are attached. If the result is anything other
> than
> "one", it knows that there's another copy of PostgreSQL running which is
> pointed at the same data directory, and it bails out.
> http://rhaas.blogspot.nl/2012/06/absurd-shared-memory-limits.html
>
IIUIC, the problems they are trying to solve are:
* have only one process open / manage a given data base
* have SHM of arbitrary size
Currently, we use named SHM to make the value of
two 64 bit numbers per repo visible to all processes.
Also, we don't have a master process that would
channel access to a given repository.
The "corruption" issue is only about how to behave
if someone wrote random data to one of our repo
files. That's being addressed now (don't crash, have
a predictable behavior in most cases).
If this works for postgres I wonder why it wouldn't work for us.
> Is this something we cannot do because APR doesn't provide the
> necessary abstractions?
>
The postgres code / approach may be helpful when
we try to move the whole membuffer cache into a
SHM segment.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Race in svn_atomic_namespace__create
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Oct 31, 2012 at 03:24:10PM +0100, Stefan Fuhrmann wrote:
> On Wed, Oct 31, 2012 at 2:54 PM, Philip Martin
> <ph...@wandisco.com>wrote:
>
> > Philip Martin <ph...@wandisco.com> writes:
> >
> > > Stefan Fuhrmann <st...@wandisco.com> writes:
> > >
> > >> Excellent analysis, Philip! With r1404112, we use "plain"
> > >> APR mmap code with almost no coding overhead.
> > >> The only downside is that we now have a temporary
> > >> file sitting in the db folder.
> > >
> > > Error handling needs attention:
> > >
> > > $ svnadmin create repo
> > > $ svnadmin dump repo > /dev/null
> > > $ chmod -rw repo/db/rev-prop-atomicsShm
> > > $ svnadmin dump repo > /dev/null
> > > Segmentation fault
> >
> > We are mmaping a 64k file, that's bigger than a disk block on lots of
> > filesystems so updates are not atomic. Do we have to consider
> > corruption:
> >
> > $ svnadmin create repo
> > $ dd if=/dev/urandom of=repo/db/rev-prop-atomicsShm bs=64k count=1
> > $ svnadmin verify repo
> > Segmentation fault
> > $ svnadmin recover repo
> > Repository lock acquired.
> > Please wait; recovering the repository may take some time...
> >
> > Recovery completed.
> > The latest repos revision is 0.
> > $ svnadmin verify repo
> > Segmentation fault
> >
> > Perhaps recover should delete the file?
> >
>
> Done. Also, random data should no longer result in segfaults.
I just came across something that reminded me of this thread.
It seems PostgreSQL is doing something quite similar to what we
want to do here:
When the first PostgreSQL process attaches to the shared memory segment, it
checks how many processes are attached. If the result is anything other than
"one", it knows that there's another copy of PostgreSQL running which is
pointed at the same data directory, and it bails out.
http://rhaas.blogspot.nl/2012/06/absurd-shared-memory-limits.html
If this works for postgres I wonder why it wouldn't work for us.
Is this something we cannot do because APR doesn't provide the
necessary abstractions?
Re: Race in svn_atomic_namespace__create
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 31, 2012 at 2:54 PM, Philip Martin
<ph...@wandisco.com>wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
> > Stefan Fuhrmann <st...@wandisco.com> writes:
> >
> >> Excellent analysis, Philip! With r1404112, we use "plain"
> >> APR mmap code with almost no coding overhead.
> >> The only downside is that we now have a temporary
> >> file sitting in the db folder.
> >
> > Error handling needs attention:
> >
> > $ svnadmin create repo
> > $ svnadmin dump repo > /dev/null
> > $ chmod -rw repo/db/rev-prop-atomicsShm
> > $ svnadmin dump repo > /dev/null
> > Segmentation fault
>
> We are mmaping a 64k file, that's bigger than a disk block on lots of
> filesystems so updates are not atomic. Do we have to consider
> corruption:
>
> $ svnadmin create repo
> $ dd if=/dev/urandom of=repo/db/rev-prop-atomicsShm bs=64k count=1
> $ svnadmin verify repo
> Segmentation fault
> $ svnadmin recover repo
> Repository lock acquired.
> Please wait; recovering the repository may take some time...
>
> Recovery completed.
> The latest repos revision is 0.
> $ svnadmin verify repo
> Segmentation fault
>
> Perhaps recover should delete the file?
>
Done. Also, random data should no longer result in segfaults.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Stefan Fuhrmann <st...@wandisco.com> writes:
>
>> Excellent analysis, Philip! With r1404112, we use "plain"
>> APR mmap code with almost no coding overhead.
>> The only downside is that we now have a temporary
>> file sitting in the db folder.
>
> Error handling needs attention:
>
> $ svnadmin create repo
> $ svnadmin dump repo > /dev/null
> $ chmod -rw repo/db/rev-prop-atomicsShm
> $ svnadmin dump repo > /dev/null
> Segmentation fault
We are mmaping a 64k file, that's bigger than a disk block on lots of
filesystems so updates are not atomic. Do we have to consider
corruption:
$ svnadmin create repo
$ dd if=/dev/urandom of=repo/db/rev-prop-atomicsShm bs=64k count=1
$ svnadmin verify repo
Segmentation fault
$ svnadmin recover repo
Repository lock acquired.
Please wait; recovering the repository may take some time...
Recovery completed.
The latest repos revision is 0.
$ svnadmin verify repo
Segmentation fault
Perhaps recover should delete the file?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> Excellent analysis, Philip! With r1404112, we use "plain"
> APR mmap code with almost no coding overhead.
> The only downside is that we now have a temporary
> file sitting in the db folder.
Error handling needs attention:
$ svnadmin create repo
$ svnadmin dump repo > /dev/null
$ chmod -rw repo/db/rev-prop-atomicsShm
$ svnadmin dump repo > /dev/null
Segmentation fault
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Race in svn_atomic_namespace__create
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Oct 30, 2012 at 6:58 PM, Philip Martin
<ph...@wandisco.com>wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
> > That's a serious problem, the two processes are not longer using the
> > same shared memory segment to keep in sync. Changes made by one process
> > won't be visible to another.
>
> I don't see how to fix this with the current APR code. The process that
> creates the named shared memory will always delete the associated file
> at pool cleanup and that stops further processes attaching. I guess
> this is designed for a server where the parent process passes shared
> memory to child processes but it doesn't really fit our model of
> independent processes. I think we would have to change APR to get this
> to work.
>
> An alternative approach would use anonymous shared memory and have
> Subversion itself manage the ID outside APR (storing it in the lock file
> perhaps) but APR doesn't support that either: APR doesn't make the ID
> available or support attaching to anonymous shared memory. Again, we
> would have to add support to APR and then require the new APR version.
>
> Another approach would be to create the shared memory created from some
> other, long-lived, process. The user would have to run this process to
> enable caching. To handle a large number of repositories this would
> probably have to be some sort of daemon.
>
> Perhaps we could use apr_mmap_create instead? APR doesn't specify
> whether the mmap is SHARED or PRIVATE but the current implementation on
> Unix is shared. I don't know enough about Windows to understand how
> that implementation behaves.
>
Excellent analysis, Philip! With r1404112, we use "plain"
APR mmap code with almost no coding overhead.
The only downside is that we now have a temporary
file sitting in the db folder.
As for the Windows code, mmap is being used for
shared memory there as well - so it should just work.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> That's a serious problem, the two processes are not longer using the
> same shared memory segment to keep in sync. Changes made by one process
> won't be visible to another.
I don't see how to fix this with the current APR code. The process that
creates the named shared memory will always delete the associated file
at pool cleanup and that stops further processes attaching. I guess
this is designed for a server where the parent process passes shared
memory to child processes but it doesn't really fit our model of
independent processes. I think we would have to change APR to get this
to work.
An alternative approach would use anonymous shared memory and have
Subversion itself manage the ID outside APR (storing it in the lock file
perhaps) but APR doesn't support that either: APR doesn't make the ID
available or support attaching to anonymous shared memory. Again, we
would have to add support to APR and then require the new APR version.
Another approach would be to create the shared memory created from some
other, long-lived, process. The user would have to run this process to
enable caching. To handle a large number of repositories this would
probably have to be some sort of daemon.
Perhaps we could use apr_mmap_create instead? APR doesn't specify
whether the mmap is SHARED or PRIVATE but the current implementation on
Unix is shared. I don't know enough about Windows to understand how
that implementation behaves.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> Maybe, there should be a regression test that
> tries concurrent initialization.
There are two objects here: the named file and the shared memory
segment. The usual sequence for a single process is: create file,
create segment, delete segment, delete file. There are two windows when
the file exists and the segment does not and at that point a second
process can neither attach nor create. The create window is protected
by the inter-process lock but I think the delete window is not protected
as it is triggered by a pool cleanup. We need to find a way to either
take the inter-process lock or introduce some sort of delay/retry.
However I think there is a bigger problem: I don't think this use of APR
named memory allows processes to start and stop in an unordered fashion.
I've built APR 1.4.x and configure chooses:
apr.h:#define APR_USE_SHMEM_MMAP_TMP 0
apr.h:#define APR_USE_SHMEM_MMAP_SHM 0
apr.h:#define APR_USE_SHMEM_MMAP_ZERO 0
apr.h:#define APR_USE_SHMEM_SHMGET_ANON 0
apr.h:#define APR_USE_SHMEM_SHMGET 1
apr.h:#define APR_USE_SHMEM_MMAP_ANON 1
apr.h:#define APR_USE_SHMEM_BEOS 0
Start a process, svnadmin in my case, and apr_shm_attach fails as this
is the first process so apr_shm_create is run. This creates the named
file and the shared memory segment. The apr call installs a cleanup
handler shm_cleanup_owner. At this point I see the shared memory
segment:
$ ipcs -m | grep 7340039
0x0101e244 7340039 pm 600 65536 1
and the process using it:
$ lsof | grep 7340039
lt-svnadm 24614 pm DEL REG 0,4 7340039 /SYSV0101e244
Start a second process, this time the apr_shm_attach works and the two
processes use the same shared memory segment. At this point I see two
processes using the shared memory segment:
$ lsof | grep 7340039
lt-svnadm 24614 pm DEL REG 0,4 7340039 /SYSV0101e244
lt-svnadm 24623 pm DEL REG 0,4 7340039 /SYSV0101e244
Allow the first process to exit. The cleanup handler detaches from the
shared memory removes the named file. The second process still uses the
segment but the named file is deleted. At this point the shared memory
segment key has changed:
$ ipcs -m | grep 7340039
0x00000000 7340039 pm 600 65536 1 dest
but the second process is still using it:
lt-svnadm 24623 pm DEL REG 0,4 7340039 /SYSV0101e244
Start a third process, the apr_shm_attach fails because the file is
removed. So this process creates a new named file and a second shared
memory segment. At this point I see two shared memory segments:
$ ipcs -m | egrep '(7340039|7372808)'
0x00000000 7340039 pm 600 65536 1 dest
0x0101e245 7372808 pm 600 65536 1
and the second and third processes are using different segments:
$ lsof | egrep '(7340039|7372808)'
lt-svnadm 24623 pm DEL REG 0,4 7340039 /SYSV0101e244
lt-svnadm 24637 pm DEL REG 0,4 7372808 /SYSV0101e245
That's a serious problem, the two processes are not longer using the
same shared memory segment to keep in sync. Changes made by one process
won't be visible to another.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Race in svn_atomic_namespace__create
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Oct 30, 2012 at 12:11 AM, Philip Martin
<ph...@wandisco.com>wrote:
> Stefan Fuhrmann <st...@wandisco.com> writes:
>
> > On Mon, Oct 29, 2012 at 10:46 PM, Philip Martin
> > <ph...@wandisco.com>wrote:
> >
> >> Philip Martin <ph...@wandisco.com> writes:
> >>
> >> > Philip Martin <ph...@wandisco.com> writes:
> >> >
> >> >> I can't see any order in which we can do attach/create that doesn't
> have
> >> >> a similar race. I think the best solution is a short loop trying
> >> >> attach-create a few times before giving up.
> >> >
> >> > I've committed a loop in r1403463. That doesn't fix the race but it
> is
> >> > now very unlikely to fail.
> >>
> >
> > The creation code is protected by a repo-global lock/unlock pair.
> > So, in theory, there should be no race condition.
>
> Which lock and where? Does this lock out other processes?
>
Lines 266 to 292 implement the lock. It first takes out the
process-local and and then the global lock (a repo-local file
lock). L416 acquires the lock in svn_atomic_namespace__create
and L453 releases it.
>> I've just observed the same failure with the looping code. I'm not sure
> >> what is wrong. I suppose there is a window during the creation process
> >> where the file exists, so the create fails, but the memory is not yet
> >> ready, so the attach also fails. If one process is in this state
> >> another process might loop around 10 times and have both create and
> >> attach fail. Perhaps a short and/or random delay would help?
> >>
> >
> > It's on my TODO list to identify the root cause of this issue.
>
> I think it must be the window between
>
> apr_file_open( APR_EXCL )
>
> and
>
> mmap( MAP_SHARED )
>
> in apr_shm_create. During that period any other process will see both
> apr_shm_create and apr_shm_attach fail. But that would imply that your
> process lock isn't working.
>
It is well possible that the locking logic is faulty.
Maybe, there should be a regression test that
tries concurrent initialization.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Stefan Fuhrmann <st...@wandisco.com> writes:
>
>> The creation code is protected by a repo-global lock/unlock pair.
>> So, in theory, there should be no race condition.
>
> Which lock and where? Does this lock out other processes?
I see it's db/rev-prop-atomicsMutex
>> It's on my TODO list to identify the root cause of this issue.
I can reproduce the error as follows:
$ svnadmin create repo
$ gdb -arg subversion/svnadmin/.libs/lt-svnadmin dump repo
(gdb) b svn_atomic_namespace__create
(gdb) run
Breakpoint 1, svn_atomic_namespace__create
(gdb) fin
(gdb) kill
(gdb) shell rm repo/db/rev-prop-atomicsShm
(gdb) run
Breakpoint 1, svn_atomic_namespace__create
(gdb) cont
../src/subversion/libsvn_fs_fs/fs_fs.c:3220: (apr_err=160052)
svnadmin: E160052: Revprop caching for 'repo/db' disabled because SHM infrastructure for revprop caching failed to initialize.
../src/subversion/libsvn_fs_fs/fs_fs.c:3171: (apr_err=17)
../src/subversion/libsvn_subr/named_atomic.c:455: (apr_err=17)
svnadmin: E000017: Can't get shared memory for named atomics: File exists
Not sure I understand it.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> On Mon, Oct 29, 2012 at 10:46 PM, Philip Martin
> <ph...@wandisco.com>wrote:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>> > Philip Martin <ph...@wandisco.com> writes:
>> >
>> >> I can't see any order in which we can do attach/create that doesn't have
>> >> a similar race. I think the best solution is a short loop trying
>> >> attach-create a few times before giving up.
>> >
>> > I've committed a loop in r1403463. That doesn't fix the race but it is
>> > now very unlikely to fail.
>>
>
> The creation code is protected by a repo-global lock/unlock pair.
> So, in theory, there should be no race condition.
Which lock and where? Does this lock out other processes?
>> I've just observed the same failure with the looping code. I'm not sure
>> what is wrong. I suppose there is a window during the creation process
>> where the file exists, so the create fails, but the memory is not yet
>> ready, so the attach also fails. If one process is in this state
>> another process might loop around 10 times and have both create and
>> attach fail. Perhaps a short and/or random delay would help?
>>
>
> It's on my TODO list to identify the root cause of this issue.
I think it must be the window between
apr_file_open( APR_EXCL )
and
mmap( MAP_SHARED )
in apr_shm_create. During that period any other process will see both
apr_shm_create and apr_shm_attach fail. But that would imply that your
process lock isn't working.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Race in svn_atomic_namespace__create
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Oct 29, 2012 at 10:46 PM, Philip Martin
<ph...@wandisco.com>wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
> > Philip Martin <ph...@wandisco.com> writes:
> >
> >> I can't see any order in which we can do attach/create that doesn't have
> >> a similar race. I think the best solution is a short loop trying
> >> attach-create a few times before giving up.
> >
> > I've committed a loop in r1403463. That doesn't fix the race but it is
> > now very unlikely to fail.
>
The creation code is protected by a repo-global lock/unlock pair.
So, in theory, there should be no race condition.
> I've just observed the same failure with the looping code. I'm not sure
> what is wrong. I suppose there is a window during the creation process
> where the file exists, so the create fails, but the memory is not yet
> ready, so the attach also fails. If one process is in this state
> another process might loop around 10 times and have both create and
> attach fail. Perhaps a short and/or random delay would help?
>
It's on my TODO list to identify the root cause of this issue.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> I can't see any order in which we can do attach/create that doesn't have
>> a similar race. I think the best solution is a short loop trying
>> attach-create a few times before giving up.
>
> I've committed a loop in r1403463. That doesn't fix the race but it is
> now very unlikely to fail.
I've just observed the same failure with the looping code. I'm not sure
what is wrong. I suppose there is a window during the creation process
where the file exists, so the create fails, but the memory is not yet
ready, so the attach also fails. If one process is in this state
another process might loop around 10 times and have both create and
attach fail. Perhaps a short and/or random delay would help?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Race in svn_atomic_namespace__create
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> I can't see any order in which we can do attach/create that doesn't have
> a similar race. I think the best solution is a short loop trying
> attach-create a few times before giving up.
I've committed a loop in r1403463. That doesn't fix the race but it is
now very unlikely to fail.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download