RFC: dbwrap_ctdb and empty vs deleted records

Michael Adam obnox at samba.org
Mon Jul 25 08:57:38 UTC 2016


On 2016-07-23 at 13:47 +0200, Ralph Böhme wrote:
> On Fri, Jul 22, 2016 at 11:49:34PM +0200, Ralph Böhme wrote:
> > On Fri, Jul 22, 2016 at 04:42:35PM +0200, Michael Adam wrote:
> > > Thinking aloud:
> > > 
> > > The example in the commit msg is from brlock code.
> > > We'd need to lock a file, release it, have an
> > > empty brl record, and before it gets vacuumed
> > > call do_lock on the file again.
> > > 
> > > So possibly by a long vacuum interval and a specially
> > > crafted sequence of file ops...
> > 
> > hm, my reading of the commit message of the revert was a bit
> > different:
> > 
> >     do_lock()
> >       -> grabs lock on brl record with brl_get_locks()
> >         -> calls brl_lock()
> >           -> calls brl_lock_posix or _windows_default()
> >             -> calls contend_level2_oplocks_begin()
> >               -> calls brl_locks_get_read_only()
> >                 -> calls dbwrap_parse_record on the same brl record as above
> > 
> > This suggest that this can be triggered by a single request calling
> > into do_lock(). Can it?
> 
> no, it can't. Sorry, wooly thinking late at night.
> 
> You need exactly the order of operations you described:
> 
> - lock byterange
> - unlock -> this creates the tombstone record
> - lock byterange again

Yeah, this is exactly what I meant with the above.
(Long vac interval to make sure the tombstone rec
 does not get cleaned in the meantime between
 the operations.)

> I've added a brl test and with that am able to reproduce the deadlock
> when 925625b52886d40b50fc is reverted.

Oh, very cool! thanks for that.
I think we could bring this into master right now as a
first step, but one question: To my understanding, the
patch does not explicitly but only implicitly test the
ctdb/smbd interaction. It is written such that it should
always pass. Hence we do not need to skip it if clustering
is not active... What do you think?

> My patch does not deadlock, so it seems it's ok. Updated patchset that
> contains the test attached.

Will look again.

Cheers - Michael



> The test is not really needed to trigger the deadlock, earlier tests
> in the smb2.lock testsuite will deadlock as well. But having an
> explicit test will serve as documentation of the issue.
> 
> Please review&push if ok.
> 
> Cheerio!
> -slow

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160725/5946491d/signature.sig>


More information about the samba-technical mailing list