Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 22 Aug 2012 05:15:21 +0000 (UTC)
From:      Konstantin Belousov <kib@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-9@freebsd.org
Subject:   svn commit: r239554 - in stable/9/sys: fs/nfsclient fs/nwfs fs/smbfs nfsclient vm
Message-ID:  <201208220515.q7M5FL0o071023@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: kib
Date: Wed Aug 22 05:15:21 2012
New Revision: 239554
URL: http://svn.freebsd.org/changeset/base/239554

Log:
  MFC r239040:
  Reduce code duplication and exposure of direct access to struct
  vm_page oflags by providing helper function
  vm_page_readahead_finish(), which handles completed reads for pages
  with indexes other then the requested one, for VOP_GETPAGES().
  
  MFC r239246:
  Do not leave invalid pages in the object after the short read for a
  network file systems (not only NFS proper). Short reads cause pages
  other then the requested one, which were not filled by read response,
  to stay invalid.
  
  Change the vm_page_readahead_finish() interface to not take the error
  code, but instead to make a decision to free or to (de)activate the
  page only by its validity. As result, not requested invalid pages are
  freed even if the read RPC indicated success.

Modified:
  stable/9/sys/fs/nfsclient/nfs_clbio.c
  stable/9/sys/fs/nwfs/nwfs_io.c
  stable/9/sys/fs/smbfs/smbfs_io.c
  stable/9/sys/nfsclient/nfs_bio.c
  stable/9/sys/vm/vm_page.c
  stable/9/sys/vm/vm_page.h
  stable/9/sys/vm/vnode_pager.c
Directory Properties:
  stable/9/sys/   (props changed)
  stable/9/sys/fs/   (props changed)

Modified: stable/9/sys/fs/nfsclient/nfs_clbio.c
==============================================================================
--- stable/9/sys/fs/nfsclient/nfs_clbio.c	Wed Aug 22 05:14:59 2012	(r239553)
+++ stable/9/sys/fs/nfsclient/nfs_clbio.c	Wed Aug 22 05:15:21 2012	(r239554)
@@ -217,42 +217,18 @@ ncl_getpages(struct vop_getpages_args *a
 			    ("nfs_getpages: page %p is dirty", m));
 		} else {
 			/*
-			 * Read operation was short.  If no error occured
-			 * we may have hit a zero-fill section.   We simply
-			 * leave valid set to 0.
+			 * Read operation was short.  If no error
+			 * occured we may have hit a zero-fill
+			 * section.  We leave valid set to 0, and page
+			 * is freed by vm_page_readahead_finish() if
+			 * its index is not equal to requested, or
+			 * page is zeroed and set valid by
+			 * vm_pager_get_pages() for requested page.
 			 */
 			;
 		}
-		if (i != ap->a_reqpage) {
-			/*
-			 * Whether or not to leave the page activated is up in
-			 * the air, but we should put the page on a page queue
-			 * somewhere (it already is in the object).  Result:
-			 * It appears that emperical results show that
-			 * deactivating pages is best.
-			 */
-
-			/*
-			 * Just in case someone was asking for this page we
-			 * now tell them that it is ok to use.
-			 */
-			if (!error) {
-				if (m->oflags & VPO_WANTED) {
-					vm_page_lock(m);
-					vm_page_activate(m);
-					vm_page_unlock(m);
-				} else {
-					vm_page_lock(m);
-					vm_page_deactivate(m);
-					vm_page_unlock(m);
-				}
-				vm_page_wakeup(m);
-			} else {
-				vm_page_lock(m);
-				vm_page_free(m);
-				vm_page_unlock(m);
-			}
-		}
+		if (i != ap->a_reqpage)
+			vm_page_readahead_finish(m);
 	}
 	VM_OBJECT_UNLOCK(object);
 	return (0);

Modified: stable/9/sys/fs/nwfs/nwfs_io.c
==============================================================================
--- stable/9/sys/fs/nwfs/nwfs_io.c	Wed Aug 22 05:14:59 2012	(r239553)
+++ stable/9/sys/fs/nwfs/nwfs_io.c	Wed Aug 22 05:15:21 2012	(r239554)
@@ -457,36 +457,8 @@ nwfs_getpages(ap)
 			    ("nwfs_getpages: page %p is dirty", m));
 		}
 
