[PATCH] [ldb_modules ABI break] Re: [WIP][PATCH] Safe LDB locking for better replication

[Andrew Bartlett]

On Thu, 2017-06-22 at 17:50 +1200, Andrew Bartlett via samba-technical
wrote:
> On Fri, 2017-06-16 at 15:13 +0200, Stefan Metzmacher via samba-
> technical wrote:
> > Am 16.06.2017 um 06:55 schrieb Andrew Bartlett:
> > > On Thu, 2017-06-15 at 16:58 +1200, Andrew Bartlett via samba-
> > > technical
> > > wrote:
> > > > On Wed, 2017-06-14 at 22:02 +1200, Andrew Bartlett via samba-
> > > > technical
> > > > wrote:
> > > > > > Further comments most welcome, and I plan to re-write it to
> > > > > > use
> > > > > > read_lock() and read_unlock() operations tomorrow. 
> > > > 
> > > > Attached is the patch set against master
> > > > 
> > > > As mentioned previously, my TODO list is:
> > > >  - fix tests not to hang when failing
> > > 
> > > They don't seem to hang, at least when reverting the whole-db lock.
> > > 
> > > >  - add test for the talloc_free(req) unlock case
> > > 
> > > I've added such a test.
> > > 
> > > >  - add some kind of test for the partitions locking 
> > > 
> > > I've added a test for this.
> > > 
> > > >  - a dbcheck rule for out of date @ATTRIBUTES and @INDEXLIST
> > > > (rather
> > > > than a runtime check)
> > > 
> > > This is still TODO.
> > > 
> > > With the proviso that I need that dbcheck rule (or drop this aspect
> > > of
> > > the patches), can I please get your review on this series.  I'll
> > > run
> > > some more builds, but I'm pretty confident on the patch series at
> > > this
> > > point and would like to get this into master as soon as you are
> > > comfortable. 
> > > 
> > > It would be good to have at least a week of this in master before
> > > rc1
> > > freezes, so we can deal with any unexpected fallout, despite our
> > > thorough testing.
> > 
> > I've pushed the first bunch of patches.
> > 
> > You can also push the attaches two patches (which I split from one
> > single commit that looked strange).
> 
> I've taken the first of those, and discarded the behaviour change.  We
> can discuss that once this more important fix is in.  Therefore I think
> the patches are now ready for master.  
> 
> Additionally, while searches during the init_module hooks are not
> ideal, they are still locked at the ldb_tdb level, and we can add more
> locks once we have this in. 
> 
> Therefore, I propose we merge the attached, which I've had Douglas
> carefully look over to try and get a fresh perspective.  
> 
> The tests I've initiated pass, and I look forward to your review and
> hopefully a push!

Without wishing to derail this, I would note that this does change the
ldb module ABI (which we never promised anyway), and also is not
backwards compatible with Samba < 4.7.

It is likely to trigger deadlock detected "failed to upgrade hash
locks", LDB_ERR_BUSY on earlier Samba versions, as they came to not
expect locking (and the required lock ordering) to be enforced on
ldb_tdb.

Naturally, this is balanced by the fact that we absolutely have to
merge this, as the read and replication corruption it caused has been
observed in the real world, not just our selftest.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba