Message-ID: <3ACCB6A7.EA84241@thebarn.com>
Date: Thu, 05 Apr 2001 14:17:11 -0400
From: Russell Cattelan <cattelan@thebarn.com>
MIME-Version: 1.0
To: Utz Lehmann <leh@tecosim.de>, @thebarn.com, linux-xfs@oss.sgi.com
Subject: Re: shutdown umount hangs
References: <leh@tecosim.de> <200104051412.f35ECMU25857@jen.americas.sgi.com> <20010405172344.A1151@tecosim.de> <3ACC9D13.33DA7299@thebarn.com>
Content-Type: multipart/mixed;
 boundary="------------90EB41DEB21E6E612C1AA8F1"
Sender: owner-linux-xfs@oss.sgi.com
Precedence: bulk

This is a multi-part message in MIME format.
--------------90EB41DEB21E6E612C1AA8F1
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Ok one more shot at it.

I'm not convinced this is entirely correct, but is closer to the irix semantics

notable exception; the pagebuf lock is not grabbed before checking the pin count.

I'm trying to think of a scenario where is this matters, it is much cheaper to

just check the count rather than grabbing the lock checking the count and then dropping the

lock.

If this still hangs please send the bt and the output of kdb's pb <addr>.

I've tested doing busy umounts:

mount /xfs2; cd /xfs2/dbench/; ./dbench 10 ; cd / ; umount /xfs2

seems to work?!

I'm going toss a lvm volume on top of this and see if that makes a difference.

--
Russell Cattelan
--
Digital Elves inc. -- Currently on loan to SGI
Linux XFS core developer.


--------------90EB41DEB21E6E612C1AA8F1
Content-Type: text/plain; charset=us-ascii;
 name="pb_patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="pb_patch"

--- /usr/tmp/TmpDir.25365-0/linux/fs/pagebuf/page_buf.c_1.72	Thu Apr  5 13:08:24 2001
+++ linux/fs/pagebuf/page_buf.c	Thu Apr  5 12:45:05 2001
@@ -2136,6 +2136,7 @@
 	struct list_head *head, *curr;
 	unsigned long save;
 	pagebuf_marker_t *pb_marker_ptr;
+	int lr;
 
 
 	pb_marker_ptr = kmalloc(sizeof(pagebuf_marker_t), GFP_KERNEL);
@@ -2164,7 +2165,8 @@
 
 		if (pb->pb_target == target) {
 			if (pb->pb_flags & PBF_DELWRI) {
-
+			  
+			  /* This would be handled correctly below */
 			  if ((flags & PBDF_PINCOUNT) && pagebuf_ispin(pb)){
 				/* just want a count of the number of pinned buffer */
 				(*pincount)++;
@@ -2172,11 +2174,6 @@
 				continue;
 			  }
 
-				pb->pb_flags &= ~PBF_DELWRI;
-				pb->pb_flags |= PBF_WRITE;
-				if (flags & PBDF_WAIT)
-					pb->pb_flags &= ~PBF_ASYNC;
-
 			  	/* insert a place holder */
 				list_add(&pb_marker_ptr->pb_list, curr);
 
@@ -2184,40 +2181,43 @@
 						&pb_daemon->pb_delwrite_lock,
 						save);
 
-				if (pb->pb_flags & _PBF_LOCKABLE){
-				  if (flags & PBDF_PINCOUNT){
-					/* if we are doing this pin count thing and we can't get the lock just skip it for now */
-					}if (!pagebuf_cond_lock(pb)){
+				 /* if (pb->pb_flags & _PBF_LOCKABLE){ */
+				/* meta data pagebufs better be lockable pagebuf's */
+				  assert(pb->pb_flags & _PBF_LOCKABLE); 
+	
+				  if (!pagebuf_ispin(pb)){
+
+					if ((flags & (PBDF_WAIT | PBDF_PINCOUNT)) == PBDF_WAIT)
+					  lr = pagebuf_lock(pb);
+					else if (flags &  PBDF_PINCOUNT)
+					  lr = pagebuf_cond_lock(pb);
+
+					if (lr == 0){ /* got the lock */
+					  pb->pb_flags &= ~PBF_DELWRI;
+					  pb->pb_flags |= PBF_WRITE;
+					  if (flags & PBDF_WAIT)
+						pb->pb_flags &= ~PBF_ASYNC;
 					  __pagebuf_iorequest(pb);
-					} else {
+					} else  /* didn't get the lock just bump pin count return val*/
 					  (*pincount)++;
-					  /* make it async again... don't want to wait on in below */
-					  pb->pb_flags |= PBF_ASYNC;
-					}
-				  }else {
-					pagebuf_lock(pb);
-					__pagebuf_iorequest(pb);
-				  }
 
-				spin_lock_irqsave(
-						&pb_daemon->pb_delwrite_lock,
-						save);
+				  } else { /* still pinned */
+					if (flags & PBDF_PINCOUNT) 
+					  (*pincount)++;
+				  }
+				spin_lock_irqsave(&pb_daemon->pb_delwrite_lock, save);
 				/*
-				 * ok got the lock back; pick up the place
+				 * ok got the list lock back; pick up the place
 				 * holder and continue on
 				 */
 				curr = pb_marker_ptr->pb_list.next;
 				list_del(&pb_marker_ptr->pb_list);
-			} else {
-				/* not doing anything with current...
-				 * move on to the next one */
-				curr = curr->next;
-			}
-		} else {
-			/* not doing anything with current...
-			 * move on to the next one */
-			curr = curr->next;
-		}
+				continue;
+			} 
+		} 
+		/* not doing anything with current...
+		 * move on to the next one */
+		curr = curr->next;
 	}
 
 	spin_unlock_irqrestore(&pb_daemon->pb_delwrite_lock, save);
@@ -2270,14 +2270,13 @@
 		PB_TRACE(pb, PB_TRACE_REC(walkq3), pagebuf_ispin(pb));
 
 		if (pb->pb_target == target) {
-			int	sync = (pb->pb_flags & PBF_ASYNC) == 0;
 			list_add(&pb_marker_ptr->pb_list, curr);
 
 			spin_unlock_irqrestore(
 					&pb_daemon->pb_delwrite_lock,
 					flags);
 
-			if (sync) {
+			if (!(pb->pb_flags & PBF_ASYNC)) {
 				pagebuf_iowait(pb);
 				pb->pb_flags |= PBF_ASYNC;
 				pagebuf_delwri_dequeue(pb);

--------------90EB41DEB21E6E612C1AA8F1--