[PATCH] Enable TDB mutexes by default in dbwrap and ctdb

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
Hi all,

as discussed previously, attached patchset enables TDB mutexes by default in
dbwrap and in ctdb.

Amitay and Martin: did I miss any pieces in ctdb?

I filed a bugnumber for this to get it into 4.7.

Please review.

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
Hi Ralph,

On Mon, 10 Jul 2017 12:17:50 +0200, Ralph Böhme <[hidden email]> wrote:

> as discussed previously, attached patchset enables TDB mutexes by default in
> dbwrap and in ctdb.
>
> Amitay and Martin: did I miss any pieces in ctdb?
>
> I filed a bugnumber for this to get it into 4.7.
>
> Please review.

- { "TDBMutexEnabled", 0, false,
+ { "TDBMutexEnabled", 0, true,

2nd element is the tunable value, 3rd is whether it is obsolete.  So
you really want:

+ { "TDBMutexEnabled", 1, false,

:-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Mon, Jul 10, 2017 at 09:41:52PM +1000, Martin Schwenke wrote:

> Hi Ralph,
>
> On Mon, 10 Jul 2017 12:17:50 +0200, Ralph Böhme <[hidden email]> wrote:
>
> > as discussed previously, attached patchset enables TDB mutexes by default in
> > dbwrap and in ctdb.
> >
> > Amitay and Martin: did I miss any pieces in ctdb?
> >
> > I filed a bugnumber for this to get it into 4.7.
> >
> > Please review.
>
> - { "TDBMutexEnabled", 0, false,
> + { "TDBMutexEnabled", 0, true,
>
> 2nd element is the tunable value, 3rd is whether it is obsolete.  So
> you really want:
>
> + { "TDBMutexEnabled", 1, false,
>
> :-)
argl.

Updated patch attached.

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
Hi Ralph,

On Mon, Jul 10, 2017 at 9:56 PM, Ralph Böhme <[hidden email]> wrote:

> On Mon, Jul 10, 2017 at 09:41:52PM +1000, Martin Schwenke wrote:
> > Hi Ralph,
> >
> > On Mon, 10 Jul 2017 12:17:50 +0200, Ralph Böhme <[hidden email]> wrote:
> >
> > > as discussed previously, attached patchset enables TDB mutexes by
> default in
> > > dbwrap and in ctdb.
> > >
> > > Amitay and Martin: did I miss any pieces in ctdb?
> > >
> > > I filed a bugnumber for this to get it into 4.7.
> > >
> > > Please review.
> >
> > -     { "TDBMutexEnabled", 0, false,
> > +     { "TDBMutexEnabled", 0, true,
> >
> > 2nd element is the tunable value, 3rd is whether it is obsolete.  So
> > you really want:
> >
> > +     { "TDBMutexEnabled", 1, false,
> >
> > :-)
>
> argl.
>
> Updated patch attached.
>
>
Reviewed-by: Amitay Isaacs <[hidden email]>

I missed modifying dbwrap code with the latest change regarding calculation
of tdb_flags.
Here are 2 additional patches that are required.

1. Do not pass tdb_flags to db attach controls
2. Ask CTDB for tdb open flags

Please review and include them along with your patches.

Amitay.

dbwrap.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
Hi Amitay,

On Tue, Jul 11, 2017 at 01:06:07AM +1000, Amitay Isaacs wrote:
> Reviewed-by: Amitay Isaacs <[hidden email]>

Thanks!

> I missed modifying dbwrap code with the latest change regarding calculation
> of tdb_flags.
> Here are 2 additional patches that are required.
>
> 1. Do not pass tdb_flags to db attach controls
> 2. Ask CTDB for tdb open flags

afaict 2 result in TDB_SEQNUM not being enabled anymore. Afair this gets
requested on open by the client (ie smbd/dbwrap) and is never enabled in ctdb
without a CTDB_CONTROL_ENABLE_SEQNUM control?

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Tue, Jul 11, 2017 at 2:51 AM, Ralph Böhme <[hidden email]> wrote:

> Hi Amitay,
>
> On Tue, Jul 11, 2017 at 01:06:07AM +1000, Amitay Isaacs wrote:
> > Reviewed-by: Amitay Isaacs <[hidden email]>
>
> Thanks!
>
> > I missed modifying dbwrap code with the latest change regarding
> calculation
> > of tdb_flags.
> > Here are 2 additional patches that are required.
> >
> > 1. Do not pass tdb_flags to db attach controls
> > 2. Ask CTDB for tdb open flags
>
> afaict 2 result in TDB_SEQNUM not being enabled anymore. Afair this gets
> requested on open by the client (ie smbd/dbwrap) and is never enabled in
> ctdb
> without a CTDB_CONTROL_ENABLE_SEQNUM control?
>
>
I wondered about that.  As far as I can see we don't really use the
TDB_SEQNUM feature in CTDB.

Does anyone remember why was it added in the first place?

If we don't need the TDB SEQNUM feature, then I can drop it from CTDB.

Amitay.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Tue, Jul 11, 2017 at 3:40 AM, Amitay Isaacs <[hidden email]> wrote:

> On Tue, Jul 11, 2017 at 2:51 AM, Ralph Böhme <[hidden email]> wrote:
>
>> Hi Amitay,
>>
>> On Tue, Jul 11, 2017 at 01:06:07AM +1000, Amitay Isaacs wrote:
>> > Reviewed-by: Amitay Isaacs <[hidden email]>
>>
>> Thanks!
>>
>> > I missed modifying dbwrap code with the latest change regarding
>> calculation
>> > of tdb_flags.
>> > Here are 2 additional patches that are required.
>> >
>> > 1. Do not pass tdb_flags to db attach controls
>> > 2. Ask CTDB for tdb open flags
>>
>> afaict 2 result in TDB_SEQNUM not being enabled anymore. Afair this gets
>> requested on open by the client (ie smbd/dbwrap) and is never enabled in
>> ctdb
>> without a CTDB_CONTROL_ENABLE_SEQNUM control?
>>
>>
> I wondered about that.  As far as I can see we don't really use the
> TDB_SEQNUM feature in CTDB.
>
> Does anyone remember why was it added in the first place?
>
> If we don't need the TDB SEQNUM feature, then I can drop it from CTDB.
>
> Amitay.
>
Till we figure out if TDB_SEQNUM is required or not, here are the updated
patches that maintain the existing behaviour.

Amitay.

dbwrap.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Jul 11, 2017 at 03:40:13AM +1000, Amitay Isaacs via samba-technical wrote:
> I wondered about that.  As far as I can see we don't really use the
> TDB_SEQNUM feature in CTDB.

ctdb itself probably doesn't, but dbwrap_ctdb users might depend on
the sequence number being bumped up if the database changes.

> Does anyone remember why was it added in the first place?
>
> If we don't need the TDB SEQNUM feature, then I can drop it from CTDB.

It was added in the past for the notify databases, which are gone by
now. But it's a part of the published TDB API, and it would be a
surprise to dbwrap users if it did not work for ctdb managed
databases. Doing a quick grep shows that brlock.tdb uses it. Whether
that is really strictly needed is something I need to investigate, but
it's there.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Tue, Jul 11, 2017 at 3:20 PM, Volker Lendecke <[hidden email]>
wrote:

> On Tue, Jul 11, 2017 at 03:40:13AM +1000, Amitay Isaacs via
> samba-technical wrote:
> > I wondered about that.  As far as I can see we don't really use the
> > TDB_SEQNUM feature in CTDB.
>
> ctdb itself probably doesn't, but dbwrap_ctdb users might depend on
> the sequence number being bumped up if the database changes.
>
> > Does anyone remember why was it added in the first place?
> >
> > If we don't need the TDB SEQNUM feature, then I can drop it from CTDB.
>
> It was added in the past for the notify databases, which are gone by
> now. But it's a part of the published TDB API, and it would be a
> surprise to dbwrap users if it did not work for ctdb managed
> databases. Doing a quick grep shows that brlock.tdb uses it. Whether
> that is really strictly needed is something I need to investigate, but
> it's there.
>

I looked at the logic of updating tdb seqnum cluster-wide and it's ad-hoc.
Since samba can update the record on any node, CTDB has to keep
periodically checking for tdb seqnum changes and then propagate them.

Also, since there is no tdb api to set the seqnum to specific value,
database recovery will reset the seqnum.

Here's the code from brlock.c:

        if (!lp_clustering()) {
                /*
                 * We can't use the SEQNUM trick to cache brlock
                 * entries in the clustering case because ctdb seqnum
                 * propagation has a delay.
                 */
                tdb_flags |= TDB_SEQNUM;
        }

Looks like we don't rely on seqnum with CTDB.

It looks like seqnum is used to find out if the database has changed or not
to infer if the record has changed or not. May be record sequence number
(RSN) can be used with dbwrap_ctdb.

However, there are no monotonic guarantees on RSN since RSN is reset to 0
when the record gets deleted.  But since the record is tracked for changes
only while it exists, may be RSN could work.

Amitay.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Jul 11, 2017 at 04:05:45AM +1000, Amitay Isaacs wrote:
> Till we figure out if TDB_SEQNUM is required or not, here are the updated
> patches that maintain the existing behaviour.

the way tdb_flags is used as an in/out parameter is somewhat hard to follow.

What about the attached patchset that moves all the controls to the caller?

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Wed, Jul 12, 2017 at 5:58 AM, Ralph Böhme <[hidden email]> wrote:

> On Tue, Jul 11, 2017 at 04:05:45AM +1000, Amitay Isaacs wrote:
> > Till we figure out if TDB_SEQNUM is required or not, here are the updated
> > patches that maintain the existing behaviour.
>
> the way tdb_flags is used as an in/out parameter is somewhat hard to
> follow.
>
> What about the attached patchset that moves all the controls to the caller?
>
>
Yeah, this is definitely better.

Reviewed-by: Amitay Isaacs <[hidden email]>

Amitay.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Wed, Jul 12, 2017 at 4:04 PM, Amitay Isaacs <[hidden email]> wrote:

> On Wed, Jul 12, 2017 at 5:58 AM, Ralph Böhme <[hidden email]> wrote:
>
>> On Tue, Jul 11, 2017 at 04:05:45AM +1000, Amitay Isaacs wrote:
>> > Till we figure out if TDB_SEQNUM is required or not, here are the
>> updated
>> > patches that maintain the existing behaviour.
>>
>> the way tdb_flags is used as an in/out parameter is somewhat hard to
>> follow.
>>
>> What about the attached patchset that moves all the controls to the
>> caller?
>>
>>
> Yeah, this is definitely better.
>
>
There was a typo in the patch that moved the calculation of persistent flag.

Fix is attached.

Amitay.

0001-dbwrap_ctdb-Fix-calculation-of-persistent-flag.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Wed, Jul 19, 2017 at 04:31:34PM +1000, Amitay Isaacs wrote:

> On Wed, Jul 12, 2017 at 4:04 PM, Amitay Isaacs <[hidden email]> wrote:
>
> > On Wed, Jul 12, 2017 at 5:58 AM, Ralph Böhme <[hidden email]> wrote:
> >
> >> On Tue, Jul 11, 2017 at 04:05:45AM +1000, Amitay Isaacs wrote:
> >> > Till we figure out if TDB_SEQNUM is required or not, here are the
> >> updated
> >> > patches that maintain the existing behaviour.
> >>
> >> the way tdb_flags is used as an in/out parameter is somewhat hard to
> >> follow.
> >>
> >> What about the attached patchset that moves all the controls to the
> >> caller?
> >>
> >>
> > Yeah, this is definitely better.
> >
> >
> There was a typo in the patch that moved the calculation of persistent flag.

glad you spotted that! Pushed.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Enable TDB mutexes by default in dbwrap and ctdb

Samba - samba-technical mailing list
On Wed, Jul 19, 2017 at 5:06 PM, Ralph Böhme <[hidden email]> wrote:

> On Wed, Jul 19, 2017 at 04:31:34PM +1000, Amitay Isaacs wrote:
> > On Wed, Jul 12, 2017 at 4:04 PM, Amitay Isaacs <[hidden email]> wrote:
> >
> > > On Wed, Jul 12, 2017 at 5:58 AM, Ralph Böhme <[hidden email]> wrote:
> > >
> > >> On Tue, Jul 11, 2017 at 04:05:45AM +1000, Amitay Isaacs wrote:
> > >> > Till we figure out if TDB_SEQNUM is required or not, here are the
> > >> updated
> > >> > patches that maintain the existing behaviour.
> > >>
> > >> the way tdb_flags is used as an in/out parameter is somewhat hard to
> > >> follow.
> > >>
> > >> What about the attached patchset that moves all the controls to the
> > >> caller?
> > >>
> > >>
> > > Yeah, this is definitely better.
> > >
> > >
> > There was a typo in the patch that moved the calculation of persistent
> flag.
>
> glad you spotted that! Pushed.
>

In our nightly tests, ctdb cluster would not start up because winbindd kept
crashing.  Looking at the back trace revealed infinite recursion while
attaching g_lock.  ;-)

Amitay.
Loading...