login
Search
Nick Piggin recently changed the read_cache_page interface to be synchronous, which is pretty much what the file systems want anyway. Turns out that they have more in common than that, though, and some of them want to be able to get an uptodate *locked* page. Many of them want a kmapped page, which is uptodate and unlocked, and they all have their own individual helper functions to achieve this. Since the helper functions are so similar, this patch just combines them into a small number of simple library functions, which call read_cache_page (renamed to __read_cache_page because it now returns a locked page). The immediate result is a vast reduction in the number of fs-specific helper functions. The secondary goal is to reduce the number of places the page lock is taken, and eliminate a lot of PageUptodate and PageError checks. The file systems that still use PageChecked now have checker functions that return an error if the page is corrupted or has some other error. This simplifies the logic since the checker function is not part of any helper function anymore. Compile tested on x86_64. Signed-off-by: Nate Diller <nate.diller@gmail.com> --- drivers/mtd/devices/block2mtd.c | 28 +------ fs/afs/dir.c | 56 +++----------- fs/afs/mntpt.c | 10 -- fs/cramfs/inode.c | 3 fs/ext2/dir.c | 82 ++++++++------------- fs/freevxfs/vxfs_extern.h | 1 fs/freevxfs/vxfs_inode.c | 2 fs/freevxfs/vxfs_lookup.c | 4 - fs/freevxfs/vxfs_subr.c | 33 -------- fs/hfs/bnode.c | 4 - fs/hfsplus/bnode.c | 4 - fs/jffs2/fs.c | 27 ------- fs/jffs2/gc.c | 15 ++- fs/jfs/jfs_metapage.c | 5 - fs/minix/dir.c | 59 ++++----------- ...
Export a single version of read_cache_page, which returns with a locked,
Uptodate page or a synchronous error, and use inline helper functions to
replicate the old behavior. Also, introduce new helper functions for the
most common file system uses, which include kmapping the page, as well as
needing to keep the page locked. These changes collectively eliminate a
substantial amount of private fs logic in favor of generic code.
It also simplifies filemap.c significantly, by assuming that callers want
synchronous behavior, which is true for all callers anyway except one.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/include/linux/pagemap.h linux-2.6.21-rc6-mm1-test/include/linux/pagemap.h
--- linux-2.6.21-rc6-mm1/include/linux/pagemap.h 2007-04-11 14:22:19.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/include/linux/pagemap.h 2007-04-11 14:29:31.000000000 -0700
@@ -108,21 +108,30 @@ static inline struct page *grab_cache_pa
extern struct page * grab_cache_page_nowait(struct address_space *mapping,
unsigned long index);
-extern struct page * read_cache_page_async(struct address_space *mapping,
- unsigned long index, filler_t *filler,
- void *data);
-extern struct page * read_cache_page(struct address_space *mapping,
+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);
-static inline struct page *read_mapping_page_async(
- struct address_space *mapping,
+void fastcall unlock_page(struct page *page);
+static inline struct page *read_cache_page(struct address_space *mapping,
+ unsigned long index, filler_t *filler,
+ void *data)
+{
+ struct page *page;
+
+ page = __read_cache_page(mapping, index, filler, data);
+ if (!IS_ERR(page))
+ unlock_page(page);
+ return page;
+}
+
+static inline struct ...Replace ext2_get_page() and ext2_put_page() using the new read_kmap_page()
and put_kmapped_page() calls. Also, change the ext2_check_page() call to
return the page's error status, and update the call sites accordingly.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/ext2/dir.c linux-2.6.21-rc5-mm4-test/fs/ext2/dir.c
--- linux-2.6.21-rc5-mm4/fs/ext2/dir.c 2007-04-06 12:27:03.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/ext2/dir.c 2007-04-06 14:34:23.000000000 -0700
@@ -35,12 +35,6 @@ static inline unsigned ext2_chunk_size(s
return inode->i_sb->s_blocksize;
}
-static inline void ext2_put_page(struct page *page)
-{
- kunmap(page);
- page_cache_release(page);
-}
-
static inline unsigned long dir_pages(struct inode *inode)
{
return (inode->i_size+PAGE_CACHE_SIZE-1)>>PAGE_CACHE_SHIFT;
@@ -74,7 +68,7 @@ static int ext2_commit_chunk(struct page
return err;
}
-static void ext2_check_page(struct page *page)
+static int ext2_check_page(struct page *page)
{
struct inode *dir = page->mapping->host;
struct super_block *sb = dir->i_sb;
@@ -86,6 +80,14 @@ static void ext2_check_page(struct page
ext2_dirent *p;
char *error;
+ if (likely(PageChecked(page))) {
+ if (likely(!PageError(page)))
+ return 0;
+
+ put_kmapped_page(page);
+ return -EIO;
+ }
+
if ((dir->i_size >> PAGE_CACHE_SHIFT) == page->index) {
limit = dir->i_size & ~PAGE_CACHE_MASK;
if (limit & (chunk_size - 1))
@@ -112,7 +114,7 @@ static void ext2_check_page(struct page
goto Eend;
out:
SetPageChecked(page);
- return;
+ return 0;
/* Too bad, we had an error */
@@ -153,24 +155,8 @@ Eend:
fail:
SetPageChecked(page);
SetPageError(page);
-}
-
-static struct page * ext2_get_page(struct inode *dir, unsigned long n)
-{
- struct address_space *mapping = dir->i_mapping;
- struct page *page = read_mapping_page(mapping, n, NULL);
- if (!IS_ERR(page)) {
- kmap(page);
- if ...Replace afs_dir_get_page() and afs_dir_put_page() using the new
read_kmap_page() and put_kmapped_page() calls, and eliminate unnecessary
PageError checks. Also, change the afs_dir_check_page() call to return
the page's error status, and update the call site accordingly.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/afs/dir.c linux-2.6.21-rc5-mm4-test/fs/afs/dir.c
--- linux-2.6.21-rc5-mm4/fs/afs/dir.c 2007-04-06 12:27:03.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/afs/dir.c 2007-04-06 14:30:22.000000000 -0700
@@ -115,12 +115,15 @@ struct afs_dir_lookup_cookie {
/*
* check that a directory page is valid
*/
-static inline void afs_dir_check_page(struct inode *dir, struct page *page)
+static inline int afs_dir_check_page(struct inode *dir, struct page *page)
{
struct afs_dir_page *dbuf;
loff_t latter;
int tmp, qty;
+ if (likely(PageChecked(page)))
+ return PageError(page);
+
#if 0
/* check the page count */
qty = desc.size / sizeof(dbuf->blocks[0]);
@@ -154,52 +157,16 @@ static inline void afs_dir_check_page(st
}
SetPageChecked(page);
- return;
+ return 0;
error:
SetPageChecked(page);
SetPageError(page);
-
+ return 1;
} /* end afs_dir_check_page() */
/*****************************************************************************/
/*
- * discard a page cached in the pagecache
- */
-static inline void afs_dir_put_page(struct page *page)
-{
- kunmap(page);
- page_cache_release(page);
-
-} /* end afs_dir_put_page() */
-
-/*****************************************************************************/
-/*
- * get a page into the pagecache
- */
-static struct page *afs_dir_get_page(struct inode *dir, unsigned long index)
-{
- struct page *page;
-
- _enter("{%lu},%lu", dir->i_ino, index);
-
- page = read_mapping_page(dir->i_mapping, index, NULL);
- if (!IS_ERR(page)) {
- kmap(page);
- if (!PageChecked(page))
- afs_dir_check_page(dir, page);
- if ...NAK. This conflicts with my AFS security patches, and eliminates any way of passing the key through to readpage(). David -
Hmmm you're right. Is your security work going into the next -mm? If so, I'll just re-base this cleanup patch on that ... at the very least I want to get rid of afs_dir_put_page(). Also, did you consider passing the key pointer directly and modifying the readpage actor to simply cast the pointer back, like read_mapping_page(mapping, page, (struct file *)key)? It seems like a waste to allocate a whole file struct on the stack just for the ->private field. Andrew in the mean time just disregard this patch. NATE -
I don't know. Andrew hasn't said anything. Andrew? Are you waiting for it There's one small problem with that... And that's filemap_nopage() (it passes vma->vm_file to readpage() unconditionally). Unless, of course, your patches fix that too... David -
On Thu, 12 Apr 2007 19:57:23 +0100 AF_RXRPC is a davem thing and "AFS: Add security support and fix bugs" has dependencise upon it. So we're waiting for you and the networking guys to sort all that out. Or am I wrong?? -
I thought this was probably the case, but no-one's actually said what the Doubtful. David -
Hi Andrew, I would request you to drop the patch update-isdn-tree-to-use-pci_get_device.patch from the -mm tree since a new bug is been identified in that by jeff. thanks. surya. -
But you can't mmap() a directory anyway so ... oh. Interesting.
afs_file_readpage() does directories too. The only thing I can think
of then is
struct address_space_operations afs_file_aops = {
.readpage = afs_file_readpage,
}
struct address_space_operations afs_dir_aops = {
.readpage = afs_key_readpage,
}
int afs_file_readpage(file, page){
return afs_key_readpage(file->private, page)
}
but that's a lot of code to avoid a single stack allocation. The
whole fake file pointer thing still strikes me as a little ugly, and
you're definitely not the first one who needed this sort of hackery.
ugh
NATE
-
A better way might be to stick a void * in struct file and pass that through
to readpage() and readpages() instead of the struct file *. That way, anyone
who wants the traditional arrangement can just point that extra void * at the
struct file.
Of course, I'm in favour of making it a struct key * like this:
struct address_space_operations {
...
int (*readpage)(struct key *, struct page *);
...
int (*readpages)(struct key *, struct address_space *,
struct list_head *, unsigned);
...
};
struct file {
...
struct key *f_key;
};
struct page *filemap_nopage(struct vm_area_struct *area, ...)
{
...
struct file *file = area->vm_file;
...
error = mapping->a_ops->readpage(file->f_key, page);
...
}
But I'm not sure the NFS crew, for instance, would be happy with that. Maybe
passing file->private_data through would do. That's basically what NFS and
FUSE, for instance, want, and it would also do for AFS.
David
-
Use the new locking variant of read_mapping_page to avoid doing extra work.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/jfs/jfs_metapage.c linux-2.6.21-rc6-mm1-test/fs/jfs/jfs_metapage.c
--- linux-2.6.21-rc6-mm1/fs/jfs/jfs_metapage.c 2007-04-09 17:23:48.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/jfs/jfs_metapage.c 2007-04-09 21:37:09.000000000 -0700
@@ -632,12 +632,11 @@ struct metapage *__get_metapage(struct i
}
SetPageUptodate(page);
} else {
- page = read_mapping_page(mapping, page_index, NULL);
- if (IS_ERR(page) || !PageUptodate(page)) {
+ page = __read_mapping_page(mapping, page_index, NULL);
+ if (IS_ERR(page)) {
jfs_err("read_mapping_page failed!");
return NULL;
}
- lock_page(page);
}
mp = page_to_mp(page, page_offset);
-
Replace minix dir_get_page() and dir_put_page() using the new
read_kmap_page() and put_kmapped_page()/put_locked_page() calls. Also, use
__read_kmap_page() instead of re-taking the page_lock.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/minix/dir.c linux-2.6.21-rc5-mm4-test/fs/minix/dir.c
--- linux-2.6.21-rc5-mm4/fs/minix/dir.c 2007-04-05 17:14:25.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/minix/dir.c 2007-04-06 02:31:55.000000000 -0700
@@ -23,12 +23,6 @@ const struct file_operations minix_dir_o
.fsync = minix_sync_file,
};
-static inline void dir_put_page(struct page *page)
-{
- kunmap(page);
- page_cache_release(page);
-}
-
/*
* Return the offset into page `page_nr' of the last valid
* byte in that page, plus one.
@@ -60,22 +54,6 @@ static int dir_commit_chunk(struct page
return err;
}
-static struct page * dir_get_page(struct inode *dir, unsigned long n)
-{
- struct address_space *mapping = dir->i_mapping;
- struct page *page = read_mapping_page(mapping, n, NULL);
- if (!IS_ERR(page)) {
- kmap(page);
- if (!PageUptodate(page))
- goto fail;
- }
- return page;
-
-fail:
- dir_put_page(page);
- return ERR_PTR(-EIO);
-}
-
static inline void *minix_next_entry(void *de, struct minix_sb_info *sbi)
{
return (void*)((char*)de + sbi->s_dirsize);
@@ -102,7 +80,7 @@ static int minix_readdir(struct file * f
for ( ; n < npages; n++, offset = 0) {
char *p, *kaddr, *limit;
- struct page *page = dir_get_page(inode, n);
+ struct page *page = read_kmap_page(inode->i_mapping, n);
if (IS_ERR(page))
continue;
@@ -128,12 +106,12 @@ static int minix_readdir(struct file * f
(n << PAGE_CACHE_SHIFT) | offset,
inumber, DT_UNKNOWN);
if (over) {
- dir_put_page(page);
+ put_kmapped_page(page);
goto done;
}
}
}
- dir_put_page(page);
+ put_kmapped_page(page);
}
done:
@@ -177,7 +155,7 @@ minix_dirent ...Replace page_read() with read_kmap_page()/__read_kmap_page(). This probably
fixes behaviour on highmem systems, since page_address() was being used
without kmap(). Also eliminate the need to re-take the page lock during
writes to the page.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/drivers/mtd/devices/block2mtd.c linux-2.6.21-rc5-mm4-test/drivers/mtd/devices/block2mtd.c
--- linux-2.6.21-rc5-mm4/drivers/mtd/devices/block2mtd.c 2007-04-05 17:14:24.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/drivers/mtd/devices/block2mtd.c 2007-04-06 01:59:19.000000000 -0700
@@ -39,12 +39,6 @@ struct block2mtd_dev {
/* Static info about the MTD, used in cleanup_module */
static LIST_HEAD(blkmtd_device_list);
-
-static struct page *page_read(struct address_space *mapping, int index)
-{
- return read_mapping_page(mapping, index, NULL);
-}
-
/* erase a specified part of the device */
static int _block2mtd_erase(struct block2mtd_dev *dev, loff_t to, size_t len)
{
@@ -56,23 +50,19 @@ static int _block2mtd_erase(struct block
u_long *max;
while (pages) {
- page = page_read(mapping, index);
- if (!page)
- return -ENOMEM;
+ page = __read_kmap_page(mapping, index);
if (IS_ERR(page))
return PTR_ERR(page);
max = page_address(page) + PAGE_SIZE;
for (p=page_address(page); p<max; p++)
if (*p != -1UL) {
- lock_page(page);
memset(page_address(page), 0xff, PAGE_SIZE);
set_page_dirty(page);
- unlock_page(page);
break;
}
- page_cache_release(page);
+ put_locked_page(page);
pages--;
index++;
}
@@ -125,14 +115,12 @@ static int block2mtd_read(struct mtd_inf
cpylen = len; // this page
len = len - cpylen;
- page = page_read(dev->blkdev->bd_inode->i_mapping, index);
- if (!page)
- return -ENOMEM;
+ page = read_kmap_page(dev->blkdev->bd_inode->i_mapping, index);
if (IS_ERR(page))
return PTR_ERR(page);
memcpy(buf, page_address(page) + ...read_mapping_page() is now fully synchronous, so there's no need wait for
the page lock or check for I/O errors.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/reiser4/plugin/file/tail_conversion.c linux-2.6.21-rc6-mm1-test/fs/reiser4/plugin/file/tail_conversion.c
--- linux-2.6.21-rc6-mm1/fs/reiser4/plugin/file/tail_conversion.c 2007-04-09 17:24:03.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/reiser4/plugin/file/tail_conversion.c 2007-04-10 21:33:47.000000000 -0700
@@ -608,14 +608,6 @@ int extent2tail(unix_file_info_t *uf_inf
break;
}
- wait_on_page_locked(page);
-
- if (!PageUptodate(page)) {
- page_cache_release(page);
- result = RETERR(-EIO);
- break;
- }
-
/* cut part of file we have read */
start_byte = (__u64) (i << PAGE_CACHE_SHIFT);
set_key_offset(&from, start_byte);
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/reiser4/plugin/item/extent_file_ops.c linux-2.6.21-rc6-mm1-test/fs/reiser4/plugin/item/extent_file_ops.c
--- linux-2.6.21-rc6-mm1/fs/reiser4/plugin/item/extent_file_ops.c 2007-04-10 19:41:14.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/reiser4/plugin/item/extent_file_ops.c 2007-04-10 21:38:41.000000000 -0700
@@ -1220,15 +1220,8 @@ int reiser4_read_extent(struct file *fil
page = read_mapping_page(mapping, cur_page, file);
if (IS_ERR(page))
return PTR_ERR(page);
- lock_page(page);
- if (!PageUptodate(page)) {
- unlock_page(page);
- page_cache_release(page);
- warning("jmacd-97178", "extent_read: page is not up to date");
- return RETERR(-EIO);
- }
+
mark_page_accessed(page);
- unlock_page(page);
/* If users can be writing to this page using arbitrary virtual
addresses, take care about potential aliasing before reading
-
Remove unneeded PageError checking in read_dev_sector(), and clean up the
code a bit.
Can anyone point out why it's OK to use page_address() here on a page which
has not been kmapped? If it's not OK, then a good number of callers need to
be fixed.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/partitions/check.c linux-2.6.21-rc6-mm1-test/fs/partitions/check.c
--- linux-2.6.21-rc6-mm1/fs/partitions/check.c 2007-04-09 17:24:03.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/partitions/check.c 2007-04-10 21:59:01.000000000 -0700
@@ -568,16 +568,12 @@ unsigned char *read_dev_sector(struct bl
page = read_mapping_page(mapping, (pgoff_t)(n >> (PAGE_CACHE_SHIFT-9)),
NULL);
- if (!IS_ERR(page)) {
- if (PageError(page))
- goto fail;
- p->v = page;
- return (unsigned char *)page_address(page) + ((n & ((1 << (PAGE_CACHE_SHIFT - 9)) - 1)) << 9);
-fail:
- page_cache_release(page);
+ if (IS_ERR(page)) {
+ p->v = NULL;
+ return NULL;
}
- p->v = NULL;
- return NULL;
+ p->v = page;
+ return (unsigned char *)page_address(page) + ((n & ((1 << (PAGE_CACHE_SHIFT - 9)) - 1)) << 9);
}
EXPORT_SYMBOL(read_dev_sector);
-
Replace sysv dir_get_page() with the new read_kmap_page().
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/sysv/dir.c linux-2.6.21-rc5-mm4-test/fs/sysv/dir.c
--- linux-2.6.21-rc5-mm4/fs/sysv/dir.c 2007-04-05 17:14:25.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/sysv/dir.c 2007-04-06 01:59:19.000000000 -0700
@@ -50,15 +50,6 @@ static int dir_commit_chunk(struct page
return err;
}
-static struct page * dir_get_page(struct inode *dir, unsigned long n)
-{
- struct address_space *mapping = dir->i_mapping;
- struct page *page = read_mapping_page(mapping, n, NULL);
- if (!IS_ERR(page))
- kmap(page);
- return page;
-}
-
static int sysv_readdir(struct file * filp, void * dirent, filldir_t filldir)
{
unsigned long pos = filp->f_pos;
@@ -77,7 +68,7 @@ static int sysv_readdir(struct file * fi
for ( ; n < npages; n++, offset = 0) {
char *kaddr, *limit;
struct sysv_dir_entry *de;
- struct page *page = dir_get_page(inode, n);
+ struct page *page = read_kmap_page(inode->i_mapping, n);
if (IS_ERR(page))
continue;
@@ -149,7 +140,7 @@ struct sysv_dir_entry *sysv_find_entry(s
do {
char *kaddr;
- page = dir_get_page(dir, n);
+ page = read_kmap_page(dir->i_mapping, n);
if (!IS_ERR(page)) {
kaddr = (char*)page_address(page);
de = (struct sysv_dir_entry *) kaddr;
@@ -191,7 +182,7 @@ int sysv_add_link(struct dentry *dentry,
/* We take care of directory expansion in the same loop */
for (n = 0; n <= npages; n++) {
- page = dir_get_page(dir, n);
+ page = read_kmap_page(dir->i_mapping, n);
err = PTR_ERR(page);
if (IS_ERR(page))
goto out;
@@ -299,7 +290,7 @@ int sysv_empty_dir(struct inode * inode)
for (i = 0; i < npages; i++) {
char *kaddr;
struct sysv_dir_entry * de;
- page = dir_get_page(inode, i);
+ page = read_kmap_page(inode->i_mapping, i);
if (IS_ERR(page))
continue;
@@ -353,7 +344,7 @@ void sysv_set_link(struct ...Replace ufs_get_page()/ufs_get_locked_page() and
ufs_put_page()/ufs_put_locked_page() using the new read_kmap_page() and
put_kmapped_page() calls and their locking variants. Also, change the
ufs_check_page() call to return the page's error status, and update the
call sites accordingly.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/ufs/balloc.c linux-2.6.21-rc5-mm4-test/fs/ufs/balloc.c
--- linux-2.6.21-rc5-mm4/fs/ufs/balloc.c 2007-04-05 17:13:29.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/ufs/balloc.c 2007-04-06 12:46:02.000000000 -0700
@@ -272,7 +272,7 @@ static void ufs_change_blocknr(struct in
index = i >> (PAGE_CACHE_SHIFT - inode->i_blkbits);
if (likely(cur_index != index)) {
- page = ufs_get_locked_page(mapping, index);
+ page = __read_mapping_page(mapping, index, NULL);
if (!page)/* it was truncated */
continue;
if (IS_ERR(page)) {/* or EIO */
@@ -325,8 +325,10 @@ static void ufs_change_blocknr(struct in
bh = bh->b_this_page;
} while (bh != head);
- if (likely(cur_index != index))
- ufs_put_locked_page(page);
+ if (likely(cur_index != index)) {
+ unlock_page(page);
+ page_cache_release(page);
+ }
}
UFSD("EXIT\n");
}
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/ufs/truncate.c linux-2.6.21-rc5-mm4-test/fs/ufs/truncate.c
--- linux-2.6.21-rc5-mm4/fs/ufs/truncate.c 2007-04-05 17:13:29.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/ufs/truncate.c 2007-04-06 12:46:14.000000000 -0700
@@ -395,8 +395,9 @@ static int ufs_alloc_lastblock(struct in
lastfrag--;
- lastpage = ufs_get_locked_page(mapping, lastfrag >>
- (PAGE_CACHE_SHIFT - inode->i_blkbits));
+ lastpage = __read_mapping_page(mapping, lastfrag >>
+ (PAGE_CACHE_SHIFT - inode->i_blkbits),
+ NULL);
if (IS_ERR(lastpage)) {
err = -EIO;
goto out;
@@ -441,7 +442,8 @@ static int ufs_alloc_lastblock(struct in
}
...Replace reiserfs_get_page() and reiserfs_put_page() using the new
read_kmap_page() and put_kmapped_page() calls and their locking variants.
Also, propagate the gfp_mask() deadlock comment to callsites.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/reiserfs/xattr.c linux-2.6.21-rc5-mm4-test/fs/reiserfs/xattr.c
--- linux-2.6.21-rc5-mm4/fs/reiserfs/xattr.c 2007-04-05 17:14:25.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/reiserfs/xattr.c 2007-04-06 14:41:34.000000000 -0700
@@ -438,33 +438,6 @@ int xattr_readdir(struct file *file, fil
return res;
}
-/* Internal operations on file data */
-static inline void reiserfs_put_page(struct page *page)
-{
- kunmap(page);
- page_cache_release(page);
-}
-
-static struct page *reiserfs_get_page(struct inode *dir, unsigned long n)
-{
- struct address_space *mapping = dir->i_mapping;
- struct page *page;
- /* We can deadlock if we try to free dentries,
- and an unlink/rmdir has just occured - GFP_NOFS avoids this */
- mapping_set_gfp_mask(mapping, GFP_NOFS);
- page = read_mapping_page(mapping, n, NULL);
- if (!IS_ERR(page)) {
- kmap(page);
- if (PageError(page))
- goto fail;
- }
- return page;
-
- fail:
- reiserfs_put_page(page);
- return ERR_PTR(-EIO);
-}
-
static inline __u32 xattr_hash(const char *msg, int len)
{
return csum_partial(msg, len, 0);
@@ -537,13 +510,15 @@ reiserfs_xattr_set(struct inode *inode,
else
chunk = buffer_size - buffer_pos;
- page = reiserfs_get_page(xinode, file_pos >> PAGE_CACHE_SHIFT);
+ /* We can deadlock if we try to free dentries,
+ and an unlink/rmdir has just occured - GFP_NOFS avoids this */
+ mapping_set_gfp_mask(mapping, GFP_NOFS);
+ page = __read_kmap_page(mapping, file_pos >> PAGE_CACHE_SHIFT);
if (IS_ERR(page)) {
err = PTR_ERR(page);
goto out_filp;
}
- lock_page(page);
data = page_address(page);
if (file_pos == 0) {
@@ -566,8 +541,7 @@ ...Replace vxfs_get_page() with the new read_kmap_page().
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/freevxfs/vxfs_extern.h linux-2.6.21-rc5-mm4-test/fs/freevxfs/vxfs_extern.h
--- linux-2.6.21-rc5-mm4/fs/freevxfs/vxfs_extern.h 2007-04-05 17:13:29.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/freevxfs/vxfs_extern.h 2007-04-06 01:59:19.000000000 -0700
@@ -69,7 +69,6 @@ extern const struct file_operations vxfs
extern int vxfs_read_olt(struct super_block *, u_long);
/* vxfs_subr.c */
-extern struct page * vxfs_get_page(struct address_space *, u_long);
extern void vxfs_put_page(struct page *);
extern struct buffer_head * vxfs_bread(struct inode *, int);
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/freevxfs/vxfs_inode.c linux-2.6.21-rc5-mm4-test/fs/freevxfs/vxfs_inode.c
--- linux-2.6.21-rc5-mm4/fs/freevxfs/vxfs_inode.c 2007-04-05 17:14:25.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/freevxfs/vxfs_inode.c 2007-04-06 01:59:19.000000000 -0700
@@ -138,7 +138,7 @@ __vxfs_iget(ino_t ino, struct inode *ili
u_long offset;
offset = (ino % (PAGE_SIZE / VXFS_ISIZE)) * VXFS_ISIZE;
- pp = vxfs_get_page(ilistp->i_mapping, ino * VXFS_ISIZE / PAGE_SIZE);
+ pp = read_kmap_page(ilistp->i_mapping, ino * VXFS_ISIZE / PAGE_SIZE);
if (!IS_ERR(pp)) {
struct vxfs_inode_info *vip;
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/freevxfs/vxfs_lookup.c linux-2.6.21-rc5-mm4-test/fs/freevxfs/vxfs_lookup.c
--- linux-2.6.21-rc5-mm4/fs/freevxfs/vxfs_lookup.c 2007-04-05 17:13:29.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/freevxfs/vxfs_lookup.c 2007-04-06 01:59:19.000000000 -0700
@@ -125,7 +125,7 @@ vxfs_find_entry(struct inode *ip, struct
caddr_t kaddr;
struct page *pp;
- pp = vxfs_get_page(ip->i_mapping, page);
+ pp = read_kmap_page(ip->i_mapping, page);
if (IS_ERR(pp))
continue;
kaddr = (caddr_t)page_address(pp);
@@ -280,7 +280,7 @@ vxfs_readdir(struct file *fp, void *retp
...Replace ntfs_map_page() and ntfs_unmap_page() using the new read_kmap_page()
and put_kmapped_page() calls, and their locking variants, and remove
unneeded PageError checking.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/ntfs/aops.h linux-2.6.21-rc5-mm4-test/fs/ntfs/aops.h
--- linux-2.6.21-rc5-mm4/fs/ntfs/aops.h 2007-04-05 17:14:25.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/ntfs/aops.h 2007-04-06 01:59:19.000000000 -0700
@@ -31,73 +31,6 @@
#include "inode.h"
-/**
- * ntfs_unmap_page - release a page that was mapped using ntfs_map_page()
- * @page: the page to release
- *
- * Unpin, unmap and release a page that was obtained from ntfs_map_page().
- */
-static inline void ntfs_unmap_page(struct page *page)
-{
- kunmap(page);
- page_cache_release(page);
-}
-
-/**
- * ntfs_map_page - map a page into accessible memory, reading it if necessary
- * @mapping: address space for which to obtain the page
- * @index: index into the page cache for @mapping of the page to map
- *
- * Read a page from the page cache of the address space @mapping at position
- * @index, where @index is in units of PAGE_CACHE_SIZE, and not in bytes.
- *
- * If the page is not in memory it is loaded from disk first using the readpage
- * method defined in the address space operations of @mapping and the page is
- * added to the page cache of @mapping in the process.
- *
- * If the page belongs to an mst protected attribute and it is marked as such
- * in its ntfs inode (NInoMstProtected()) the mst fixups are applied but no
- * error checking is performed. This means the caller has to verify whether
- * the ntfs record(s) contained in the page are valid or not using one of the
- * ntfs_is_XXXX_record{,p}() macros, where XXXX is the record type you are
- * expecting to see. (For details of the macros, see fs/ntfs/layout.h.)
- *
- * If the page is in high memory it is mapped into memory directly addressible
- * by the kernel.
- ...Now that read_mapping_page() does error checking internally, there is no
need to check PageError here.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/hfsplus/bnode.c linux-2.6.21-rc6-mm1-test/fs/hfsplus/bnode.c
--- linux-2.6.21-rc6-mm1/fs/hfsplus/bnode.c 2007-04-09 17:20:13.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/hfsplus/bnode.c 2007-04-10 21:28:45.000000000 -0700
@@ -442,10 +442,6 @@ static struct hfs_bnode *__hfs_bnode_cre
page = read_mapping_page(mapping, block, NULL);
if (IS_ERR(page))
goto fail;
- if (PageError(page)) {
- page_cache_release(page);
- goto fail;
- }
page_cache_release(page);
node->page[i] = page;
}
-
read_mapping_page_async() is going away, so convert its only user to
read_mapping_page(). This change has not been benchmarked, however, in
order to get real parallelism this wants something completely different,
like __do_page_cache_readahead(), which is not currently exported.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/cramfs/inode.c linux-2.6.21-rc6-mm1-test/fs/cramfs/inode.c
--- linux-2.6.21-rc6-mm1/fs/cramfs/inode.c 2007-04-09 17:24:03.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/cramfs/inode.c 2007-04-09 21:37:09.000000000 -0700
@@ -180,8 +180,7 @@ static void *cramfs_read(struct super_bl
struct page *page = NULL;
if (blocknr + i < devsize) {
- page = read_mapping_page_async(mapping, blocknr + i,
- NULL);
+ page = read_mapping_page(mapping, blocknr + i, NULL);
/* synchronous error? */
if (IS_ERR(page))
page = NULL;
-
Why is read_mapping_page_async going away? This probably needs a lot more testing, and I'd be much happier if you split it out of the series and sent it separately at the end. -
Hi, That function wasn't fully async anyway, as it would often sleep in lock_page(). AFAICT only in the special case of a partial written page would this function return a not yet uptodate page. bye, Roman -
yes, exactly, the structure of read_cache_page() and friends is totally not appropriate for doing async I/O to more than one page at a time, and the whole point of the special treatment in cramfs was to read 4 pages at once rather than synchronously reading each of the 4 seperately. read_cache_page_async() is totally wrong for that use, its purpose would be to get a reference to a single page that is likely to be in cache already without having to take the page_lock. Turns out nobody needs to do that, so there's no point in keeping it around. If the performance gain of reading all 4 pages at once would be worth the effort, this code should be using __do_page_cache_readahead(). That function allocates all the pages first, then reads them in asynchronously as a group. It is currently not exported. NATE -
Replace jffs2_gc_fetch_page() and jffs2_gc_release_page() using the
read_cache_page() and put_kmapped_page() calls, and update the call site
accordingly. Explicit calls to kmap()/kunmap() make the code more clear.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/jffs2/fs.c linux-2.6.21-rc5-mm4-test/fs/jffs2/fs.c
--- linux-2.6.21-rc5-mm4/fs/jffs2/fs.c 2007-04-05 17:14:25.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/jffs2/fs.c 2007-04-06 01:59:19.000000000 -0700
@@ -621,33 +621,6 @@ struct jffs2_inode_info *jffs2_gc_fetch_
return JFFS2_INODE_INFO(inode);
}
-unsigned char *jffs2_gc_fetch_page(struct jffs2_sb_info *c,
- struct jffs2_inode_info *f,
- unsigned long offset,
- unsigned long *priv)
-{
- struct inode *inode = OFNI_EDONI_2SFFJ(f);
- struct page *pg;
-
- pg = read_cache_page(inode->i_mapping, offset >> PAGE_CACHE_SHIFT,
- (void *)jffs2_do_readpage_unlock, inode);
- if (IS_ERR(pg))
- return (void *)pg;
-
- *priv = (unsigned long)pg;
- return kmap(pg);
-}
-
-void jffs2_gc_release_page(struct jffs2_sb_info *c,
- unsigned char *ptr,
- unsigned long *priv)
-{
- struct page *pg = (void *)*priv;
-
- kunmap(pg);
- page_cache_release(pg);
-}
-
static int jffs2_flash_setup(struct jffs2_sb_info *c) {
int ret = 0;
diff -urpN -X dontdiff linux-2.6.21-rc5-mm4/fs/jffs2/gc.c linux-2.6.21-rc5-mm4-test/fs/jffs2/gc.c
--- linux-2.6.21-rc5-mm4/fs/jffs2/gc.c 2007-04-05 17:13:10.000000000 -0700
+++ linux-2.6.21-rc5-mm4-test/fs/jffs2/gc.c 2007-04-06 01:59:19.000000000 -0700
@@ -1078,7 +1078,7 @@ static int jffs2_garbage_collect_dnode(s
uint32_t alloclen, offset, orig_end, orig_start;
int ret = 0;
unsigned char *comprbuf = NULL, *writebuf;
- unsigned long pg;
+ struct page *page;
unsigned char *pg_ptr;
memset(&ri, 0, sizeof(ri));
@@ -1219,12 +1219,16 @@ static int jffs2_garbage_collect_dnode(s
* page OK. We'll actually write it out again in ...wow, you're right. I was sure I compile-tested this ... oh, "depends on MTD". oops. thanks for reviewing. does it look OK to you otherwise? NATE -
Please don't remove the jffs2_gc_{fetch,release}_page functions. The
reason they're in a separate file is because that file (fs.c) is built
on Linux only, while the file you're moving the code into (gc.c) is
OS-agnostic; it's used on other sytems (eCos) too.
Rather than forcing eCos and any other systems which have a JFFS2 port
to implement a crappy Linux 'emulation' layer, I try to keep the
Linuxisms localised to certain files which can be completely
reimplemented for a non-Linux port.
--
dwmw2
-
Now that read_mapping_page() does error checking internally, there is no
need to check PageError here.
Signed-off-by: Nate Diller <nate.diller@gmail.com>
---
diff -urpN -X dontdiff linux-2.6.21-rc6-mm1/fs/hfs/bnode.c linux-2.6.21-rc6-mm1-test/fs/hfs/bnode.c
--- linux-2.6.21-rc6-mm1/fs/hfs/bnode.c 2007-04-09 17:20:13.000000000 -0700
+++ linux-2.6.21-rc6-mm1-test/fs/hfs/bnode.c 2007-04-10 21:28:03.000000000 -0700
@@ -282,10 +282,6 @@ static struct hfs_bnode *__hfs_bnode_cre
page = read_mapping_page(mapping, block++, NULL);
if (IS_ERR(page))
goto fail;
- if (PageError(page)) {
- page_cache_release(page);
- goto fail;
- }
page_cache_release(page);
node->page[i] = page;
}
-
