Skip site navigation (1)Skip section navigation (2) 
Date:      Sat, 6 Jun 2020 00:02:50 +0000 (UTC)
From:      Chuck Silvers <chs@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r361852 - head/sys/kern
Message-ID:  <202006060002.05602oJW002384@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: chs
Date: Sat Jun  6 00:02:50 2020
New Revision: 361852
URL: https://svnweb.freebsd.org/changeset/base/361852

Log:
  Fix hang due to missing unbusy in sendfile when an async data I/O fails.
  
  r359473 removed the page unbusy logic from sendfile_iodone() because when
  vm_pager_get_pages_async() would return an error after failing to start
  the async I/O (eg. because VOP_BMAP failed), sendfile_swapin() would also
  unbusy the pages, and it was wrong to unbusy twice.  However this breaks
  the case where vm_pager_get_pages_async() succeeds in starting an async I/O
  and the async I/O is what fails.  In this case, sendfile_iodone() must
  unbusy the pages, and because sendfile_iodone() doesn't know which case
  it is in, sendfile_iodone() must always unbusy pages and relookup pages
  which have been substituted with bogus_page, which in turn means that
  sendfile_swapin() must never do unbusy or relookup for pages which have
  been given to vm_pager_get_pages_async(), even if there is an error.
  
  Reviewed by:	kib, markj
  Sponsored by:	Netflix
  Differential Revision:	https://reviews.freebsd.org/D25136

Modified:
  head/sys/kern/kern_sendfile.c

Modified: head/sys/kern/kern_sendfile.c
==============================================================================
--- head/sys/kern/kern_sendfile.c	Fri Jun  5 22:07:10 2020	(r361851)
+++ head/sys/kern/kern_sendfile.c	Sat Jun  6 00:02:50 2020	(r361852)
@@ -292,36 +292,30 @@ sendfile_iodone(void *arg, vm_page_t *pa, int count, i
 	struct socket *so;
 	int i;
 
-	if (error != 0) {
+	if (error != 0)
 		sfio->error = error;
-		/*
-		 * Restore of the pg[] elements is done by
-		 * sendfile_swapin().
-		 */
-	} else {
-		/*
-		 * Restore the valid page pointers.  They are already
-		 * unbusied, but still wired.  For error != 0 case,
-		 * sendfile_swapin() handles unbusy.
-		 *
-		 * XXXKIB since pages are only wired, and we do not
-		 * own the object lock, other users might have
-		 * invalidated them in meantime.  Similarly, after we
-		 * unbusied the swapped-in pages, they can become
-		 * invalid under us.
-		 */
-		MPASS(count == 0 || pa[0] != bogus_page);
-		for (i = 0; i < count; i++) {
-			if (pa[i] == bogus_page) {
-				sfio->pa[(pa[0]->pindex - sfio->pindex0) + i] =
-				    pa[i] = vm_page_relookup(sfio->obj,
-				    pa[0]->pindex + i);
-				KASSERT(pa[i] != NULL,
-				    ("%s: page %p[%d] disappeared",
-				    __func__, pa, i));
-			} else {
-				vm_page_xunbusy_unchecked(pa[i]);
-			}
+
+	/*
+	 * Restore the valid page pointers.  They are already
+	 * unbusied, but still wired.
+	 *
+	 * XXXKIB since pages are only wired, and we do not
+	 * own the object lock, other users might have
+	 * invalidated them in meantime.  Similarly, after we
+	 * unbusied the swapped-in pages, they can become
+	 * invalid under us.
+	 */
+	MPASS(count == 0 || pa[0] != bogus_page);
+	for (i = 0; i < count; i++) {
+		if (pa[i] == bogus_page) {
+			sfio->pa[(pa[0]->pindex - sfio->pindex0) + i] =
+			    pa[i] = vm_page_relookup(sfio->obj,
+			    pa[0]->pindex + i);
+			KASSERT(pa[i] != NULL,
+			    ("%s: page %p[%d] disappeared",
+			    __func__, pa, i));
+		} else {
+			vm_page_xunbusy_unchecked(pa[i]);
 		}
 	}
 
@@ -534,22 +528,12 @@ sendfile_swapin(vm_object_t obj, struct sf_io *sfio, i
 			sendfile_iowait(sfio, "sferrio");
 
 			/*
-			 * Perform full pages recovery before returning EIO.
+			 * Do remaining pages recovery before returning EIO.
 			 * Pages from 0 to npages are wired.
-			 * Pages from (i + 1) to (i + count - 1) may be
-			 * substituted to bogus page, and not busied.
-			 * Pages from (i + count) to (i + count1 - 1) are
-			 * not busied.
-			 * Rest of the pages from i to npages are busied.
+			 * Pages from (i + count1) to npages are busied.
 			 */
 			for (j = 0; j < npages; j++) {
-				if (j >= i + count && j < i + count1)
-					;
-				else if (j > i && j < i + count - 1 &&
-				    pa[j] == bogus_page)
-					pa[j] = vm_page_relookup(obj,
-					    OFF_TO_IDX(vmoff(j, off)));
-				else if (j >= i)
+				if (j >= i + count1)
 					vm_page_xunbusy(pa[j]);
 				KASSERT(pa[j] != NULL && pa[j] != bogus_page,
 				    ("%s: page %p[%d] I/O recovery failure",

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202006060002.05602oJW002384>