[Drbd-dev] [DRBD 8.0 PATCH] Update state processing so that after-state-change is always done in worker thread

[Drbd-dev] [DRBD 8.0 PATCH] Update state processing so that after-state-change is always done in worker thread

[Graham, Simon]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

This patch updates the state processing so that the after state change
processing is always done on the worker thread - my main motivation for
this was to ensure that state change notifications are never re-ordered.


This also involves the following:
1. starting the worker thread is done inline in drbd_set_state
2. the worker will be started whenever it is needed rather than
   only when certain states are reached.
3. Marking the meta data dirty is done inline in drbd_set_state

Simon

[-- Attachment #2: 0003-Update-state-processing-so-that-after-state-change-p.patch --]
[-- Type: application/octet-stream, Size: 25868 bytes --]

From ac58c47d6c998e1f31301b612e69d98b9a7910e8 Mon Sep 17 00:00:00 2001
From: Simon P. Graham <Simon.Graham@stratus.com>
Date: Sun, 23 Dec 2007 16:23:04 -0500
Subject: [PATCH] Update state processing so that after-state-change processing is always done on worker thread

This also involves the following:
1. starting the worker thread is done inline in drbd_set_state
2. the worker will be started whenever it is needed rather than
   only when certain states are reached.
3. Marking the meta data dirty is done inline in drbd_set_state
---
 drbd/drbd_int.h      |   12 +----
 drbd/drbd_main.c     |  143 ++++++++++++++++++++++++-------------------------
 drbd/drbd_nl.c       |   44 +++++++--------
 drbd/drbd_receiver.c |   27 +++------
 drbd/drbd_worker.c   |   17 ++----
 5 files changed, 106 insertions(+), 137 deletions(-)

diff --git a/drbd/drbd_int.h b/drbd/drbd_int.h
index c0379b7..01eeba6 100644
--- a/drbd/drbd_int.h
+++ b/drbd/drbd_int.h
@@ -925,7 +925,6 @@ static inline void drbd_put_data_sock(drbd_dev *mdev)
 enum chg_state_flags {
 	ChgStateHard    = 1,
 	ChgStateVerbose = 2,
-	ScheduleAfter   = 4,
 };
 
 extern int drbd_change_state(drbd_dev* mdev, enum chg_state_flags f,
@@ -935,8 +934,6 @@ extern int _drbd_request_state(drbd_dev*, drbd_state_t, drbd_state_t,
 			       enum chg_state_flags);
 extern int _drbd_set_state(drbd_dev*, drbd_state_t, enum chg_state_flags );
 extern void print_st_err(drbd_dev*, drbd_state_t, drbd_state_t, int );
-extern void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
-			   enum chg_state_flags);
 extern int  drbd_thread_start(struct Drbd_thread *thi);
 extern void _drbd_thread_stop(struct Drbd_thread *thi, int restart, int wait);
 extern void drbd_thread_signal(struct Drbd_thread *thi);
@@ -1473,12 +1470,6 @@ static inline void drbd_state_unlock(drbd_dev *mdev)
 	wake_up(&mdev->misc_wait);
 }
 
-static inline int drbd_request_state(drbd_dev* mdev, drbd_state_t mask,
-				     drbd_state_t val)
-{
-	return _drbd_request_state(mdev, mask, val, ChgStateVerbose);
-}
-
 /**
  * drbd_chk_io_error: Handles the on_io_error setting, should be called from
  * all io completion handlers. See also drbd_io_error().
@@ -1496,8 +1487,7 @@ static inline void __drbd_chk_io_error(drbd_dev* mdev, int forcedetach)
 	case Detach:
 	case CallIOEHelper:
 		if (mdev->state.disk > Failed) {
-			_drbd_set_state(_NS(mdev,disk,Failed),
-					ChgStateHard|ScheduleAfter);
+			_drbd_set_state(_NS(mdev,disk,Failed),ChgStateHard);
 			ERR("Local IO failed. Detaching...\n");
 		}
 		break;
diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
index 984d00f..8d18601 100644
--- a/drbd/drbd_main.c
+++ b/drbd/drbd_main.c
@@ -75,6 +75,8 @@ int drbd_init(void);
 STATIC int drbd_open(struct inode *inode, struct file *file);
 STATIC int drbd_close(struct inode *inode, struct file *file);
 STATIC int w_after_state_ch(drbd_dev *mdev, struct drbd_work *w, int unused);
+STATIC void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
+			   enum chg_state_flags flags);
 STATIC int w_md_sync(drbd_dev *mdev, struct drbd_work *w, int unused);
 STATIC void md_sync_timer_fn(unsigned long data);
 
@@ -332,8 +334,7 @@ int drbd_io_error(drbd_dev* mdev, int forcedetach)
 
 	spin_lock_irqsave(&mdev->req_lock,flags);
 	if( (send = (mdev->state.disk == Failed)) ) {
-		_drbd_set_state(_NS(mdev,disk,Diskless),
-				ChgStateHard|ScheduleAfter);
+		_drbd_set_state(_NS(mdev,disk,Diskless),ChgStateHard);
 	}
 	spin_unlock_irqrestore(&mdev->req_lock,flags);
 
@@ -389,14 +390,13 @@ int drbd_change_state(drbd_dev* mdev, enum chg_state_flags f,
 	rv = _drbd_set_state(mdev, ns, f);
 	ns = mdev->state;
 	spin_unlock_irqrestore(&mdev->req_lock,flags);
-	if (rv==SS_Success && !(f&ScheduleAfter)) after_state_ch(mdev,os,ns,f);
 
 	return rv;
 }
 
 void drbd_force_state(drbd_dev* mdev, drbd_state_t mask, drbd_state_t val)
 {
-	drbd_change_state(mdev,ChgStateHard,mask,val);
+	drbd_change_state(mdev,ChgStateVerbose|ChgStateHard,mask,val);
 }
 
 STATIC int is_valid_state(drbd_dev* mdev, drbd_state_t ns);
@@ -436,7 +436,6 @@ set_st_err_t _req_st_cond(drbd_dev* mdev,drbd_state_t mask, drbd_state_t val)
  * _drbd_request_state:
  * This function is the most gracefull way to change state. For some state
  * transition this function even does a cluster wide transaction.
- * It has a cousin named drbd_request_state(), which is always verbose.
  */
 int _drbd_request_state(drbd_dev* mdev, drbd_state_t mask, drbd_state_t val,
 		       enum chg_state_flags f)
