<!DOCTYPE html><html><head><title></title><style type="text/css">
p.MsoNormal,p.MsoNoSpacing{margin:0}</style></head><body><div style="font-family:Arial;">On Tue, Oct 13, 2020, at 00:44, Ricardo Signes wrote:<br></div><blockquote type="cite" id="qt" style=""><ul><li><div>brong <br></div><ul><li>locking! conversationsdb has locking problems, annotations are "a whole locking nightmare"<br></li><li>there's a lock inversion between JMAP and other calls in the locking of convdb vs. mailboxes<br></li><li>we can change how convdb locking works internally so you can take/release a lock without closing the entire db, add a user lock, then we're done!(?)<br></li><li>…but it's a bunch of work, and that will be Bron's next project for Cyrus<br></li></ul></li></ul></blockquote><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">Some more about this...<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">So... I just reverted my attempt from <a href="https://github.com/cyrusimap/cyrus-imapd/pull/3232">https://github.com/cyrusimap/cyrus-imapd/pull/3232</a> to fix the deadlock problems.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">Here's the root problem:<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">mailbox.c:mailbox_open_advanced() takes a <b>shared NAMELOCK on the mailbox name</b>, then opens the index and first locks the user.conversations file before locking the indexes.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">So far so good.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">BUT: JMAP (and other things which need a long lock) takes a <b>(shared/exclusive) lock on the user.conversations file BEFORE it opens mailboxes</b>.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">This leads to a lock inversion.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">My attempted fix was to release the namelock while locking the conversations - which reverted everything to the order that the JMAP code does. This caused errors because the mailbox could be deleted during that time that the lock is missing, and that broke invarients.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">There are a couple of options:<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;"><b>1) always lock conversations first</b><br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">Refactor mailbox handling to always lock the conversations.db BEFORE it tries to lock the mailbox name.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">This means that we probably need to find a bunch of places in the code where we're doing copy and rename actions and lock one or both of the conversations databases early as well, so the lock is held across the entire action.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;"><b>2) create a new user-level lock for JMAP</b><br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">This would be useful for other things and would stop our current abuse of the conversations.db as a user-level lock. Instead we could open and close it along with mailboxes and commit more frequently, which would give better transaction safety. It does mean yet another lock, which would need to be managed in both mailbox_open_* and anywhere which is currently using the conversations database as a proxy lock.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">...<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">There are pros and cons of both. I'm leaning a bit towards the second one because it allows us to commit the conversations DB along with the mailbox in each case.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">...<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">Here's the log information that led to knowing about this, using <a href="https://github.com/cyrusimap/cyrus-imapd/pull/3179">https://github.com/cyrusimap/cyrus-imapd/pull/3179</a> in a build:<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">2020-10-07T07:30:37.415533-04:00 fastmail1 slotf1t01/sync_client[4156099]: LOCKORDER: 0=<E:6:/slot1/conf/lock/q/*S*10^37^129^209*brong@fastmaildev^com.lock><b> 1=<S:8:/slot1/conf/lock/domain/f/</b><a href="http://fastmaildev.com/b/user/brong.lock"><b>fastmaildev.com/b/user/brong.lock</b></a><b>> 2=<S:10:/slot1/conf/domain/f/</b><a href="http://fastmaildev.com/user/b/brong.conversations"><b>fastmaildev.com/user/b/brong.conversations</b></a><b>></b><br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">2020-10-07T07:30:37.078907-04:00 fastmail1 slotf1t01/httpjmap[4156617]: LOCKORDER: <b>0=<S:11:/slot1/conf/domain/f/</b><a href="http://fastmaildev.com/user/b/brong.conversations"><b>fastmaildev.com/user/b/brong.conversations</b></a><b>> 1=<S:13:/slot1/conf/lock/domain/f/</b><a href="http://fastmaildev.com/b/user/brong.lock"><b>fastmaildev.com/b/user/brong.lock</b></a><b>></b><br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">...<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">So the plan appears to be:<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;"><b>1) do a new user-level lock</b> that is held when any mailbox is opened as well as held for the entire command by JMAP. This will probably be a reuse of the existing user_namespacelock which is used for rename safety.</div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;"><b>2) make conversations lock inside mailboxes</b> - for efficiency this will be done by making it not locked for the entire time it's open, so making it more like every other cyrusdb which can be held open for efficiency and only locked when used. Conversations will be lazyloaded inside mailbox if not already open, and it will just use refcounting like every other DB.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;"><b>3) BONUS: move per-mailbox annotations to only be a mailbox internal thing too</b> - and just like cache, lazyload it when needed.</div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">I'm going to get started on that when I get a chance.<br></div><div style="font-family:Arial;"><br></div><div style="font-family:Arial;">Bron.</div><div style="font-family:Arial;"><br></div><div id="sig56629417"><div>--<br></div><div> Bron Gondwana, CEO, Fastmail Pty Ltd<br></div><div> brong@fastmailteam.com<br></div><div><br></div></div><div style="font-family:Arial;"><br></div></body></html>