openldap-bugs

openldap-bugs@openldap.org

1 participants
21001 discussions

Re: (ITS#5451) syncprov_checkpoint deadlock bugfix?
by rein＠tollevik.no 10 Apr '08

10 Apr '08

On Wed, 9 Apr 2008, h.b.furuseth(a)usit.uio.no wrote: > Does this help? From my fiddling with ITS#5340 (REP_ENTRY_MODIFIABLE). > I do not understand syncprov's handling of REP_ENTRY_MUSTRELEASE though. > (For one thing it seems to assume that REP_ENTRY_MUSTRELEASE is set if > and only if rs.sr_entry->e_private != NULL. Which is possibly true with > back-bdb but seems a shaky assumption in general.) Almost, except that it triggers an abort.. Using be_entry_release_r() was not correct, as it ended up calling entry_free(). It works if I changes it to use overlay_entry_release_ov() instead, as in the alternative patch at the end. I have put a test script that shows this deadlock on: ftp://ftp.openldap.org/incoming/test053-syncprov-glue It deadlocks without this patch, it and the rest of the test suite succeed with it :-) I haven't tested it in production though.. Rein Index: OpenLDAP/servers/slapd/overlays/syncprov.c diff -u OpenLDAP/servers/slapd/overlays/syncprov.c:1.1.1.14 OpenLDAP/servers/slapd/overlays/syncprov.c:1.14 --- OpenLDAP/servers/slapd/overlays/syncprov.c:1.1.1.14 Thu Apr 10 17:31:24 2008 +++ OpenLDAP/servers/slapd/overlays/syncprov.c Thu Apr 10 17:34:36 2008 @@ -2389,7 +2396,14 @@ if ( !ap ) { if ( !(rs->sr_flags & REP_ENTRY_MODIFIABLE) ) { - rs->sr_entry = entry_dup( rs->sr_entry ); + Entry *new = entry_dup( rs->sr_entry ); + if ( rs->sr_flags & REP_ENTRY_MUSTRELEASE ) { + overlay_entry_release_ov( op, rs->sr_entry, 0, (slap_overinst *)op->o_bd->bd_info ); + rs->sr_flags ^= REP_ENTRY_MUSTRELEASE; + } else if ( rs->sr_flags & REP_ENTRY_MUSTBEFREED ) { + entry_free( rs->sr_entry ); + } + rs->sr_entry = new; rs->sr_flags |= REP_ENTRY_MODIFIABLE|REP_ENTRY_MUSTBEFREED; a = attr_find( rs->sr_entry->e_attrs,

1 0

Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist
by h.b.furuseth＠usit.uio.no 10 Apr '08

10 Apr '08

Howard Chu writes: > I have the feeling that MUSTBEFREED and MUSTRELEASE should be > consolidated into a single flag. be_entry_release_r() should simply > call entry_free() if e->e_private is NULL. Maybe. But consider translucent - it grabs incoming entries and remembers them. It can "steal" a MUSTBEFREED entry (though it doesn't now), but must duplicate a MUSTRELEASE entry so that it won't hold on to a lock in an underlying database. -- Hallvard

1 0

Re: (ITS#5451) syncprov_checkpoint deadlock bugfix?
by h.b.furuseth＠usit.uio.no 10 Apr '08

10 Apr '08

Howard Chu writes: > h.b.furuseth(a)usit.uio.no wrote: >> Does this help? From my fiddling with ITS#5340 (REP_ENTRY_MODIFIABLE). >> I do not understand syncprov's handling of REP_ENTRY_MUSTRELEASE though. >> (For one thing it seems to assume that REP_ENTRY_MUSTRELEASE is set if >> and only if rs.sr_entry->e_private != NULL. Which is possibly true with >> back-bdb but seems a shaky assumption in general.) > > That is a requirement, yes. If you implement a backend that has > cache/locking constraints on its entries and you don't provide a > pointer from the entry back to your cache state, then you're an idiot. OTOH you can send an entry without setting MUSTRELEASE, and release it after sending instead. In particular if you didn't about the undocumented sr_flags and their purpose. Or if you don't use be_release for unlocking but just to clean up e_private. ... have I gone blind? I don't see back-bdb setting that flag. Did I just read ITS#3671 and trust it to have updated bdb? -- Hallvard

1 0

Re: (ITS#5459) threw assertion in attr_dup
by richton＠nbcs.rutgers.edu 10 Apr '08

10 Apr '08

> Is this entry still in your database now? Clearly this condition should never > happen - if zero-length values are valid for the syntax, then it should have > been preserved in the (normalized) a_nvals. If not, then it should not have > been in the a_vals in the first place. No, it got deleted last night. But from the core I see: at_syntax_oid = 0x100495d80 "1.3.6.1.4.1.1466.115.121.1.26" IA5String, so empty string is valid. (And makes no logical sense in this situation, but that's for another day...)

1 0

Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist
by hyc＠symas.com 10 Apr '08

10 Apr '08

h.b.furuseth(a)usit.uio.no wrote: > - REP_ENTRY_MUSTRELEASE and REP_ENTRY_MUSTBEFREED: > > Callers that set these flags when sending an entry, *allow* the > callee to release/free rs->sr_entry. > Callers cannot depend on it being release/freed, but some do anyway. > > It should simplify things to make the send functions always obey > this flag, but I'm not sure how much cleanup that requires. > There is partial code for that in slap_send_search_entry(), along > with a FIXME note. > > Free MUSTBEFREED-entries with entry_free(), and MUSTRELEASE-entries > with be_entry_release_r(). There is no "use be_entry_release_w()" > flag, but no OpenLDAP code sends write-locked entries anyway. I have the feeling that MUSTBEFREED and MUSTRELEASE should be consolidated into a single flag. be_entry_release_r() should simply call entry_free() if e->e_private is NULL. > + REP_ENTRY_MUSTRELEASE: > > Created for ITS#3671 to ensure that network hangs don't delay > cache entry unlocks: > > Modules should release their cache entry locks before sending > anything. They can't do that with cached entries they send in > rs->sr_entry, so instead they should set REP_ENTRY_MUSTRELEASE. > Conversely, anything which does network I/O (and other potentially > slooow ops I guess) should first obey this flag. > > Several overlays don't obey, nor some other code for sending an > entry when it decides to send an error response instead. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#5451) syncprov_checkpoint deadlock bugfix?
by hyc＠symas.com 10 Apr '08

10 Apr '08

h.b.furuseth(a)usit.uio.no wrote: > Does this help? From my fiddling with ITS#5340 (REP_ENTRY_MODIFIABLE). > I do not understand syncprov's handling of REP_ENTRY_MUSTRELEASE though. > (For one thing it seems to assume that REP_ENTRY_MUSTRELEASE is set if > and only if rs.sr_entry->e_private != NULL. Which is possibly true with > back-bdb but seems a shaky assumption in general.) That is a requirement, yes. If you implement a backend that has cache/locking constraints on its entries and you don't provide a pointer from the entry back to your cache state, then you're an idiot. That means every time you want to manipulate the cache state for that entry you'll have to do an expensive search for it, instead of simply dereferencing e_private. If you maintain private state for the entry, you point to it with e_private. If e_private is not set, then there is no private state. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#5459) threw assertion in attr_dup
by hyc＠symas.com 10 Apr '08

10 Apr '08

Aaron Richton wrote: > Ergh, didn't realize there was more past that. > > (dbx) print a->a_vals[0] > a->a_vals[0] = { > bv_len = 28U > bv_val = 0x11925f250 "fixed-address 172.16.143.152" > } > (dbx) print a->a_vals[1] > a->a_vals[1] = { > bv_len = 0 > bv_val = 0x117e9a810 "" > } > (dbx) print a->a_vals[2] > a->a_vals[2] = { > bv_len = 0 > bv_val = (nil) > } > (dbx) print a->a_nvals[0] > a->a_nvals[0] = { > bv_len = 28U > bv_val = 0x11a48ff90 "fixed-address 172.16.143.152" > } > (dbx) print a->a_nvals[1] > a->a_nvals[1] = { > bv_len = 0 > bv_val = (nil) > } > Is this entry still in your database now? Clearly this condition should never happen - if zero-length values are valid for the syntax, then it should have been preserved in the (normalized) a_nvals. If not, then it should not have been in the a_vals in the first place. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#5459) threw assertion in attr_dup
by richton＠nbcs.rutgers.edu 10 Apr '08

10 Apr '08

Ergh, didn't realize there was more past that. (dbx) print a->a_vals[0] a->a_vals[0] = { bv_len = 28U bv_val = 0x11925f250 "fixed-address 172.16.143.152" } (dbx) print a->a_vals[1] a->a_vals[1] = { bv_len = 0 bv_val = 0x117e9a810 "" } (dbx) print a->a_vals[2] a->a_vals[2] = { bv_len = 0 bv_val = (nil) } (dbx) print a->a_nvals[0] a->a_nvals[0] = { bv_len = 28U bv_val = 0x11a48ff90 "fixed-address 172.16.143.152" } (dbx) print a->a_nvals[1] a->a_nvals[1] = { bv_len = 0 bv_val = (nil) }

1 0

Re: (ITS#5459) threw assertion in attr_dup
by hyc＠symas.com 10 Apr '08

10 Apr '08

Aaron Richton wrote: >> Definitely an oddball. Can you print i, j, and a->a_vals[0] thru >> a->a_vals[i], >> plus a->a_nvals[0] thru a->a_nvals[j]. > > Just got back to my desk and started looking at this. So when the two > slaves that threw this were restarted, they went into an infinite loop > trying to replicate the entry that killed them: > > <= entry_decode: slap_str2undef_ad(^Zethernet 01:aa:aa:aa:aa:a1): AttributeDescription contains inappropriate characters > null_callback: error code 0x50 > syncrepl_entry: rid 110 be_add (80) > syncrepl_entry: rid 110 be_add failed (80) > do_syncrepl: rid 110 retrying > > and retrying, and retrying, and retrying...hence my thought that > the database was hosed...UNTIL... > > Apparently the entry was DEL'd a few hours later. I've now got perfect > sync from the master to the assert'd slaves. The entry_decode error definitely implies a corruption somewhere. It was looking at a 26 byte value as if it was the name of an attribute. I wonder if slapcat will run cleanly on it now, or if it will also encounter some decode errors. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#5457) bdb_add() does not always release entries
by hyc＠symas.com 10 Apr '08

10 Apr '08

Hallvard B Furuseth wrote: > hyc(a)symas.com writes: >>> bdb_unlocked_cache_return_entry_r(&bdb->bi_cache, p ); >>> (...) >> Releasing entries is somewhat irrelevant, since all locks are released when >> the transaction commits. Note that this function is a no-op in proto-bdb.h. >> All of those statements are just relics from the first entry cache design, >> before we switched to using BDB locks for everything. > > OK. Does that apply to bdb_cache_return_entry_r( bdb, oe,&lock ); > after > bdb_cache_add( bdb, ei, op->ora_e,&nrdn, rlocker,&lock ); > failed TXN_COMMIT( ltid, 0 ); > too? bdb_cache_return_entry_r() is not a no-op. No, that call obviously matters. But I'm not sure that this commit can ever actually fail, or what to do about it if it does. Since all of these write ops use a nested transaction, and there is nothing outstanding in the outer transaction, and the inner transaction has already committed successfully, the only thing that could cause the outer commit to fail would be something like running out of disk for the transaction log. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs