[Drbd-dev] Panic in _drbd_send_page() again.

[Drbd-dev] Panic in _drbd_send_page() again.

[Montrose, Ernest]

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

Hi all,
I think we have seen this panic before.  I am not sure if there was a
fix then a regression of sort.
I am looking for a clue to help narrow this down. It seems a
bvec->bv_page is freed and is being used later.  Here is the panic
message:
 
Apr 21 05:05:53 chogs kernel: CPU:    0
Apr 21 05:05:53 chogs kernel: EIP:    0061:[<ee2853c1>]    Tainted: GF
VLI
Apr 21 05:05:53 chogs kernel: EFLAGS: 00010296  (2.6.16.38-xen #1)
Apr 21 05:05:53 chogs kernel: EIP is at _drbd_send_page+0x21/0x120
[drbd]
Apr 21 05:05:53 chogs kernel: eax: ffffffff  ebx: c05958fc  ecx:
eb024000  edx: 6b6b6b6b
Apr 21 05:05:53 chogs kernel: esi: 6b6b6b6b  edi: a56b6b6b  ebp:
eb025eec  esp: eb025ebc
Apr 21 05:05:53 chogs kernel: ds: 007b  es: 007b  ss: 0069
Apr 21 05:05:53 chogs kernel: Process drbd3_worker (pid: 5739,
threadinfo=eb024000 task=ed440570)
Apr 21 05:05:53 chogs kernel: Stack: <0>00001000 00004000 00000000
eb024000 00000020 ec64b26c ffffffff 6b6b6b6b
Apr 21 05:05:53 chogs kernel:        ec64b26c c05958fc 00000003 c7a5c6d0
eb025f08 ee2854f3 6b6b6b6b ec64b26c
Apr 21 05:05:54 chogs kernel:        eb025f30 00000000 ec64b26c eb025f5c
ee285643 00000020 00008000 00000000
Apr 21 05:05:54 chogs kernel: Call Trace:
Apr 21 05:05:54 chogs kernel:  [<c0105a01>] show_stack_log_lvl+0xa1/0xe0
Apr 21 05:05:54 chogs kernel:  [<c0105bf1>] show_registers+0x181/0x200
Apr 21 05:05:54 chogs kernel:  [<c0105e10>] die+0x100/0x1b0
Apr 21 05:05:54 chogs kernel:  [<c01168f6>] do_page_fault+0x3c6/0x8c1
Apr 21 05:05:54 chogs kernel:  [<c010565f>] error_code+0x2b/0x30
Apr 21 05:05:54 chogs kernel:  [<ee2854f3>] _drbd_send_zc_bio+0x33/0x50
[drbd]
Apr 21 05:05:55 chogs kernel:  [<ee285643>] drbd_send_dblock+0x133/0x1f0
[drbd]
Apr 21 05:05:55 chogs kernel:  [<ee2703da>] w_send_dblock+0x1a/0xe0
[drbd]
Apr 21 05:05:55 chogs kernel:  [<ee270e5e>] drbd_worker+0x2de/0x4b5
[drbd]
Apr 21 05:05:55 chogs kernel:  [<ee283f8c>] drbd_thread_setup+0x8c/0x100
[drbd]
Apr 21 05:05:55 chogs kernel:  [<c0103485>]
kernel_thread_helper+0x5/0x10
Apr 21 05:05:55 chogs kernel: Code: 5d f4 8b 7d fc 89 ec 5d c3 90 55 89
e5 57 89 cf b9 00 e0 ff ff 56 53 83 ec 24 8b 75 08 89 45 f0 89 55 ec 21
e1 8b 41 18 89 45 e8 <8b> 02 f6 c4 40 74 03 8b 52 0c 8b 42 04 40 85 c0
7e 09 8b 55 ec
Apr 21 05:05:55 chogs kernel:  <0>Fatal exception: panic in 5 seconds
 
Thanks,
 
EM--

[-- Attachment #2: Type: text/html, Size: 9146 bytes --]

Re: [Drbd-dev] Panic in _drbd_send_page() again.

[Philipp Reisner]

Am Donnerstag, 26. April 2007 23:58 schrieb Montrose, Ernest:
> Hi all,
> I think we have seen this panic before.  I am not sure if there was a
> fix then a regression of sort.
> I am looking for a clue to help narrow this down. It seems a
> bvec->bv_page is freed and is being used later.  Here is the panic
> message:
>

Hi Ernest,

Could you run this through ksymoops, to get the code line
disassembled ? -- This does not look like a well-known distri kernel,
or is it ?

-Phil

05:05:53 chogs kernel: CPU:    0
05:05:53 chogs kernel: EIP:    0061:[<ee2853c1>]    Tainted: GF VLI
05:05:53 chogs kernel: EFLAGS: 00010296  (2.6.16.38-xen #1)
05:05:53 chogs kernel: EIP is at _drbd_send_page+0x21/0x120 [drbd]
05:05:53 chogs kernel: eax: ffffffff  ebx: c05958fc  ecx: eb024000  edx: 6b6b6b6b
05:05:53 chogs kernel: esi: 6b6b6b6b  edi: a56b6b6b  ebp: eb025eec  esp: eb025ebc
05:05:53 chogs kernel: ds: 007b  es: 007b  ss: 0069
05:05:53 chogs kernel: Process drbd3_worker (pid: 5739, threadinfo=eb024000 task=ed440570)
05:05:53 chogs kernel: Stack: <0>00001000 00004000 00000000 eb024000 00000020 ec64b26c ffffffff 6b6b6b6b
05:05:53 chogs kernel:        ec64b26c c05958fc 00000003 c7a5c6d0 eb025f08 ee2854f3 6b6b6b6b ec64b26c
05:05:54 chogs kernel:        eb025f30 00000000 ec64b26c eb025f5c ee285643 00000020 00008000 00000000
05:05:54 chogs kernel: Call Trace:
05:05:54 chogs kernel:  [<c0105a01>] show_stack_log_lvl+0xa1/0xe0
05:05:54 chogs kernel:  [<c0105bf1>] show_registers+0x181/0x200
05:05:54 chogs kernel:  [<c0105e10>] die+0x100/0x1b0
05:05:54 chogs kernel:  [<c01168f6>] do_page_fault+0x3c6/0x8c1
05:05:54 chogs kernel:  [<c010565f>] error_code+0x2b/0x30
05:05:54 chogs kernel:  [<ee2854f3>] _drbd_send_zc_bio+0x33/0x50 [drbd]
05:05:55 chogs kernel:  [<ee285643>] drbd_send_dblock+0x133/0x1f0 [drbd]
05:05:55 chogs kernel:  [<ee2703da>] w_send_dblock+0x1a/0xe0 [drbd]
05:05:55 chogs kernel:  [<ee270e5e>] drbd_worker+0x2de/0x4b5 [drbd]
05:05:55 chogs kernel:  [<ee283f8c>] drbd_thread_setup+0x8c/0x100 [drbd]
05:05:55 chogs kernel:  [<c0103485>] kernel_thread_helper+0x5/0x10
05:05:55 chogs kernel: Code: 5d f4 8b 7d fc 89 ec 5d c3 90 55 89 e5 57 89 cf b9 00 e0 ff ff 56 53 83 ec 24 8b 75 08 89 45 f0 89 55 ec 21 e1 8b 41 18 89 45 e8 <8b> 02 f6 c4 40 74 03 8b 52 0c 8b 42 04 40 85 c0 7e 09 8b 55 ec
05:05:55 chogs kernel:  <0>Fatal exception: panic in 5 seconds

-- 
: 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] Panic in _drbd_send_page() again.

[Graham, Simon]

> Hi Ernest,
> 
> Could you run this through ksymoops, to get the code line
> disassembled ? -- This does not look like a well-known distri kernel,
> or is it ?
> 
> -Phil

This is running 2.6.16.38 with Xen 3.0.4 patches -- it's way too late to
run ksymoops on this specific case (the system has long since moved on!)
However, looking at the disassembly for
_debd_send_page:

00003df0 <_drbd_send_page>:
    3df0:	55                   	push   %ebp
    3df1:	89 e5                	mov    %esp,%ebp
    3df3:	57                   	push   %edi
    3df4:	89 cf                	mov    %ecx,%edi
    3df6:	b9 00 e0 ff ff       	mov    $0xffffe000,%ecx
    3dfb:	56                   	push   %esi
    3dfc:	53                   	push   %ebx
    3dfd:	83 ec 24             	sub    $0x24,%esp
    3e00:	8b 75 08             	mov    0x8(%ebp),%esi
    3e03:	89 45 f0             	mov    %eax,0xfffffff0(%ebp)
    3e06:	89 55 ec             	mov    %edx,0xffffffec(%ebp)
    3e09:	21 e1                	and    %esp,%ecx
    3e0b:	8b 41 18             	mov    0x18(%ecx),%eax
    3e0e:	89 45 e8             	mov    %eax,0xffffffe8(%ebp)
    3e11:	8b 02                	mov    (%edx),%eax
    3e13:	f6 c4 40             	test   $0x40,%ah
    3e16:	74 03                	je     3e1b
<_drbd_send_page+0x2b>
    3e18:	8b 52 0c             	mov    0xc(%edx),%edx
    3e1b:	8b 42 04             	mov    0x4(%edx),%eax
    3e1e:	40                   	inc    %eax
    3e1f:	85 c0                	test   %eax,%eax

send_page+0x21 is 3e11 - loading from %edx which is 2nd param, page; 
this is attempting to read the flags field as part of the page_count()
macro call:

int _drbd_send_page(drbd_dev *mdev, struct page *page,
		    int offset, size_t size)
{
	mm_segment_t oldfs = get_fs();
	int sent,ok;
	int len   = size;

#ifdef SHOW_SENDPAGE_USAGE
...
#endif

	/* PARANOIA. if this ever triggers,
	 * something in the layers above us is really kaputt.
	 *one roundtrip later:
	 * doh. it triggered. so XFS _IS_ really kaputt ...
	 * oh well...
	 */
	if ( (page_count(page) < 1) || PageSlab(page) ) {
		/* e.g. XFS meta- & log-data is in slab pages, which
have a

The Oops message shows that edx is 0x6b6b6b6b which is the poison value
for
free memory when using CONFIG_SLAB_DEBUG -- as Ernest pointed out, this 
means that we're using a bio that has already been freed...

This is very similar to a bunch of problems found previously where the
bio
was freed too early -- the big difference with this one is that we
turned
on CONFIG_SLAB_DEBUG which enabled poisoning of freed memory...

Simon

Re: [Drbd-dev] Panic in _drbd_send_page() again.

[Lars Ellenberg]

On Wed, May 02, 2007 at 10:06:44AM -0400, Graham, Simon wrote:
> > Hi Ernest,
> > 
> > Could you run this through ksymoops, to get the code line
> > disassembled ? -- This does not look like a well-known distri kernel,
> > or is it ?
> > 
> > -Phil
> 
> This is running 2.6.16.38 with Xen 3.0.4 patches -- it's way too late to
> run ksymoops on this specific case (the system has long since moved on!)
> However, looking at the disassembly for
> _debd_send_page:


> send_page+0x21 is 3e11 - loading from %edx which is 2nd param, page; 
> this is attempting to read the flags field as part of the page_count()
> macro call:
> 
> int _drbd_send_page(drbd_dev *mdev, struct page *page,
> 		    int offset, size_t size)
> {
> 	mm_segment_t oldfs = get_fs();
> 	int sent,ok;
> 	int len   = size;
> 
> #ifdef SHOW_SENDPAGE_USAGE
> ...
> #endif
> 
> 	/* PARANOIA. if this ever triggers,
> 	 * something in the layers above us is really kaputt.
> 	 *one roundtrip later:
> 	 * doh. it triggered. so XFS _IS_ really kaputt ...
> 	 * oh well...
> 	 */
> 	if ( (page_count(page) < 1) || PageSlab(page) ) {
> 		/* e.g. XFS meta- & log-data is in slab pages, which
> have a
> 
> The Oops message shows that edx is 0x6b6b6b6b which is the poison
> value for free memory when using CONFIG_SLAB_DEBUG -- as Ernest
> pointed out, this means that we're using a bio that has already been
> freed...
> 
> This is very similar to a bunch of problems found previously where the
> bio was freed too early -- the big difference with this one is that we
> turned on CONFIG_SLAB_DEBUG which enabled poisoning of freed memory...

all pieces of information we have about this seem to indicate that the
xen block device code builds up its own bios, tries to be smart there...

and possibly outsmarts itself.

not that I'm too much into pointing fingers,
but I suggest to not only look at drbd, also look closely at the part of
xen that builds up the bios that then are submitted to drbd.
maybe they are not sufficiently initialized, or drbd has otherwise wrong
expectations about bios that do no longer hold true for the bios
submitted by xen.

-- 
: Lars Ellenberg                            Tel +43-1-8178292-0  :
: LINBIT Information Technologies GmbH      Fax +43-1-8178292-82 :
: Vivenotgasse 48, A-1120 Vienna/Europe    http://www.linbit.com :
__
please use the "List-Reply" function of your email client.

RE: [Drbd-dev] Panic in _drbd_send_page() again.

[Graham, Simon]

> 
> all pieces of information we have about this seem to indicate that the
> xen block device code builds up its own bios, tries to be smart
> there...
> 
> and possibly outsmarts itself.
> 

It's of course possible and we're looking at it but it's actually a
pretty standard piece of code that builds the bio and I don't see any
trickiness in it.

I also have a theory on the cause of this -- it's another tiny timing
window I think similar to ones we fixed earlier where the ack for a
packet would be received whilst we were still processing inside
drbd_send_zc_bio -- here's my hypothesis:

1. We're in drbd_send_zc_bio, we've sent the last segment but have not
yet looped back to
   the top of the loop to __bio_for_each_segment.
2. Ack arrives for last segment - clears RQ_NET_PENDING
3. Local IO completes, clears RQ_LOCAL_PENDING and calls req_may_be_done
==> completes bio
   because both RQ_NET_PENDING and RQ_LOCAL_PENDING are clear.

NOW we come back to the thread running drbd_send_zc_bio and the bio has
been freed... KABLOOIE!

I realize this is a very small window but, as the saying goes, where
there's a window there's a bug...

Seems to me that req_may_be_done should not complete the master bio
unless RQ_NET_SENT is set... maybe the completed_ok: case in req_mod
should test this similar to what is done in recv_acked_by_peer:...
although it seems to me that this test should actually be buried in
req_may_be_done since if this flag is not set, the request is not done!

Simon

Re: [Drbd-dev] Panic in _drbd_send_page() again.

[Lars Ellenberg]

On Fri, May 04, 2007 at 10:37:32AM -0400, Graham, Simon wrote:
> > 
> > all pieces of information we have about this seem to indicate that the
> > xen block device code builds up its own bios, tries to be smart
> > there...
> > 
> > and possibly outsmarts itself.
> > 
> 
> It's of course possible and we're looking at it but it's actually a
> pretty standard piece of code that builds the bio and I don't see any
> trickiness in it.
> 
> I also have a theory on the cause of this -- it's another tiny timing
> window I think similar to ones we fixed earlier where the ack for a
> packet would be received whilst we were still processing inside
> drbd_send_zc_bio -- here's my hypothesis:
> 
> 1. We're in drbd_send_zc_bio, we've sent the last segment but have not
> yet looped back to
>    the top of the loop to __bio_for_each_segment.
> 2. Ack arrives for last segment - clears RQ_NET_PENDING
> 3. Local IO completes, clears RQ_LOCAL_PENDING and calls req_may_be_done
> ==> completes bio
>    because both RQ_NET_PENDING and RQ_LOCAL_PENDING are clear.
> 
> NOW we come back to the thread running drbd_send_zc_bio and the bio has
> been freed... KABLOOIE!
> 
> I realize this is a very small window but, as the saying goes, where
> there's a window there's a bug...

hm. this does make sense, actually :)

> Seems to me that req_may_be_done should not complete the master bio
> unless RQ_NET_SENT is set... maybe the completed_ok: case in req_mod
> should test this similar to what is done in recv_acked_by_peer:...
> although it seems to me that this test should actually be buried in
> req_may_be_done since if this flag is not set, the request is not done!


so what you suggest is:

Index: drbd_req.c
===================================================================
--- drbd_req.c	(revision 2864)
+++ drbd_req.c	(working copy)
@@ -255,6 +255,16 @@
 	print_rq_state(req, "_req_may_be_done");
 	MUST_HOLD(&mdev->req_lock)
 
+	/* we must not complete the master bio, while it is
+	 *	still being processed by _drbd_send_zc_bio (drbd_send_dblock)
+	 *	not yet acknowledged by the peer
+	 *	not yet completed by the local io subsystem
+	 * these flags may get cleared in any order by
+	 *	the worker,
+	 *	the receiver,
+	 *	the bio_endio completion callbacks.
+	 */
+	if (s & RQ_NET_QUEUED) return;
 	if (s & RQ_NET_PENDING) return;
 	if (s & RQ_LOCAL_PENDING) return;
 

-- 
: Lars Ellenberg                                  Tel +43-1-8178292-0  :
: LINBIT Information Technologies GmbH            Fax +43-1-8178292-82 :
: Schoenbrunner Str. 244, A-1120 Vienna/Europe   http://www.linbit.com :

RE: [Drbd-dev] Panic in _drbd_send_page() again.

[Graham, Simon]

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

> so what you suggest is:
> 
> Index: drbd_req.c
> ===================================================================
> --- drbd_req.c	(revision 2864)
> +++ drbd_req.c	(working copy)
> @@ -255,6 +255,16 @@
>  	print_rq_state(req, "_req_may_be_done");
>  	MUST_HOLD(&mdev->req_lock)
> 
> +	/* we must not complete the master bio, while it is
> +	 *	still being processed by _drbd_send_zc_bio
> (drbd_send_dblock)
> +	 *	not yet acknowledged by the peer
> +	 *	not yet completed by the local io subsystem
> +	 * these flags may get cleared in any order by
> +	 *	the worker,
> +	 *	the receiver,
> +	 *	the bio_endio completion callbacks.
> +	 */
> +	if (s & RQ_NET_QUEUED) return;
>  	if (s & RQ_NET_PENDING) return;
>  	if (s & RQ_LOCAL_PENDING) return;
> 

I think this is correct -- in fact, I _think_ you could probably do away
with the RQ_NET_SENT flag (since this is, I think, the same as
!RQ_NET_QUEUED) which would be good (always good to remove extra
flags!).

In addition, there is code in req_mod right now that checks RQ_NET_SENT
before calling req_may_be_done -- this is no lomger needed since
req_may_be_done does the check. I've attached a completely untested
proposed patch that does this in addition to the change above... let me
know what you think...

Simon


[-- Attachment #2: net-queued.patch --]
[-- Type: application/octet-stream, Size: 4038 bytes --]

Index: drbd_req.c
===================================================================
--- drbd_req.c	(revision 13563)
+++ drbd_req.c	(working copy)
@@ -74,14 +74,13 @@
 			bio_data_dir(req->master_bio) == WRITE) ?
 		'W' : 'R';
 
-	INFO("%s %p %c L%c%c%cN%c%c%c%c%c %u (%llus +%u) %s\n",
+	INFO("%s %p %c L%c%c%cN%c%c%c%c %u (%llus +%u) %s\n",
 	     txt, req, rw,
 	     s & RQ_LOCAL_PENDING ? 'p' : '-',
 	     s & RQ_LOCAL_COMPLETED ? 'c' : '-',
 	     s & RQ_LOCAL_OK ? 'o' : '-',
 	     s & RQ_NET_PENDING ? 'p' : '-',
 	     s & RQ_NET_QUEUED ? 'q' : '-',
-	     s & RQ_NET_SENT ? 's' : '-',
 	     s & RQ_NET_DONE ? 'd' : '-',
 	     s & RQ_NET_OK ? 'o' : '-',
 	     req->epoch,
@@ -255,6 +254,7 @@
 	print_rq_state(req, "_req_may_be_done");
 	MUST_HOLD(&mdev->req_lock)
 
+	if (s & RQ_NET_QUEUED) return;
 	if (s & RQ_NET_PENDING) return;
 	if (s & RQ_LOCAL_PENDING) return;
 
@@ -623,7 +623,6 @@
 			 * so we know what to dirty on connection loss */
 		}
 		req->rq_state &= ~RQ_NET_QUEUED;
-		req->rq_state |= RQ_NET_SENT;
 		/* because _drbd_send_zc_bio could sleep, and may want to
 		 * dereference the bio even after the "write_acked_by_peer" and
 		 * "completed_ok" events came in, once we return from
@@ -662,8 +661,7 @@
 		D_ASSERT(req->rq_state & RQ_NET_PENDING);
 		dec_ap_pending(mdev);
 		req->rq_state &= ~RQ_NET_PENDING;
-		if (req->rq_state & RQ_NET_SENT)
-			_req_may_be_done(req,error);
+		_req_may_be_done(req,error);
 		/* else: done by handed_over_to_network */
 		break;
 
@@ -673,8 +671,7 @@
 		req->rq_state &= ~(RQ_NET_OK|RQ_NET_PENDING);
 		/* FIXME THINK! is it DONE now, or is it not? */
 		req->rq_state |= RQ_NET_DONE;
-		if (req->rq_state & RQ_NET_SENT)
-			_req_may_be_done(req,error);
+		_req_may_be_done(req,error);
 		/* else: done by handed_over_to_network */
 		break;
 
@@ -687,7 +684,7 @@
 			 * this is bad, because if the connection is lost now,
 			 * we won't be able to clean them up... */
 			const unsigned long s = req->rq_state;
-			INFO("%s %p %c L%c%c%cN%c%c%c%c%c %u (%llus +%u) %s\n",
+			INFO("%s %p %c L%c%c%cN%c%c%c%c %u (%llus +%u) %s\n",
 			     "FIXME", req,
 			     /* in fact, it can only be a WRITE, but anyways */
 			     bio_data_dir(req->master_bio) == WRITE ? 'W' : 'R',
@@ -696,7 +693,6 @@
 			     s & RQ_LOCAL_OK ? 'o' : '-',
 			     s & RQ_NET_PENDING ? 'p' : '-',
 			     s & RQ_NET_QUEUED ? 'q' : '-',
-			     s & RQ_NET_SENT ? 's' : '-',
 			     s & RQ_NET_DONE ? 'd' : '-',
 			     s & RQ_NET_OK ? 'o' : '-',
 			     req->epoch,
@@ -704,7 +700,7 @@
 			     req->size,
 			     conns_to_name(mdev->state.conn));
 		}
-		D_ASSERT(req->rq_state & RQ_NET_SENT);
+		D_ASSERT(!(req->rq_state & RQ_NET_QUEUED);
 		req->rq_state |= RQ_NET_DONE;
 		_req_may_be_done(req,error);
 		break;
@@ -714,11 +710,7 @@
 		dec_ap_pending(mdev);
 		req->rq_state &= ~RQ_NET_PENDING;
 		req->rq_state |= (RQ_NET_OK|RQ_NET_DONE);
-		/* can it happen that we receive the DataReply
-		 * before the send DataRequest function returns? */
-		if (req->rq_state & RQ_NET_SENT)
-			_req_may_be_done(req,error);
-		/* else: done by handed_over_to_network */
+		_req_may_be_done(req,error);
 		break;
 	};
 }
Index: drbd_req.h
===================================================================
--- drbd_req.h	(revision 13563)
+++ drbd_req.h	(working copy)
@@ -165,10 +165,7 @@
 	 * no longer occur. */
 	__RQ_NET_QUEUED,
 
-	/* well, actually only "handed over to the network stack" */
-	__RQ_NET_SENT,
-
-	/* when set, the request may be freed.
+	/* when set, the request may be freed (IF RQ_NET_QUEUED is clear)
 	 * in (C) this happens when WriteAck is received,
 	 * in (B,A) when the corresponding BarrierAck is received */
 	__RQ_NET_DONE,
@@ -190,7 +187,6 @@
 
 #define RQ_NET_PENDING     (1UL << __RQ_NET_PENDING)
 #define RQ_NET_QUEUED      (1UL << __RQ_NET_QUEUED)
-#define RQ_NET_SENT        (1UL << __RQ_NET_SENT)
 #define RQ_NET_DONE        (1UL << __RQ_NET_DONE)
 #define RQ_NET_OK          (1UL << __RQ_NET_OK)
 #define RQ_NET_SIS         (1UL << __RQ_NET_SIS)

Re: [Drbd-dev] Panic in _drbd_send_page() again.

[Lars Ellenberg]

On Fri, May 04, 2007 at 03:02:45PM -0400, Graham, Simon wrote:
> > so what you suggest is:
> > 
> > Index: drbd_req.c
> > ===================================================================
> > --- drbd_req.c	(revision 2864)
> > +++ drbd_req.c	(working copy)
> > @@ -255,6 +255,16 @@
> >  	print_rq_state(req, "_req_may_be_done");
> >  	MUST_HOLD(&mdev->req_lock)
> > 
> > +	/* we must not complete the master bio, while it is
> > +	 *	still being processed by _drbd_send_zc_bio
> > (drbd_send_dblock)
> > +	 *	not yet acknowledged by the peer
> > +	 *	not yet completed by the local io subsystem
> > +	 * these flags may get cleared in any order by
> > +	 *	the worker,
> > +	 *	the receiver,
> > +	 *	the bio_endio completion callbacks.
> > +	 */
> > +	if (s & RQ_NET_QUEUED) return;
> >  	if (s & RQ_NET_PENDING) return;
> >  	if (s & RQ_LOCAL_PENDING) return;
> > 
> 
> I think this is correct -- in fact, I _think_ you could probably do away
> with the RQ_NET_SENT flag (since this is, I think, the same as
> !RQ_NET_QUEUED) which would be good (always good to remove extra
> flags!).
> 
> In addition, there is code in req_mod right now that checks RQ_NET_SENT
> before calling req_may_be_done -- this is no lomger needed since
> req_may_be_done does the check. I've attached a completely untested
> proposed patch that does this in addition to the change above... let me
> know what you think...

For protocol C [and thats all that really matters, I know ;)]
this would be correct. I think.
But then, for protocol C only, we could get rid of a lot more there.

One might think the NET_SENT was always only a redundant flag,
and just the negation of the NET_QUEUED. but it is not.

I needed it to distinguish a local only request from one that was sent
over the network, but is not yet done, even though all other
NET_somthing flags might be clear for some reason.

there must be no time window where I cannot tell
whether a request had a network part or not,
from looking at the state flags.

At least for protocol A and B this would open up an other window,
where (state & NET_MASK) == 0 [*], even though it is still on the lists,
and the corresponding barrier ack is still pending.

[*] e.g. when receiving a neg_acked before handed_over_to_network.
    maybe that is even the only corner case, given that otherwise
    either NET_OK or NET_DONE should be set...

there is obviously redundant information in there,
using five bits for way below 32 states...
we can probably reorganize these flags, and their lifetime.
say, introduce a "RQ_NET_PART" and "RQ_LOCAL_PART".

but I don't think we can get rid of RQ_NET_SENT _that_ easy.

-- 
: Lars Ellenberg                                  Tel +43-1-8178292-0  :
: LINBIT Information Technologies GmbH            Fax +43-1-8178292-82 :
: Schoenbrunner Str. 244, A-1120 Vienna/Europe   http://www.linbit.com :

RE: [Drbd-dev] Panic in _drbd_send_page() again.

[Graham, Simon]

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

> but I don't think we can get rid of RQ_NET_SENT _that_ easy.

OK - you obviously know this stuff better than me!

I do still think that with this change we can remove the checks for
RQ_NET_SENT before calling req_may_be_done which I like because it moves
all the 'may be done' processing inside that function...

So - how about the attached...
/simgr


[-- Attachment #2: net-queued.patch --]
[-- Type: application/octet-stream, Size: 1857 bytes --]

Index: drbd_req.c
===================================================================
--- drbd_req.c	(revision 13563)
+++ drbd_req.c	(working copy)
@@ -255,6 +255,7 @@
 	print_rq_state(req, "_req_may_be_done");
 	MUST_HOLD(&mdev->req_lock)
 
+	if (s & RQ_NET_QUEUED) return;
 	if (s & RQ_NET_PENDING) return;
 	if (s & RQ_LOCAL_PENDING) return;
 
@@ -638,11 +639,7 @@
 		if (req->rq_state & RQ_NET_PENDING) dec_ap_pending(mdev);
 		req->rq_state &= ~(RQ_NET_OK|RQ_NET_PENDING);
 		req->rq_state |= RQ_NET_DONE;
-		/* if it is still queued, we may not complete it here.
-		 * it will be canceled soon.
-		 * FIXME we should change the code so this can not happen. */
-		if (!(req->rq_state & RQ_NET_QUEUED))
-			_req_may_be_done(req,error);
+		_req_may_be_done(req,error);
 		break;
 
 	case write_acked_by_peer_and_sis:
@@ -662,9 +659,7 @@
 		D_ASSERT(req->rq_state & RQ_NET_PENDING);
 		dec_ap_pending(mdev);
 		req->rq_state &= ~RQ_NET_PENDING;
-		if (req->rq_state & RQ_NET_SENT)
-			_req_may_be_done(req,error);
-		/* else: done by handed_over_to_network */
+		_req_may_be_done(req,error);
 		break;
 
 	case neg_acked:
@@ -673,9 +668,7 @@
 		req->rq_state &= ~(RQ_NET_OK|RQ_NET_PENDING);
 		/* FIXME THINK! is it DONE now, or is it not? */
 		req->rq_state |= RQ_NET_DONE;
-		if (req->rq_state & RQ_NET_SENT)
-			_req_may_be_done(req,error);
-		/* else: done by handed_over_to_network */
+		_req_may_be_done(req,error);
 		break;
 
 	case barrier_acked:
@@ -714,11 +707,7 @@
 		dec_ap_pending(mdev);
 		req->rq_state &= ~RQ_NET_PENDING;
 		req->rq_state |= (RQ_NET_OK|RQ_NET_DONE);
-		/* can it happen that we receive the DataReply
-		 * before the send DataRequest function returns? */
-		if (req->rq_state & RQ_NET_SENT)
-			_req_may_be_done(req,error);
-		/* else: done by handed_over_to_network */
+		_req_may_be_done(req,error);
 		break;
 	};
 }

Re: [Drbd-dev] Panic in _drbd_send_page() again.

[Lars Ellenberg]

On Fri, May 04, 2007 at 08:31:07PM -0400, Graham, Simon wrote:
> > but I don't think we can get rid of RQ_NET_SENT _that_ easy.
> 
> OK - you obviously know this stuff better than me!
> 
> I do still think that with this change we can remove the checks for
> RQ_NET_SENT before calling req_may_be_done which I like because it moves
> all the 'may be done' processing inside that function...

that's correct.
 :)


-- 
: Lars Ellenberg                                  Tel +43-1-8178292-0  :
: LINBIT Information Technologies GmbH            Fax +43-1-8178292-82 :
: Schoenbrunner Str. 244, A-1120 Vienna/Europe   http://www.linbit.com :