@@ -485,8 +484,6 @@ int _drbd_request_state(drbd_dev* mdev, drbd_state_t mask, drbd_state_t val,
 	ns = mdev->state;
 	spin_unlock_irqrestore(&mdev->req_lock,flags);
 
-	if (rv==SS_Success && !(f&ScheduleAfter)) after_state_ch(mdev,os,ns,f);
-
 	return rv;
 }
 
@@ -608,6 +605,7 @@ int _drbd_set_state(drbd_dev* mdev, drbd_state_t ns,enum chg_state_flags flags)
 	drbd_state_t os;
 	int rv=SS_Success, warn_sync_abort=0;
 	enum fencing_policy fp;
+	struct after_state_chg_work* ascw;
 
 	MUST_HOLD(&mdev->req_lock);
 
@@ -806,11 +804,25 @@ int _drbd_set_state(drbd_dev* mdev, drbd_state_t ns,enum chg_state_flags flags)
 		}
 	}
 
-	if ( os.disk == Diskless && os.conn == StandAlone &&
-	     (ns.disk > Diskless || ns.conn >= Unconnected) ) {
-		int i;
-		i = try_module_get(THIS_MODULE);
-		D_ASSERT(i);
+	if(inc_local(mdev)) {
+		u32 mdf = mdev->bc->md.flags & ~(MDF_Consistent|MDF_PrimaryInd|
+						 MDF_ConnectedInd|MDF_WasUpToDate|
+						 MDF_PeerOutDated );
+
+		if (test_bit(CRASHED_PRIMARY,&mdev->flags) ||
+		    mdev->state.role == Primary ||
+		    ( mdev->state.pdsk < Inconsistent &&
+		      mdev->state.peer == Primary ) )  mdf |= MDF_PrimaryInd;
+		if (mdev->state.conn > WFReportParams) mdf |= MDF_ConnectedInd;
+		if (mdev->state.disk > Inconsistent)   mdf |= MDF_Consistent;
+		if (mdev->state.disk > Outdated)       mdf |= MDF_WasUpToDate;
+		if (mdev->state.pdsk <= Outdated &&
+		    mdev->state.pdsk >= Inconsistent)  mdf |= MDF_PeerOutDated;
+		if( mdf != mdev->bc->md.flags) {
+			mdev->bc->md.flags = mdf;
+			drbd_md_mark_dirty(mdev);
+		}
+		dec_local(mdev);
 	}
 
 	/* Peer was forced UpToDate & Primary, consider to resync */
@@ -818,19 +830,18 @@ int _drbd_set_state(drbd_dev* mdev, drbd_state_t ns,enum chg_state_flags flags)
 	    os.peer == Secondary && ns.peer == Primary)
 		set_bit(CONSIDER_RESYNC, &mdev->flags);
 
-	if( flags & ScheduleAfter ) {
-		struct after_state_chg_work* ascw;
-
-		ascw = kmalloc(sizeof(*ascw), GFP_ATOMIC);
-		if(ascw) {
-			ascw->os = os;
-			ascw->ns = ns;
-			ascw->flags = flags;
-			ascw->w.cb = w_after_state_ch;
-			drbd_queue_work(&mdev->data.work,&ascw->w);
-		} else {
-			WARN("Could not kmalloc an ascw\n");
-		}
+	/* Make sure worker thread is running -- this is a NOP if it is already running */
+	drbd_thread_start(&mdev->worker);
+
+	ascw = kmalloc(sizeof(*ascw), GFP_ATOMIC);
+	if(ascw) {
+	    ascw->os = os;
+	    ascw->ns = ns;
+	    ascw->flags = flags;
+	    ascw->w.cb = w_after_state_ch;
+	    drbd_queue_work(&mdev->data.work,&ascw->w);
+	} else {
+	    WARN("Could not kmalloc an ascw\n");
 	}
 
 	return rv;
@@ -847,11 +858,10 @@ STATIC int w_after_state_ch(drbd_dev *mdev, struct drbd_work *w, int unused)
 	return 1;
 }
 
