Folks, looks like we can simplify both the callers of read_cache_page
and function itself. Without breaking existing code.
a) all but two callers do the following:
page = read_cache_page(...);
if (IS_ERR(page))
goto fail;
wait_on_page(page);
if (!PageUptodate(page) {
page_cache_release(page);
page = ERR_PTR(-EIO);
goto fail;
}
or equivalents of that.
b) two exceptions are in NFS readdir and symlink handling resp. There we
do not call wait_on_page(). However, in both cases filler _always_ unlocks
page before returning. I.e. wait_on_page() would be a no-op - could as
well have it there.
c) after moving the wait_on_page() and check for page being not uptodate
after it into the read_cache_page() the function itself becomes simpler.
I've substituted __read_cache_page() in place of its call and did an
obvious cleanup of the result.
The bottom line: __read_cache_page() is gone, callers of read_cache_page()
dropped the waiting/handling of async errors, read_cache_page() returns
either an uptodate page or error.
Notice that read_cache_page() never was really async - only if we had a
page already in pagecache, not locked and not uptodate. BTW, if page
was _not_ in pagecache and filler got an asynchronous error we used
to cald it again. For no good reason. AFAICS patch below cleans the things
up and shouldn't break anything - code that used to work should work with
new variant. I'd definitely like to see it applied - IMO it counts as
obvious cleanup... Comments?
Cheers,
Al
Patch (against 2.4.3-pre3) follows:
diff -urN S3-pre3/fs/namei.c S3-pre3-read_cache_page/fs/namei.c
--- S3-pre3/fs/namei.c Fri Feb 16 21:20:06 2001
+++ S3-pre3-read_cache_page/fs/namei.c Sat Mar 10 00:47:09 2001
@@ -1952,18 +1952,11 @@
page = read_cache_page(mapping, 0, (filler_t *)mapping->a_ops->readpage,
NULL);
if (IS_ERR(page))
- goto sync_fail;
- wait_on_page(page);
- if (!Page_Uptodate(page))
- goto async_fail;
+ goto fail;
*ppage = page;
return kmap(page);
-async_fail:
- page_cache_release(page);
- return ERR_PTR(-EIO);
-
-sync_fail:
+fail:
return (char*)page;
}
diff -urN S3-pre3/fs/nfs/dir.c S3-pre3-read_cache_page/fs/nfs/dir.c
--- S3-pre3/fs/nfs/dir.c Thu Mar 8 06:33:59 2001
+++ S3-pre3-read_cache_page/fs/nfs/dir.c Sat Mar 10 00:47:45 2001
@@ -197,8 +197,6 @@
status = PTR_ERR(page);
goto out;
}
- if (!Page_Uptodate(page))
- goto read_error;
/* NOTE: Someone else may have changed the READDIRPLUS flag */
desc->plus = NFS_USE_READDIRPLUS(inode);
@@ -210,9 +208,6 @@
out:
dfprintk(VFS, "NFS: find_dirent_page() returns %d\n", status);
return status;
- read_error:
- page_cache_release(page);
- return -EIO;
}
/*
diff -urN S3-pre3/fs/nfs/symlink.c S3-pre3-read_cache_page/fs/nfs/symlink.c
--- S3-pre3/fs/nfs/symlink.c Fri Feb 16 22:52:05 2001
+++ S3-pre3-read_cache_page/fs/nfs/symlink.c Sat Mar 10 00:48:02 2001
@@ -64,15 +64,10 @@
(filler_t *)nfs_symlink_filler, inode);
if (IS_ERR(page))
goto read_failed;
- if (!Page_Uptodate(page))
- goto getlink_read_error;
*ppage = page;
p = kmap(page);
return (char*)(p+1);
-getlink_read_error:
- page_cache_release(page);
- return ERR_PTR(-EIO);
read_failed:
return (char*)page;
}
diff -urN S3-pre3/fs/umsdos/dir.c S3-pre3-read_cache_page/fs/umsdos/dir.c
--- S3-pre3/fs/umsdos/dir.c Fri Feb 16 22:52:06 2001
+++ S3-pre3-read_cache_page/fs/umsdos/dir.c Sat Mar 10 00:48:45 2001
@@ -692,9 +692,6 @@
dentry_dst=(struct dentry *)page;
if (IS_ERR(page))
goto out;
- wait_on_page(page);
- if (!Page_Uptodate(page))
- goto async_fail;
dentry_dst = ERR_PTR(-ENOMEM);
path = (char *) kmalloc (PATH_MAX, GFP_KERNEL);
@@ -776,8 +773,6 @@
dput(hlink); /* original hlink no longer needed */
return dentry_dst;
-async_fail:
- dentry_dst = ERR_PTR(-EIO);
out_release:
page_cache_release(page);
goto out;
diff -urN S3-pre3/fs/umsdos/emd.c S3-pre3-read_cache_page/fs/umsdos/emd.c
--- S3-pre3/fs/umsdos/emd.c Thu Mar 8 06:33:59 2001
+++ S3-pre3-read_cache_page/fs/umsdos/emd.c Sat Mar 10 00:51:54 2001
@@ -125,9 +125,6 @@
(filler_t*)mapping->a_ops->readpage, NULL);
if (IS_ERR(page))
goto sync_fail;
- wait_on_page(page);
- if (!Page_Uptodate(page))
- goto async_fail;
p = (struct umsdos_dirent*)(kmap(page)+offs);
/* if this is an invalid entry (invalid name length), ignore it */
@@ -150,12 +147,6 @@
page = page2;
goto sync_fail;
}
- wait_on_page(page2);
- if (!Page_Uptodate(page2)) {
- kunmap(page);
- page_cache_release(page2);
- goto async_fail;
- }
memcpy(entry->spare,p->spare,part);
memcpy(entry->spare+part,kmap(page2),
recsize+offs-PAGE_CACHE_SIZE);
@@ -168,9 +159,6 @@
page_cache_release(page);
*pos += recsize;
return ret;
-async_fail:
- page_cache_release(page);
- page = ERR_PTR(-EIO);
sync_fail:
return PTR_ERR(page);
}
@@ -395,9 +383,6 @@
page = read_cache_page(mapping,index,readpage,NULL);
if (IS_ERR(page))
goto sync_fail;
- wait_on_page(page);
- if (!Page_Uptodate(page))
- goto async_fail;
p = kmap(page);
}
@@ -443,12 +428,6 @@
page_cache_release(page);
page = next_page;
goto sync_fail;
- }
- wait_on_page(next_page);
- if (!Page_Uptodate(next_page)) {
- page_cache_release(page);
- page = next_page;
- goto async_fail;
}
q = kmap(next_page);
if (memcmp(entry->name, rentry->name, len) ||
diff -urN S3-pre3/mm/filemap.c S3-pre3-read_cache_page/mm/filemap.c
--- S3-pre3/mm/filemap.c Thu Mar 8 06:34:01 2001
+++ S3-pre3-read_cache_page/mm/filemap.c Sat Mar 10 01:38:21 2001
@@ -2306,8 +2306,11 @@
return error;
}
-static inline
-struct page *__read_cache_page(struct address_space *mapping,
+/*
+ * Make sure that @mapping contains a page with index @index, creating a
+ * new one if needed. If that page is not uptodate - try to fill it.
+ */
+struct page *read_cache_page(struct address_space *mapping,
unsigned long index,
int (*filler)(void *,struct page*),
void *data)
@@ -2315,7 +2318,8 @@
struct page **hash = page_hash(mapping, index);
struct page *page, *cached_page = NULL;
int err;
-repeat:
+
+retry:
page = __find_get_page(mapping, index, hash);
if (!page) {
if (!cached_page) {
@@ -2325,53 +2329,38 @@
}
page = cached_page;
if (add_to_page_cache_unique(page, mapping, index, hash))
- goto repeat;
+ goto retry;
cached_page = NULL;
- err = filler(data, page);
- if (err < 0) {
+ } else {
+ lock_page(page);
+ if (!page->mapping) {
+ UnlockPage(page);
page_cache_release(page);
- page = ERR_PTR(err);
+ goto retry;
+ }
+ if (Page_Uptodate(page)) {
+ UnlockPage(page);
+ goto out1;
}
}
- if (cached_page)
- page_cache_free(cached_page);
- return page;
-}
-/*
- * Read into the page cache. If a page already exists,
- * and Page_Uptodate() is not set, try to fill the page.
- */
-struct page *read_cache_page(struct address_space *mapping,
- unsigned long index,
- int (*filler)(void *,struct page*),
- void *data)
-{
- struct page *page;
- int err;
-
-retry:
- page = __read_cache_page(mapping, index, filler, data);
- if (IS_ERR(page) || Page_Uptodate(page))
- goto out;
-
- lock_page(page);
- if (!page->mapping) {
- UnlockPage(page);
- page_cache_release(page);
- goto retry;
- }
- if (Page_Uptodate(page)) {
- UnlockPage(page);
- goto out;
- }
err = filler(data, page);
if (err < 0) {
page_cache_release(page);
page = ERR_PTR(err);
+ goto out1;
}
- out:
+ wait_on_page(page);
+ if (!Page_Uptodate(page))
+ goto async_fail;
+out1:
+ if (cached_page)
+ page_cache_free(cached_page);
return page;
+async_fail:
+ page_cache_release(page);
+ page = ERR_PTR(-EIO);
+ goto out1;
}
static inline struct page * __grab_cache_page(struct address_space *mapping,
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Thu Mar 15 2001 - 21:00:11 EST