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>