Skip to contentSkip to conversation

Convert NFS client to use ->readpages()

PATCH In review Trond Myklebust 2 participants 3 messages Nov 6, 2002

Subsystems (4)
Memory management NFS client NFS server Page cache
lore
Share & tools
lore

Trond Myklebust

author
#0 lore raw

PATCH 1/1 Convert NFS client to use ->readpages()

- Add the library function read_cache_pages(), which is used in a similar fashion to the single page 'read_cache_page()'. It hides the details of the LRU cache etc. from a filesystem that wants to to populate an address space with a list of pages.

- Fix NFS so that readahead uses the ->readpages() interface. Means that we can immediately schedule an RPC call in order to complete the I/O, rather than relying on somebody later triggering it by calling lock_page() (and hence sync_page()). The sync_page() method is race-prone, since the waiting page may try to call it before we've finished initializing the 'struct nfs_page'.

- Clear out nfs_sync_page(), the nfs_inode->read list, and

    friends. When the I/O completion gets scheduled in ->readpage(),
    ->readpages(), they have no reason to exist.

Cheers,
Trond

fs/nfs/file.c +1 -32
diff -u --recursive --new-file linux-2.5.46-01-readpages1/fs/nfs/file.c linux-2.5.46-02-readpages2/fs/nfs/file.c
@@ -77,11 +77,6 @@
 
 	dfprintk(VFS, "nfs: flush(%s/%ld)\n", inode->i_sb->s_id, inode->i_ino);
 
-	/* Make sure all async reads have been sent off. We don't bother
-	 * waiting on them though... */
-	if (file->f_mode & FMODE_READ)
-		nfs_pagein_inode(inode, 0, 0);
-
 	status = nfs_wb_file(inode, file);
 	if (!status) {
 		status = file->f_error;

@@ -170,35 +165,9 @@
 	return status;
 }
 
-/*
- * The following is used by wait_on_page_locked(), generic_file_readahead()
- * to initiate the completion of any page readahead operations.
- */
-static int nfs_sync_page(struct page *page)
-{
-	struct address_space *mapping;
-	struct inode	*inode;
-	unsigned long	index = page->index;
-	unsigned int	rpages;
-	int		result;
-
-	mapping = page->mapping;
-	if (!mapping)
-		return 0;
-	inode = mapping->host;
-	if (!inode)
-		return 0;
-
-	rpages = NFS_SERVER(inode)->rpages;
-	result = nfs_pagein_inode(inode, index, rpages);
-	if (result < 0)
-		return result;
-	return 0;
-}
-
 struct address_space_operations nfs_file_aops = {
 	.readpage = nfs_readpage,
-	.sync_page = nfs_sync_page,
+	.readpages = nfs_readpages,
 	.writepage = nfs_writepage,
 	.prepare_write = nfs_prepare_write,
 	.commit_write = nfs_commit_write,
fs/nfs/flushd.c +0 -5
diff -u --recursive --new-file linux-2.5.46-01-readpages1/fs/nfs/flushd.c linux-2.5.46-02-readpages2/fs/nfs/flushd.c
@@ -166,11 +166,6 @@
 			nfs_flush_list(&head, server->wpages, FLUSH_AGING);
 			continue;
 		}