-		if (i != ap->a_reqpage) {
-			/*
-			 * Whether or not to leave the page activated is up in
-			 * the air, but we should put the page on a page queue
-			 * somewhere (it already is in the object).  Result:
-			 * It appears that emperical results show that
-			 * deactivating pages is best.
-			 */
-
-			/*
-			 * Just in case someone was asking for this page we
-			 * now tell them that it is ok to use.
-			 */
-			if (!error) {
-				if (m->oflags & VPO_WANTED) {
-					vm_page_lock(m);
-					vm_page_activate(m);
-					vm_page_unlock(m);
-				} else {
-					vm_page_lock(m);
-					vm_page_deactivate(m);
-					vm_page_unlock(m);
-				}
-				vm_page_wakeup(m);
-			} else {
-				vm_page_lock(m);
-				vm_page_free(m);
-				vm_page_unlock(m);
-			}
-		}
+		if (i != ap->a_reqpage)
+			vm_page_readahead_finish(m);
 	}
 	VM_OBJECT_UNLOCK(object);
 	return 0;

Modified: stable/9/sys/fs/smbfs/smbfs_io.c
==============================================================================
--- stable/9/sys/fs/smbfs/smbfs_io.c	Wed Aug 22 05:14:59 2012	(r239553)
+++ stable/9/sys/fs/smbfs/smbfs_io.c	Wed Aug 22 05:15:21 2012	(r239554)
@@ -521,36 +521,8 @@ smbfs_getpages(ap)
 			;
 		}
 
-		if (i != reqpage) {
-			/*
-			 * Whether or not to leave the page activated is up in
-			 * the air, but we should put the page on a page queue
-			 * somewhere (it already is in the object).  Result:
-			 * It appears that emperical results show that
-			 * deactivating pages is best.
-			 */
-
-			/*
-			 * Just in case someone was asking for this page we
-			 * now tell them that it is ok to use.
-			 */
-			if (!error) {
-				if (m->oflags & VPO_WANTED) {
-					vm_page_lock(m);
-					vm_page_activate(m);
-					vm_page_unlock(m);
-				} else {
-					vm_page_lock(m);
-					vm_page_deactivate(m);
-					vm_page_unlock(m);
-				}
-				vm_page_wakeup(m);
-			} else {
-				vm_page_lock(m);
-				vm_page_free(m);
-				vm_page_unlock(m);
-			}
-		}
+		if (i != reqpage)
+			vm_page_readahead_finish(m);
 	}
 	VM_OBJECT_UNLOCK(object);
 	return 0;

Modified: stable/9/sys/nfsclient/nfs_bio.c
==============================================================================
--- stable/9/sys/nfsclient/nfs_bio.c	Wed Aug 22 05:14:59 2012	(r239553)
+++ stable/9/sys/nfsclient/nfs_bio.c	Wed Aug 22 05:15:21 2012	(r239554)
@@ -211,42 +211,18 @@ nfs_getpages(struct vop_getpages_args *a
 			    ("nfs_getpages: page %p is dirty", m));
 		} else {
 			/*
-			 * Read operation was short.  If no error occured
-			 * we may have hit a zero-fill section.   We simply
-			 * leave valid set to 0.
+			 * Read operation was short.  If no error
+			 * occured we may have hit a zero-fill
+			 * section.  We leave valid set to 0, and page
+			 * is freed by vm_page_readahead_finish() if
+			 * its index is not equal to requested, or
+			 * page is zeroed and set valid by
+			 * vm_pager_get_pages() for requested page.
 			 */
 			;
 		}
-		if (i != ap->a_reqpage) {
-			/*
-			 * Whether or not to leave the page activated is up in
-			 * the air, but we should put the page on a page queue
-			 * somewhere (it already is in the object).  Result:
-			 * It appears that emperical results show that
-			 * deactivating pages is best.
-			 */
-
-			/*
-			 * Just in case someone was asking for this page we
-			 * now tell them that it is ok to use.
-			 */
-			if (!error) {
-				if (m->oflags & VPO_WANTED) {
-					vm_page_lock(m);
-					vm_page_activate(m);
-					vm_page_unlock(m);
-				} else {
-					vm_page_lock(m);
-					vm_page_deactivate(m);
-					vm_page_unlock(m);
-				}
-				vm_page_wakeup(m);
-			} else {
-				vm_page_lock(m);
-				vm_page_free(m);
-				vm_page_unlock(m);
-			}
-		}
+		if (i != ap->a_reqpage)
+			vm_page_readahead_finish(m);
 	}
 	VM_OBJECT_UNLOCK(object);
 	return (0);