-void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
-		    enum chg_state_flags flags)
+STATIC void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
+			   enum chg_state_flags flags)
 {
 	enum fencing_policy fp;
-	u32 mdf;
 
 	if ( (os.conn != Connected && ns.conn == Connected) ) {
 		clear_bit(CRASHED_PRIMARY, &mdev->flags);
@@ -863,24 +873,6 @@ void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
 	fp = DontCare;
 	if(inc_local(mdev)) {
 		fp = mdev->bc->dc.fencing;
-
-		mdf = mdev->bc->md.flags & ~(MDF_Consistent|MDF_PrimaryInd|
-					     MDF_ConnectedInd|MDF_WasUpToDate|
-					     MDF_PeerOutDated );
-
-		if (test_bit(CRASHED_PRIMARY,&mdev->flags) ||
-		    mdev->state.role == Primary ||
-		    ( mdev->state.pdsk < Inconsistent &&
-		      mdev->state.peer == Primary ) )  mdf |= MDF_PrimaryInd;
-		if (mdev->state.conn > WFReportParams) mdf |= MDF_ConnectedInd;
-		if (mdev->state.disk > Inconsistent)   mdf |= MDF_Consistent;
-		if (mdev->state.disk > Outdated)       mdf |= MDF_WasUpToDate;
-		if (mdev->state.pdsk <= Outdated &&
-		    mdev->state.pdsk >= Inconsistent)  mdf |= MDF_PeerOutDated;
-		if( mdf != mdev->bc->md.flags) {
-			mdev->bc->md.flags = mdf;
-			drbd_md_mark_dirty(mdev);
-		}
 		dec_local(mdev);
 	}
 
@@ -897,8 +889,7 @@ void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
 		     (os.conn < Connected && ns.conn >= Connected) ) {
 			tl_clear(mdev);
 			spin_lock_irq(&mdev->req_lock);
-			_drbd_set_state(_NS(mdev,susp,0),
-					ChgStateVerbose | ScheduleAfter );
+			_drbd_set_state(_NS(mdev,susp,0),ChgStateVerbose);
 			spin_unlock_irq(&mdev->req_lock);
 		}
 	}
@@ -1005,8 +996,7 @@ void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
 
 		if (ns.conn == StartingSyncT) {
 			spin_lock_irq(&mdev->req_lock);
-			_drbd_set_state(_NS(mdev,conn,WFSyncUUID),
-					ChgStateVerbose | ScheduleAfter );
+			_drbd_set_state(_NS(mdev,conn,WFSyncUUID),ChgStateVerbose);
 			spin_unlock_irq(&mdev->req_lock);
 		} else /* StartingSyncS */ {
 			drbd_start_resync(mdev,SyncSource);
@@ -1053,7 +1043,7 @@ void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
 		drbd_thread_signal(&mdev->receiver);
 	}
 
-	// Now the receiver finished cleaning up itself, it should die now
+	// Now the receiver finished cleaning up itself, it should die
 	if ( os.conn != StandAlone && ns.conn == StandAlone ) {
 		drbd_thread_stop_nowait(&mdev->receiver);
 	}
@@ -1064,25 +1054,14 @@ void after_state_ch(drbd_dev* mdev, drbd_state_t os, drbd_state_t ns,
 		drbd_thread_restart_nowait(&mdev->receiver);
 	}
 
+	// Upon network connection, we need to start the received
 	if ( os.conn == StandAlone && ns.conn == Unconnected) {
 		drbd_thread_start(&mdev->receiver);
 	}
 
-	if ( os.disk == Diskless && os.conn <= Disconnecting &&
-	     (ns.disk > Diskless || ns.conn >= Unconnected) ) {
-		if(!drbd_thread_start(&mdev->worker)) {
-			module_put(THIS_MODULE);
-		}
-	}
-
-	/* FIXME what about Primary, Diskless, and then losing
-	 * the connection? since we survive that "somehow",
-	 * maybe we may not stop the worker yet,
-	 * since that would call drbd_mdev_cleanup.
-	 * after which we probably won't survive the next
-	 * request from the upper layers ... BOOM again :( */
-	if ( (os.disk > Diskless || os.conn > StandAlone) &&
-	     ns.disk == Diskless && ns.conn == StandAlone ) {
+	// Terminate worker thread if we are unconfigured - it will be
+	// restarted as needed...
+	if (ns.disk == Diskless && ns.conn == StandAlone) {
 		drbd_thread_stop_nowait(&mdev->worker);
 	}
 }
