[PATCH] avoid lock oder violation between xattr.tdb and g_lock.tdb

[Jeremy Allison]

On Thu, Jul 14, 2016 at 07:52:28AM +0200, Volker Lendecke wrote:
> On Tue, Jul 12, 2016 at 05:12:48PM +0200, Michael Adam wrote:
> > > Not sure this will work. g_lock is a watched database, and if there is
> > > lock contention, this will conflict with dbwrap_watchers.tdb.
> > 
> > Ouch. So without the below-mentioned rework of dbwrap_watch
> > (or another solution), we would need an additional lock oder
> > value(4)?
> 
> Attached find a patchset that survived a private autobuild for me.
> 
> It is incomplete as it does not have the removal patch for the current
> version of dbwrap_record_watch_send. Also, I'm not sure about the
> performance implications of the malloc/memcpy when storing large
> records. But the typical record is normally not too large. The only
> case where this could matter is highly popular share root directories
> in locking.tdb. So sooner than later we might have to add a tdb_storev
> call and the related dbwrap support.
> 
> Comments?

OK, finally loaded

[PATCH 5/9] dbwrap: Add an alternative implementation of dbwrap_watch_record_send

into my brain. That took a while. I can't find any logic flaws in the code,
amazing work ! However it took me a while to wrap my head around. The following
(attached) comments inside this patch would help me (and maybe others)
to understand it (IMHO). Please feel free to include them (modified if
you want).

Secondly, I was going to mention the multiple talloc_realloc() calls inside
dbwrap_watched_wakeup() which gets called on every dbwrap_watched_store()
and dbwrap_watched_delete() and wonder if they could be mitigated by
keeping a num_watchers variable inside the struct db_watched_subrec,
then I realized it only got triggered if the message target had
died and messaging_send_iov() returned NT_STATUS_OBJECT_NAME_NOT_FOUND,
so yeah it's already really efficient for the common case...

So you'd already thought of that :-). Great work !

So this patchset Reviewed-by: Jeremy Allison <jra at samba.org>.

As far as I can see it only needs the removal of dbwrap_record_watch_send()
and associated functions to be good for master ! I didn't cachegrind
test the performance implications due to the changes adding the
server_id arrays, but I think we can work on that later if need
be.
-------------- next part --------------
diff --git a/source3/lib/dbwrap/dbwrap_watch.c b/source3/lib/dbwrap/dbwrap_watch.c
index f2cc381..b8ffb04 100644
--- a/source3/lib/dbwrap/dbwrap_watch.c
+++ b/source3/lib/dbwrap/dbwrap_watch.c
@@ -535,6 +535,30 @@ void dbwrap_watchers_wakeall(struct messaging_context *msg)
 	dbwrap_watchers_traverse_read(dbwrap_wakeall_cb, msg);
 }
 
+/*
+ * Watched records contain a header of:
+ *
+ * [uint32] num_records | deleted bit
+ * 0 [SERVER_ID_BUF_LENGTH]                   \
+ * 1 [SERVER_ID_BUF_LENGTH]                   |
+ * ..                                         |- Array of watchers
+ * (num_records-1)[SERVER_ID_BUF_LENGTH]      /
+ *
+ * [Remainder of record....]
+ *
+ * If this header is absent then this is a
+ * fresh record of length zero (no watchers).
+ *
+ * Note that a record can be deleted with
+ * watchers present. If so the deleted bit
+ * is set and the watcher server_id's are
+ * woken to allow them to remove themselves
+ * from the watcher array. The record is left
+ * present marked with the deleted bit until all
+ * watchers are removed, then the record itself
+ * is deleted.
+ */
+
 #define NUM_WATCHERS_DELETED_BIT (1UL<<31)
 #define NUM_WATCHERS_MASK (NUM_WATCHERS_DELETED_BIT-1)
 
@@ -564,11 +588,22 @@ static ssize_t dbwrap_watched_parse(TDB_DATA data, struct server_id *ids,
 	}
 
 	if (num_watchers > num_ids) {
+		/*
+		 * Not enough space to store the watchers server_id's.
+		 * Just move past all of them to allow the remaining part
+		 * of the record to be returned.
+		 */
 		data.dptr += num_watchers * SERVER_ID_BUF_LENGTH;
 		data.dsize -= num_watchers * SERVER_ID_BUF_LENGTH;
 		goto done;
 	}
 
+	/*
+	 * Note, even if marked deleted we still must
+	 * return the id's array to allow awoken
+	 * watchers to remove themselves.
+	 */
+
 	for (i=0; i<num_watchers; i++) {
 		server_id_get(&ids[i], data.dptr);
 		data.dptr += SERVER_ID_BUF_LENGTH;