Modified: stable/9/sys/vm/vm_page.c
==============================================================================
--- stable/9/sys/vm/vm_page.c	Wed Aug 22 05:14:59 2012	(r239553)
+++ stable/9/sys/vm/vm_page.c	Wed Aug 22 05:15:21 2012	(r239554)
@@ -754,6 +754,45 @@ vm_page_free_zero(vm_page_t m)
 }
 
 /*
+ * Unbusy and handle the page queueing for a page from the VOP_GETPAGES()
+ * array which is not the request page.
+ */
+void
+vm_page_readahead_finish(vm_page_t m)
+{
+
+	if (m->valid != 0) {
+		/*
+		 * Since the page is not the requested page, whether
+		 * it should be activated or deactivated is not
+		 * obvious.  Empirical results have shown that
+		 * deactivating the page is usually the best choice,
+		 * unless the page is wanted by another thread.
+		 */
+		if (m->oflags & VPO_WANTED) {
+			vm_page_lock(m);
+			vm_page_activate(m);
+			vm_page_unlock(m);
+		} else {
+			vm_page_lock(m);
+			vm_page_deactivate(m);
+			vm_page_unlock(m);
+		}
+		vm_page_wakeup(m);
+	} else {
+		/*
+		 * Free the completely invalid page.  Such page state
+		 * occurs due to the short read operation which did
+		 * not covered our page at all, or in case when a read
+		 * error happens.
+		 */
+		vm_page_lock(m);
+		vm_page_free(m);
+		vm_page_unlock(m);
+	}
+}
+
+/*
  *	vm_page_sleep:
  *
  *	Sleep and release the page and page queues locks.

Modified: stable/9/sys/vm/vm_page.h
==============================================================================
--- stable/9/sys/vm/vm_page.h	Wed Aug 22 05:14:59 2012	(r239553)
+++ stable/9/sys/vm/vm_page.h	Wed Aug 22 05:15:21 2012	(r239554)
@@ -386,6 +386,7 @@ vm_page_t vm_page_next(vm_page_t m);
 int vm_page_pa_tryrelock(pmap_t, vm_paddr_t, vm_paddr_t *);
 vm_page_t vm_page_prev(vm_page_t m);
 void vm_page_putfake(vm_page_t m);
+void vm_page_readahead_finish(vm_page_t m);
 void vm_page_reference(vm_page_t m);
 void vm_page_remove (vm_page_t);
 void vm_page_rename (vm_page_t, vm_object_t, vm_pindex_t);

Modified: stable/9/sys/vm/vnode_pager.c
==============================================================================
--- stable/9/sys/vm/vnode_pager.c	Wed Aug 22 05:14:59 2012	(r239553)
+++ stable/9/sys/vm/vnode_pager.c	Wed Aug 22 05:15:21 2012	(r239554)
@@ -983,37 +983,8 @@ vnode_pager_generic_getpages(vp, m, byte
 			    mt));
 		}
 		
-		if (i != reqpage) {
-
-			/*
-			 * whether or not to leave the page activated is up in
-			 * the air, but we should put the page on a page queue
-			 * somewhere. (it already is in the object). Result:
-			 * It appears that empirical results show that
-			 * deactivating pages is best.
-			 */
-
-			/*
-			 * just in case someone was asking for this page we
-			 * now tell them that it is ok to use
-			 */
-			if (!error) {
-				if (mt->oflags & VPO_WANTED) {
-					vm_page_lock(mt);
-					vm_page_activate(mt);
-					vm_page_unlock(mt);
-				} else {
-					vm_page_lock(mt);
-					vm_page_deactivate(mt);
-					vm_page_unlock(mt);
-				}
-				vm_page_wakeup(mt);
-			} else {
-				vm_page_lock(mt);
-				vm_page_free(mt);
-				vm_page_unlock(mt);
-			}
-		}
+		if (i != reqpage)
+			vm_page_readahead_finish(mt);
 	}
 	VM_OBJECT_UNLOCK(object);
 	if (error) {



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