@@ -1118,6 +1097,13 @@ STATIC int drbd_thread_setup(void* arg)
 	// THINK maybe two different completions?
 	complete(&thi->startstop); // notify: thi->task unset.
 
+	INFO("Terminating %s thread\n",
+	     thi == &mdev->receiver ? "receiver" :
+	     thi == &mdev->asender  ? "asender"  :
+	     thi == &mdev->worker   ? "worker"   : "NONSENSE");
+
+	// Release mod reference taken when thread was started
+	module_put(THIS_MODULE);
 	return retval;
 }
 
@@ -1138,25 +1124,36 @@ int drbd_thread_start(struct Drbd_thread *thi)
 
 	spin_lock(&thi->t_lock);
 
-	/* INFO("drbd_thread_start: %s [%d]: %s %d -> Running\n",
-	     current->comm, current->pid,
-	     thi == &mdev->receiver ? "receiver" :
-             thi == &mdev->asender  ? "asender"  :
-             thi == &mdev->worker   ? "worker"   : "NONSENSE",
-	     thi->t_state); */
-
 	if (thi->t_state == None) {
+		INFO("Starting %s thread (from %s [%d])\n",
+		     thi == &mdev->receiver ? "receiver" :
+		     thi == &mdev->asender  ? "asender"  :
+		     thi == &mdev->worker   ? "worker"   : "NONSENSE",
+		     current->comm, current->pid);
+
+		// Get ref on module for thread - this is released when thread exits
+		if (!try_module_get(THIS_MODULE)) {
+			ERR("Failed to get module reference in drbd_thread_start\n");
+			spin_unlock(&thi->t_lock);
+			return FALSE;
+		}
+
 		init_completion(&thi->startstop);
 		D_ASSERT(thi->task == NULL);
 		thi->t_state = Running;
 		spin_unlock(&thi->t_lock);
+
 		flush_signals(current); // otherw. may get -ERESTARTNOINTR
 		pid = kernel_thread(drbd_thread_setup, (void *) thi, CLONE_FS);
 		if (pid < 0) {
 			ERR("Couldn't start thread (%d)\n", pid);
+
+			module_put(THIS_MODULE);
 			return FALSE;
 		}
+
 		wait_for_completion(&thi->startstop); // waits until thi->task is set
+
 		D_ASSERT(thi->task);
 		D_ASSERT(get_t_state(thi) == Running);
 	} else {
diff --git a/drbd/drbd_nl.c b/drbd/drbd_nl.c
index acee869..61593ed 100644
--- a/drbd/drbd_nl.c
+++ b/drbd/drbd_nl.c
@@ -194,7 +194,7 @@ drbd_disks_t drbd_try_outdate_peer(drbd_dev *mdev)
 		return mdev->state.pdsk;
 	}
 
-	if( fp == Stonith ) drbd_request_state(mdev,NS(susp,1));
+	if( fp == Stonith ) _drbd_request_state(mdev,NS(susp,1),ChgStateVerbose);
 
 	r=drbd_khelper(mdev,"outdate-peer");
 
@@ -212,7 +212,7 @@ drbd_disks_t drbd_try_outdate_peer(drbd_dev *mdev)
 	case 6: /* Peer is primary, voluntarily outdate myself */
 		WARN("Peer is primary, outdating myself.\n");
 		nps = DUnknown;
-		drbd_request_state(mdev,NS(disk,Outdated));
+		_drbd_request_state(mdev,NS(disk,Outdated),ChgStateVerbose);
 		break;
 	case 7:
 		if( fp != Stonith ) {
@@ -294,13 +294,13 @@ int drbd_set_role(drbd_dev *mdev, drbd_role_t new_role, int force)
 			continue;
 		}
 		if ( r < SS_Success ) {
-			r = drbd_request_state(mdev,mask,val); // Be verbose.
+			r = _drbd_request_state(mdev,mask,val,ChgStateVerbose); // Be verbose.
 			if( r < SS_Success ) goto fail;
 		}
 		break;
 	}
 
-	if(forced) WARN("Forced to conisder local data as UpToDate!\n");
+	if(forced) WARN("Forced to consider local data as UpToDate!\n");
 
 	drbd_sync_me(mdev);
 