-		if (nfs_scan_lru_read_timeout(server, &head)) {
-			spin_unlock(&nfs_wreq_lock);
-			nfs_pagein_list(&head, server->rpages);
-			continue;
-		}
 #if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)
 		if (nfs_scan_lru_commit_timeout(server, &head)) {
 			spin_unlock(&nfs_wreq_lock);
fs/nfs/inode.c +1 -3
diff -u --recursive --new-file linux-2.5.46-01-readpages1/fs/nfs/inode.c linux-2.5.46-02-readpages2/fs/nfs/inode.c
@@ -117,7 +117,7 @@
 	/*
 	 * The following can never actually happen...
 	 */
-	if (nfs_have_writebacks(inode) || nfs_have_read(inode)) {
+	if (nfs_have_writebacks(inode)) {
 		printk(KERN_ERR "nfs_delete_inode: inode %ld has pending RPC requests\n", inode->i_ino);
 	}
 

@@ -1555,11 +1555,9 @@
 	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
 	    SLAB_CTOR_CONSTRUCTOR) {
 		inode_init_once(&nfsi->vfs_inode);
-		INIT_LIST_HEAD(&nfsi->read);
 		INIT_LIST_HEAD(&nfsi->dirty);
 		INIT_LIST_HEAD(&nfsi->commit);
 		INIT_LIST_HEAD(&nfsi->writeback);
-		nfsi->nread = 0;
 		nfsi->ndirty = 0;
 		nfsi->ncommit = 0;
 		nfsi->npages = 0;
fs/nfs/pagelist.c +0 -7
diff -u --recursive --new-file linux-2.5.46-01-readpages1/fs/nfs/pagelist.c linux-2.5.46-02-readpages2/fs/nfs/pagelist.c
@@ -516,13 +516,6 @@
 			continue;
 		}
 #endif
-		/* OK, so we try to free up some pending readaheads */
-		nfs_scan_lru_read(server, &head);
-		if (!list_empty(&head)) {
-			spin_unlock(&nfs_wreq_lock);
-			nfs_pagein_list(&head, server->rpages);
-			continue;
-		}
 		/* Last resort: we try to flush out single requests */
 		nfs_scan_lru_dirty(server, &head);
 		if (!list_empty(&head)) {
fs/nfs/read.c +69 -108

208 lines not on this page · raw message

include/linux/nfs_fs.h +3 -12
diff -u --recursive --new-file linux-2.5.46-01-readpages1/include/linux/nfs_fs.h linux-2.5.46-02-readpages2/include/linux/nfs_fs.h
@@ -163,13 +163,11 @@
 	/*
 	 * This is the list of dirty unwritten pages.
 	 */
-	struct list_head	read;
 	struct list_head	dirty;
 	struct list_head	commit;
 	struct list_head	writeback;
 
-	unsigned int		nread,
-				ndirty,
+	unsigned int		ndirty,
 				ncommit,
 				npages;
 

@@ -348,12 +346,6 @@
 #endif
 
 static inline int
-nfs_have_read(struct inode *inode)
-{
-	return !list_empty(&NFS_I(inode)->read);
-}
-
-static inline int
 nfs_have_writebacks(struct inode *inode)
 {
 	return !list_empty(&NFS_I(inode)->writeback);

@@ -396,10 +388,9 @@
  * linux/fs/nfs/read.c
  */
 extern int  nfs_readpage(struct file *, struct page *);
-extern int  nfs_pagein_inode(struct inode *, unsigned long, unsigned int);
+extern int  nfs_readpages(struct file *, struct address_space *,
+		struct list_head *, unsigned);
 extern int  nfs_pagein_list(struct list_head *, int);
-extern int  nfs_scan_lru_read(struct nfs_server *, struct list_head *);
-extern int  nfs_scan_lru_read_timeout(struct nfs_server *, struct list_head *);
 extern void nfs_readpage_result(struct rpc_task *, unsigned int count, int eof);
 extern void nfs_readdata_release(struct rpc_task *);
include/linux/pagemap.h +2 -0
diff -u --recursive --new-file linux-2.5.46-01-readpages1/include/linux/pagemap.h linux-2.5.46-02-readpages2/include/linux/pagemap.h
@@ -63,6 +63,8 @@
 extern struct page * read_cache_page(struct address_space *mapping,
 				unsigned long index, filler_t *filler,
 				void *data);
+extern int read_cache_pages(struct address_space *mapping,
+		struct list_head *pages, filler_t *filler, void *data);
 
 extern int add_to_page_cache(struct page *page,
 		struct address_space *mapping, unsigned long index);
kernel/ksyms.c +1 -0
diff -u --recursive --new-file linux-2.5.46-01-readpages1/kernel/ksyms.c linux-2.5.46-02-readpages2/kernel/ksyms.c
@@ -284,6 +284,7 @@
 EXPORT_SYMBOL(find_or_create_page);
 EXPORT_SYMBOL(grab_cache_page_nowait);
 EXPORT_SYMBOL(read_cache_page);
+EXPORT_SYMBOL(read_cache_pages);
 EXPORT_SYMBOL(mark_page_accessed);
 EXPORT_SYMBOL(vfs_readlink);
 EXPORT_SYMBOL(vfs_follow_link);
mm/readahead.c +40 -0
diff -u --recursive --new-file linux-2.5.46-01-readpages1/mm/readahead.c linux-2.5.46-02-readpages2/mm/readahead.c
@@ -42,6 +42,46 @@
 	return (VM_MIN_READAHEAD * 1024) / PAGE_CACHE_SIZE;
 }
 
+/**
+ * read_cache_pages - populate an address space with some pages, and
+ * 			start reads against them.
+ * @mapping: the address_space
+ * @pages: The address of a list_head which contains the target pages.  These
+ *   pages have their ->index populated and are otherwise uninitialised.
+ * @filler: callback routine for filling a single page.
+ * @data: private data for the callback routine.
+ *
+ * Hides the details of the LRU cache etc from the filesystems.
+ */
+int
+read_cache_pages(struct address_space *mapping,
+		 struct list_head *pages,
+		 int (*filler)(void *, struct page *),
+		 void *data)
+{
+	struct page *page;
+	struct pagevec lru_pvec;
+	int ret = 0;
+
+	pagevec_init(&lru_pvec, 0);
+
+	while (!list_empty(pages)) {
+		page = list_entry(pages->prev, struct page, list);
+		list_del(&page->list);
+		if (add_to_page_cache(page, mapping, page->index)) {
+			page_cache_release(page);
+			continue;
+		}
+		ret = filler(data, page);
+		if (!pagevec_add(&lru_pvec, page))
+			__pagevec_lru_add(&lru_pvec);
+		if (ret)
+			break;
+	}
+	pagevec_lru_add(&lru_pvec);
+	return ret;
+}
+
 static int
 read_pages(struct address_space *mapping, struct file *filp,
 		struct list_head *pages, unsigned nr_pages)
#1 lore raw
Quoted 13 lines · Trond Myklebust
fs/nfs/read.c +12 -0
+int
+nfs_readpages(struct file *filp, struct address_space *mapping,
+               struct list_head *pages, unsigned nr_pages)
+{
+       LIST_HEAD(head);
+       struct nfs_readdesc desc = {
+               .filp           = filp,
+               .head           = &head,
+       };
+       struct nfs_server *server = NFS_SERVER(mapping->host);
+       int is_sync = server->rsize < PAGE_CACHE_SIZE;
+       int ret;

fs/nfs/read.c line 394

Quoted 10 lines
fs/nfs/read.c +10 -0
+
+       ret = read_cache_pages(mapping, pages,
+                              is_sync ? readpage_sync_filler :
+                                        readpage_async_filler,
+                              &desc);
+       if (!list_empty(pages)) {
+               struct page *page = list_entry(pages->prev, struct page, list);
+               list_del(&page->list);
+               page_cache_release(page);
+       }

What are the above few lines doing? Looks odd.

Or should it be

	while (!list_empty(...))

?

Quoted 8 lines
fs/nfs/read.c +8 -0
+       if (!list_empty(&head)) {
+               int err = nfs_pagein_list(&head, server->rpages);
+               if (!ret)
+                       ret = err;
+       }
+       return ret;
+}
+
author
#2 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

Andrew Morton <[email protected]> writes:

> Or should it be

> while (!list_empty(...))

Duh. No idea how that crept in.

Concerning your other query: the code is correct as it stands since nfs_pagein_list() is guaranteed to drain the entire list.

fs/nfs/read.c renamed from fs/nfs/read.c.orig +1 -1
@@ -387,7 +387,7 @@
 			       is_sync ? readpage_sync_filler :
 					 readpage_async_filler,
 			       &desc);
-	if (!list_empty(pages)) {
+	while (!list_empty(pages)) {
 		struct page *page = list_entry(pages->prev, struct page, list);
 		list_del(&page->list);
 		page_cache_release(page);