[Drbd-dev] PATCH: Missing state change netlink events

[Drbd-dev] PATCH: Missing state change netlink events

[Graham, Simon]

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

 <<ava-3834.patch>> We have noticed that not all state changes are being
reported via netlink (as seen with the 'drbdsetup /dev/drbd0 events -a
-u' command) - the reason for this is that the state broadcast is done
in after_state_ch using the _current_ mdev state when it runs - it's
entirely possible for two state changes to occur in quick succession
before the after_state_ch worker runs which results in the same state
being broadcast twice - this is especially a problem if you are looking
to see disk Failed state changes as the state tends to change to Failed
and then to Diskless very quickly, which can result in the Diskless
state being reported twice.

I think the fix is simple - modify the code so that after_state_ch
broadcasts the new state rather than the current state (patch attached)
- I would have removed the passing of mdev completely from
drbd_bcast_state() if it weren't for the fact it is required by the
generated to_tags function. I'm testing this at the moment but it's a
little hard to be sure it's right since this requires fairly tight
timing.

Simon


[-- Attachment #2: ava-3834.patch --]
[-- Type: application/octet-stream, Size: 1581 bytes --]

Index: src/drbd/drbd_nl.c
===================================================================
--- src/drbd/drbd_nl.c	(revision 20610)
+++ src/drbd/drbd_nl.c	(working copy)
@@ -1688,7 +1688,7 @@
 
 atomic_t drbd_nl_seq = ATOMIC_INIT(2); // two.
 
-void drbd_bcast_state(drbd_dev *mdev)
+void drbd_bcast_state(drbd_dev *mdev, drbd_state_t state)
 {
 	char buffer[sizeof(struct cn_msg)+
 		    sizeof(struct drbd_nl_cfg_reply)+
@@ -1700,7 +1700,7 @@
 
 	// WARN("drbd_bcast_state() got called\n");
 
-	tl = get_state_to_tags(mdev,(struct get_state*)&mdev->state,tl);
+	tl = get_state_to_tags(mdev,(struct get_state*)&state,tl);
 	*tl++ = TT_END; /* Close the tag list */
 
 	cn_reply->id.idx = CN_IDX_DRBD;
Index: src/drbd/drbd_main.c
===================================================================
--- src/drbd/drbd_main.c	(revision 20610)
+++ src/drbd/drbd_main.c	(working copy)
@@ -872,7 +872,7 @@
 	}
 
 	/* Inform userspace about the change... */
-	drbd_bcast_state(mdev);
+	drbd_bcast_state(mdev, ns);
 
 	/* Here we have the actions that are performed after a
 	   state change. This function might sleep */
Index: src/drbd/drbd_int.h
===================================================================
--- src/drbd/drbd_int.h	(revision 20610)
+++ src/drbd/drbd_int.h	(working copy)
@@ -1415,7 +1415,7 @@
 
 void drbd_nl_cleanup(void);
 int __init drbd_nl_init(void);
-void drbd_bcast_state(drbd_dev *mdev);
+void drbd_bcast_state(drbd_dev *mdev, drbd_state_t state);
 void drbd_bcast_sync_progress(drbd_dev *mdev);
 void drbd_bcast_split_brain(drbd_dev *mdev,int flag);
 

Re: [Drbd-dev] PATCH: Missing state change netlink events

[Philipp Reisner]

On Sunday 04 November 2007 22:46:47 Graham, Simon wrote:
>  <<ava-3834.patch>> We have noticed that not all state changes are being
> reported via netlink (as seen with the 'drbdsetup /dev/drbd0 events -a
> -u' command) - the reason for this is that the state broadcast is done
> in after_state_ch using the _current_ mdev state when it runs - it's
> entirely possible for two state changes to occur in quick succession
> before the after_state_ch worker runs which results in the same state
> being broadcast twice - this is especially a problem if you are looking
> to see disk Failed state changes as the state tends to change to Failed
> and then to Diskless very quickly, which can result in the Diskless
> state being reported twice.
>
> I think the fix is simple - modify the code so that after_state_ch
> broadcasts the new state rather than the current state (patch attached)
> - I would have removed the passing of mdev completely from
> drbd_bcast_state() if it weren't for the fact it is required by the
> generated to_tags function. I'm testing this at the moment but it's a
> little hard to be sure it's right since this requires fairly tight
> timing.
>

Hi Simon,

You are right. Obviously.
I have put it into GIT.

-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] PATCH: Missing state change netlink events

[Graham, Simon]

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

> > I think the fix is simple - modify the code so that after_state_ch
> > broadcasts the new state rather than the current state (patch
> attached)
> > - I would have removed the passing of mdev completely from
> > drbd_bcast_state() if it weren't for the fact it is required by the
> > generated to_tags function. I'm testing this at the moment but it's
a
> > little hard to be sure it's right since this requires fairly tight
> > timing.
> >
> 
> Hi Simon,
> 
> You are right. Obviously.
> I have put it into GIT.
>

Actually, this patch wasn't quite complete -- there were a couple of
places where the wrong new state was broadcast (where ScheduleAfter was
_not_ set in the call to _drbd_set_state). Patch attached.

Simon

 

[-- Attachment #2: ava-3847.patch --]
[-- Type: application/octet-stream, Size: 654 bytes --]

Index: src/drbd/drbd_receiver.c
===================================================================
--- src/drbd/drbd_receiver.c	(revision 20661)
+++ src/drbd/drbd_receiver.c	(working copy)
@@ -2431,6 +2431,7 @@
 	if((nconn == Connected || nconn == WFBitMapT) &&
 	   ns.pdsk == Negotiating ) ns.pdsk = UpToDate;
 	rv = _drbd_set_state(mdev,ns,ChgStateVerbose | ChgStateHard);
+	ns = mdev->state;
 	spin_unlock_irq(&mdev->req_lock);
 
 	if(rv < SS_Success) {
@@ -2778,6 +2779,7 @@
 		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) {

Re: [Drbd-dev] PATCH: Missing state change netlink events

[Philipp Reisner]

On Tuesday 06 November 2007 13:41:34 Graham, Simon wrote:
> > > I think the fix is simple - modify the code so that after_state_ch
> > > broadcasts the new state rather than the current state (patch
> >
> > attached)
> >
> > > - I would have removed the passing of mdev completely from
> > > drbd_bcast_state() if it weren't for the fact it is required by the
> > > generated to_tags function. I'm testing this at the moment but it's
>
> a
>
> > > little hard to be sure it's right since this requires fairly tight
> > > timing.
> >
> > Hi Simon,
> >
> > You are right. Obviously.
> > I have put it into GIT.
>
> Actually, this patch wasn't quite complete -- there were a couple of
> places where the wrong new state was broadcast (where ScheduleAfter was
> _not_ set in the call to _drbd_set_state). Patch attached.
>

Right.

-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 :