@@ -852,7 +852,7 @@ STATIC int drbd_nl_disk_conf(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 
 	nbc->known_size = drbd_get_capacity(nbc->backing_bdev);
 
-	if((retcode = drbd_request_state(mdev,NS(disk,Attaching))) < SS_Success ) {
+	if((retcode = _drbd_request_state(mdev,NS(disk,Attaching),ChgStateVerbose)) < SS_Success ) {
 		goto release_bdev2_fail;
 	}
 
@@ -987,7 +987,7 @@ STATIC int drbd_nl_disk_conf(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 
 	/* All tests on MDF_PrimaryInd, MDF_ConnectedInd,
 	   MDF_Consistent and MDF_WasUpToDate must happen before
-	   this point, because drbd_request_state() modifies these
+	   this point, because _drbd_request_state() modifies these
 	   flags. */
 
 	/* In case we are Connected postpone any desicion on the new disk
@@ -1001,7 +1001,6 @@ STATIC int drbd_nl_disk_conf(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 	rv = _drbd_set_state(mdev, ns, ChgStateVerbose);
 	ns = mdev->state;
 	spin_unlock_irq(&mdev->req_lock);
-	if (rv==SS_Success) after_state_ch(mdev,os,ns,ChgStateVerbose);
 
 	if (rv < SS_Success) {
 		goto unlock_bm;
@@ -1048,7 +1047,7 @@ STATIC int drbd_nl_detach(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 			  struct drbd_nl_cfg_reply *reply)
 {
 	drbd_sync_me(mdev);
-	reply->ret_code = drbd_request_state(mdev,NS(disk,Diskless));
+	reply->ret_code = _drbd_request_state(mdev,NS(disk,Diskless),ChgStateVerbose);
 
 	return 0;
 }
@@ -1226,7 +1225,7 @@ FIXME LGE
 	}
 	mdev->cram_hmac_tfm = tfm;
 
-	retcode = drbd_request_state(mdev,NS(conn,Unconnected));
+	retcode = _drbd_request_state(mdev,NS(conn,Unconnected),ChgStateVerbose);
 
 	reply->ret_code = retcode;
 	return 0;
@@ -1251,9 +1250,8 @@ STATIC int drbd_nl_disconnect(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 	if ( retcode == SS_NothingToDo ) goto done;
 	else if ( retcode == SS_AlreadyStandAlone ) goto done;
 	else if ( retcode == SS_PrimaryNOP ) {
-		// Our statche checking code wants to see the peer outdated.
-		retcode = drbd_request_state(mdev,NS2(conn,Disconnecting,
-						      pdsk,Outdated));
+		// Our state checking code wants to see the peer outdated.
+		retcode = _drbd_request_state(mdev,NS2(conn,Disconnecting,pdsk,Outdated),ChgStateVerbose);
 	} else if (retcode == SS_CW_FailedByPeer) {
 		// The peer probabely wants to see us outdated.
 		retcode = _drbd_request_state(mdev,NS2(conn,Disconnecting,
@@ -1262,7 +1260,7 @@ STATIC int drbd_nl_disconnect(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 			// We are diskless and our peer wants to outdate us.
 			// So, simply go away, and let the peer try to
 			// outdate us with its 'outdate-peer' handler later.
-			retcode = drbd_request_state(mdev,NS(conn,StandAlone));
+			retcode = _drbd_request_state(mdev,NS(conn,StandAlone),ChgStateVerbose);
 		}
 	}
 
@@ -1295,7 +1293,7 @@ void resync_after_online_grow(drbd_dev *mdev)
 	if (iass)
 		drbd_start_resync(mdev,SyncSource);
 	else
-		drbd_request_state(mdev,NS(conn,WFSyncUUID));
+		_drbd_request_state(mdev,NS(conn,WFSyncUUID),ChgStateVerbose);
 }
 
 STATIC int drbd_nl_resize(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
@@ -1428,8 +1426,8 @@ STATIC int drbd_nl_syncer_conf(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 STATIC int drbd_nl_invalidate(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 			      struct drbd_nl_cfg_reply *reply)
 {
-	reply->ret_code = drbd_request_state(mdev,NS2(conn,StartingSyncT,
-						      disk,Inconsistent));
+	reply->ret_code = _drbd_request_state(mdev,NS2(conn,StartingSyncT,disk,Inconsistent),
+					      ChgStateVerbose);
 	return 0;
 }
 
@@ -1437,8 +1435,8 @@ STATIC int drbd_nl_invalidate_peer(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 				   struct drbd_nl_cfg_reply *reply)
 {
 
-	reply->ret_code = drbd_request_state(mdev,NS2(conn,StartingSyncS,
-						      pdsk,Inconsistent));
+	reply->ret_code = _drbd_request_state(mdev,NS2(conn,StartingSyncS,pdsk,Inconsistent),
+					      ChgStateVerbose);
 
 	return 0;
 }
@@ -1448,7 +1446,7 @@ STATIC int drbd_nl_pause_sync(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 {
 	int retcode=NoError;
 
-	if(drbd_request_state(mdev,NS(user_isp,1)) == SS_NothingToDo)
+	if(_drbd_request_state(mdev,NS(user_isp,1),ChgStateVerbose) == SS_NothingToDo)
 		retcode = PauseFlagAlreadySet;
 
 	reply->ret_code = retcode;
@@ -1460,7 +1458,7 @@ STATIC int drbd_nl_resume_sync(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 {
 	int retcode=NoError;
 
-	if(drbd_request_state(mdev,NS(user_isp,0)) == SS_NothingToDo)
+	if(_drbd_request_state(mdev,NS(user_isp,0),ChgStateVerbose) == SS_NothingToDo)
 		retcode = PauseFlagAlreadyClear;
 
 	reply->ret_code = retcode;
@@ -1470,7 +1468,7 @@ STATIC int drbd_nl_resume_sync(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 STATIC int drbd_nl_suspend_io(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 			      struct drbd_nl_cfg_reply *reply)
 {
-	reply->ret_code = drbd_request_state(mdev,NS(susp,1));
+	reply->ret_code = _drbd_request_state(mdev,NS(susp,1),ChgStateVerbose);
 
 	return 0;
 }
@@ -1478,14 +1476,14 @@ STATIC int drbd_nl_suspend_io(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 STATIC int drbd_nl_resume_io(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 			     struct drbd_nl_cfg_reply *reply)
 {
-	reply->ret_code = drbd_request_state(mdev,NS(susp,0));
+	reply->ret_code = _drbd_request_state(mdev,NS(susp,0),ChgStateVerbose);
 	return 0;
 }
 
 STATIC int drbd_nl_outdate(drbd_dev *mdev, struct drbd_nl_cfg_req *nlp,
 			   struct drbd_nl_cfg_reply *reply)
 {
-	reply->ret_code = drbd_request_state(mdev,NS(disk,Outdated));
+	reply->ret_code = _drbd_request_state(mdev,NS(disk,Outdated),ChgStateVerbose);
 	return 0;
 }
 
diff --git a/drbd/drbd_receiver.c b/drbd/drbd_receiver.c
index 6414a5f..08ade49 100644
--- a/drbd/drbd_receiver.c
+++ b/drbd/drbd_receiver.c
@@ -718,7 +718,7 @@ int drbd_connect(drbd_dev *mdev)
 
 	D_ASSERT(!mdev->data.socket);
 
-	if (_drbd_request_state(mdev,NS(conn,WFConnection),0) < SS_Success )
+	if (_drbd_request_state(mdev,NS(conn,WFConnection),ChgStateVerbose) < SS_Success )
 		return -2;
 
 	clear_bit(DISCARD_CONCURRENT, &mdev->flags);
@@ -822,14 +822,14 @@ int drbd_connect(drbd_dev *mdev)
 	if (h <= 0) return h;
 
 	if ( mdev->cram_hmac_tfm ) {
-		/* drbd_request_state(mdev, NS(conn, WFAuth)); */
+		/* _drbd_request_state(mdev, NS(conn, WFAuth), ChgStateVerbose); */
 		if (!drbd_do_auth(mdev)) {
 			ERR("Authentication of peer failed\n");
 			return -1;
 		}
 	}
 
-	if (drbd_request_state(mdev, NS(conn, WFReportParams)) < SS_Success) return 0;
+	if (_drbd_request_state(mdev, NS(conn, WFReportParams),ChgStateVerbose) < SS_Success) return 0;
 
 	sock->sk->sk_sndtimeo = mdev->net_conf->timeout*HZ/10;
 	sock->sk->sk_rcvtimeo = MAX_SCHEDULE_TIMEOUT;
@@ -2300,7 +2300,7 @@ STATIC int receive_sizes(drbd_dev *mdev, Drbd_Header *h)
 
 		if(nconn == conn_mask) return FALSE;
 
-		if(drbd_request_state(mdev,NS(conn,nconn)) < SS_Success) {
+		if(_drbd_request_state(mdev,NS(conn,nconn),ChgStateVerbose) < SS_Success) {
 			drbd_force_state(mdev,NS(conn,Disconnecting));
 			return FALSE;
 		}
@@ -2417,7 +2417,7 @@ STATIC int receive_state(drbd_dev *mdev, Drbd_Header *h)
 {
 	Drbd_State_Packet *p = (Drbd_State_Packet*)h;
 	drbd_conns_t nconn,oconn;
-	drbd_state_t os,ns,peer_state;
+	drbd_state_t ns,peer_state;
 	int rv;
 
 	ERR_IF(h->length != (sizeof(*p)-sizeof(*h))) return FALSE;
@@ -2453,7 +2453,6 @@ STATIC int receive_state(drbd_dev *mdev, Drbd_Header *h)
 	spin_lock_irq(&mdev->req_lock);
 	if( mdev->state.conn != oconn ) goto retry;
 	clear_bit(CONSIDER_RESYNC, &mdev->flags);
-	os = mdev->state;
 	ns.i = mdev->state.i;
 	ns.conn = nconn;
 	ns.peer = peer_state.role;
@@ -2463,7 +2462,7 @@ STATIC int receive_state(drbd_dev *mdev, Drbd_Header *h)
 	   ns.disk == Negotiating ) ns.disk = UpToDate;
 	if((nconn == Connected || nconn == WFBitMapT) &&
 	   ns.pdsk == Negotiating ) ns.pdsk = UpToDate;
-	rv = _drbd_set_state(mdev,ns,ChgStateVerbose | ChgStateHard);
+	rv = _drbd_set_state(mdev,ns,ChgStateVerbose|ChgStateHard);
 	ns = mdev->state;
 	spin_unlock_irq(&mdev->req_lock);
 
@@ -2484,10 +2483,6 @@ STATIC int receive_state(drbd_dev *mdev, Drbd_Header *h)
 		}
 	}
 
-	if (rv==SS_Success) {
-		after_state_ch(mdev,os,ns,ChgStateVerbose | ChgStateHard);
-	}
-
 	mdev->net_conf->want_lose = 0;
 
 	/* FIXME assertion for (gencounts do not diverge) */
@@ -2560,7 +2555,7 @@ STATIC int receive_bitmap(drbd_dev *mdev, Drbd_Header *h)
 	} else if (mdev->state.conn == WFBitMapT) {
 		ok = drbd_send_bitmap(mdev);
 		if (!ok) goto out;
-		ok = drbd_request_state(mdev,NS(conn,WFSyncUUID));
+		ok = _drbd_request_state(mdev,NS(conn,WFSyncUUID),ChgStateVerbose);
 		D_ASSERT( ok == SS_Success );
 	} else {
 		ERR("unexpected cstate (%s) in receive_bitmap\n",
@@ -2802,7 +2797,7 @@ STATIC void drbd_disconnect(drbd_dev *mdev)
 		if( fp >= Resource &&
 		    mdev->state.pdsk >= DUnknown ) {
 			drbd_disks_t nps = drbd_try_outdate_peer(mdev);
-			drbd_request_state(mdev,NS(pdsk,nps));
+			_drbd_request_state(mdev,NS(pdsk,nps),ChgStateVerbose);
 		}
 	}
 
@@ -2813,12 +2808,8 @@ STATIC void drbd_disconnect(drbd_dev *mdev)
 		ns = os;
 		ns.conn = Unconnected;
 		rv=_drbd_set_state(mdev,ns,ChgStateVerbose);
-		ns = mdev->state;
 	}
 	spin_unlock_irq(&mdev->req_lock);
