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",
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to