-	if (rv == SS_Success) {
-		after_state_ch(mdev,os,ns,ChgStateVerbose);
-	}
 
 	if(os.conn == Disconnecting) {
 		wait_event( mdev->misc_wait,atomic_read(&mdev->net_cnt) == 0 );
@@ -2839,7 +2830,7 @@ STATIC void drbd_disconnect(drbd_dev *mdev)
 		}
 		kfree(mdev->net_conf);
 		mdev->net_conf=NULL;
-		drbd_request_state(mdev, NS(conn,StandAlone));
+		_drbd_request_state(mdev, NS(conn,StandAlone),ChgStateVerbose);
 	}
 
 	/* they do trigger all the time.
diff --git a/drbd/drbd_worker.c b/drbd/drbd_worker.c
index 1cb9637..a832f73 100644
--- a/drbd/drbd_worker.c
+++ b/drbd/drbd_worker.c
@@ -557,9 +557,7 @@ int drbd_resync_finished(drbd_dev* mdev)
 
 	drbd_bm_recount_bits(mdev);
 
-	drbd_request_state(mdev,NS3(conn,Connected,
-				    disk,dstate,
-				    pdsk,pdstate));
+	_drbd_request_state(mdev,NS3(conn,Connected,disk,dstate,pdsk,pdstate),ChgStateVerbose);
 
 	drbd_md_sync(mdev);
 
@@ -813,8 +811,7 @@ STATIC int _drbd_pause_after(drbd_dev *mdev)
 		if (!(odev = minor_to_mdev(i)) ||
 		    (odev->state.conn == StandAlone && odev->state.disk == Diskless) ) continue;
 		if (! _drbd_may_sync_now(odev)) {
-			rv |= ( _drbd_set_state(_NS(odev,aftr_isp,1),
-						ChgStateHard|ScheduleAfter)
+			rv |= ( _drbd_set_state(_NS(odev,aftr_isp,1),ChgStateHard)
 				!= SS_NothingToDo ) ;
 		}
 	}
@@ -837,8 +834,7 @@ STATIC int _drbd_resume_next(drbd_dev *mdev)
 		if( !(odev = minor_to_mdev(i)) ) continue;
 		if ( odev->state.aftr_isp ) {
 			if (_drbd_may_sync_now(odev)) {
-				rv |= ( _drbd_set_state(_NS(odev,aftr_isp,0),
-							ChgStateHard|ScheduleAfter)
+				rv |= ( _drbd_set_state(_NS(odev,aftr_isp,0),ChgStateHard)
 					!= SS_NothingToDo ) ;
 			}
 		}
@@ -885,7 +881,7 @@ void drbd_alter_sa(drbd_dev *mdev, int na)
  */
 void drbd_start_resync(drbd_dev *mdev, drbd_conns_t side)
 {
-	drbd_state_t os,ns;
+	drbd_state_t ns;
 	int r=0;
 
 	MTRACE(TraceTypeResync, TraceLvlSummary,
@@ -911,7 +907,7 @@ void drbd_start_resync(drbd_dev *mdev, drbd_conns_t side)
 	}
 
 	drbd_global_lock();
-	ns = os = mdev->state;
+	ns = mdev->state;
 
 	ns.aftr_isp = !_drbd_may_sync_now(mdev);
 
@@ -938,8 +934,6 @@ void drbd_start_resync(drbd_dev *mdev, drbd_conns_t side)
 	drbd_global_unlock();
 
 	if ( r == SS_Success ) {
-		after_state_ch(mdev,os,ns,ChgStateVerbose);
-
 		INFO("Began resync as %s (will sync %lu KB [%lu bits set]).\n",
 		     conns_to_name(ns.conn),
 		     (unsigned long) mdev->rs_total << (BM_BLOCK_SIZE_B-10),
@@ -1053,7 +1047,6 @@ int drbd_worker(struct Drbd_thread *thi)
 
 	D_ASSERT( mdev->state.disk == Diskless && mdev->state.conn == StandAlone );
 	drbd_mdev_cleanup(mdev);
-	module_put(THIS_MODULE);
 
 	INFO("worker terminated\n");
 
-- 
1.5.4.rc1

Re: [Drbd-dev] [DRBD 8.0 PATCH] Update state processing so that after-state-change is always done in worker thread

[Lars Ellenberg]

On Fri, Jan 11, 2008 at 10:22:15AM -0500, Graham, Simon wrote:
> This patch updates the state processing so that the after state change
> processing is always done on the worker thread - my main motivation for
> this was to ensure that state change notifications are never re-ordered.
> 
> 
> This also involves the following:
> 1. starting the worker thread is done inline in drbd_set_state
> 2. the worker will be started whenever it is needed rather than
>    only when certain states are reached.
> 3. Marking the meta data dirty is done inline in drbd_set_state

this needs some time to get merged,
but has been on the list for a few weeks already.


-- 
: Lars Ellenberg                            Tel +43-1-8178292-55 :
: LINBIT Information Technologies GmbH      Fax +43-1-8178292-82 :
: Vivenotgasse 48, A-1120 Vienna/Europe    http://www.linbit.com :

Re: [Drbd-dev] [DRBD 8.0 PATCH] Update state processing so that after-state-change is always done in worker thread

[Philipp Reisner]

Am Freitag, 11. Januar 2008 16:22:15 schrieb Graham, Simon:
> This patch updates the state processing so that the after state change
> processing is always done on the worker thread - my main motivation for
> this was to ensure that state change notifications are never re-ordered.
>
>
> This also involves the following:
> 1. starting the worker thread is done inline in drbd_set_state
> 2. the worker will be started whenever it is needed rather than
>    only when certain states are reached.
> 3. Marking the meta data dirty is done inline in drbd_set_state
>
> Simon

Hi Simon,

I definitely want to pick up that one, here are my notes from my
first review:

> @@ -818,19 +830,18 @@ int _drbd_set_state(drbd_dev* mdev, drbd_state_t
> ns,enum chg_state_flags flags) os.peer == Secondary && ns.peer == Primary)
>  		set_bit(CONSIDER_RESYNC, &mdev->flags);
>
> -	if( flags & ScheduleAfter ) {
> -		struct after_state_chg_work* ascw;
> -
> -		ascw = kmalloc(sizeof(*ascw), GFP_ATOMIC);
> -		if(ascw) {
> -			ascw->os = os;
> -			ascw->ns = ns;
> -			ascw->flags = flags;
> -			ascw->w.cb = w_after_state_ch;
> -			drbd_queue_work(&mdev->data.work,&ascw->w);
> -		} else {
> -			WARN("Could not kmalloc an ascw\n");
> -		}
> +	/* Make sure worker thread is running -- this is a NOP if it is already running */ 
> +	drbd_thread_start(&mdev->worker); 
> +

It is not allowed to call drbd_thread_start() while we are in _drbd_set_state(),
because drbd_thread_start() uses a sleeping function (wait_for_completion()).
And _drbd_set_state() is running with the req_lock hold.

But I think that can be easily took out of _drbd_set_state(), and done in the
context of the netlink connector callbacks.

I will prepare that...

-Phil
-- 
: Dipl-Ing Philipp Reisner                      Tel +43-1-8178292-50 :
: LINBIT Information Technologies GmbH          Fax +43-1-8178292-82 :
: Vivenotgasse 48, 1120 Vienna, Austria        http://www.linbit.com :

RE: [Drbd-dev] [DRBD 8.0 PATCH] Update state processing so that after-state-change is always done in worker thread

[Graham, Simon]

Phil,

> It is not allowed to call drbd_thread_start() while we are in
> _drbd_set_state(),
> because drbd_thread_start() uses a sleeping function
> (wait_for_completion()).
> And _drbd_set_state() is running with the req_lock hold.
> 
> But I think that can be easily took out of _drbd_set_state(), and done
> in the
> context of the netlink connector callbacks.
> 

Whilst this is definitely a good idea, I convinced myself this would not
be a problem because we should never actually have to start the thread
in the context of a call from drbd_set_state - it should already be
running in this case in which case drbd_start_thread is a NOP.

Simon