The return value of this function is never used, so let's
be honest and declare it as void.
Some places where invalidatepage returned 0, I have inserted
comments suggesting a BUG_ON.
Signed-off-by: Neil Brown <[email protected]>
### Diffstat output
./fs/afs/file.c | 6 +++---
./fs/buffer.c | 22 +++++++++++-----------
./fs/ext3/inode.c | 4 ++--
./fs/jbd/transaction.c | 12 +++++-------
./fs/jfs/jfs_metapage.c | 7 +++----
./fs/reiser4/as_ops.c | 13 ++++++-------
./fs/reiser4/vfs_ops.h | 2 +-
./fs/reiserfs/inode.c | 8 +++++---
./fs/xfs/linux-2.6/xfs_aops.c | 4 ++--
./include/linux/buffer_head.h | 4 ++--
./include/linux/fs.h | 2 +-
./include/linux/jbd.h | 2 +-
12 files changed, 42 insertions(+), 44 deletions(-)
diff ./fs/afs/file.c~current~ ./fs/afs/file.c
--- ./fs/afs/file.c~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/afs/file.c 2006-03-13 10:46:55.000000000 +1100
@@ -28,7 +28,7 @@ static int afs_file_release(struct inode
#endif
static int afs_file_readpage(struct file *file, struct page *page);
-static int afs_file_invalidatepage(struct page *page, unsigned long offset);
+static void afs_file_invalidatepage(struct page *page, unsigned long offset);
static int afs_file_releasepage(struct page *page, gfp_t gfp_flags);
struct inode_operations afs_file_inode_operations = {
@@ -212,7 +212,7 @@ int afs_cache_get_page_cookie(struct pag
/*
* invalidate part or all of a page
*/
-static int afs_file_invalidatepage(struct page *page, unsigned long offset)
+static void afs_file_invalidatepage(struct page *page, unsigned long offset)
{
int ret = 1;
@@ -238,11 +238,11 @@ static int afs_file_invalidatepage(struc
if (!PageWriteback(page))
ret = page->mapping->a_ops->releasepage(page,
0);
+ /* possibly should BUG_ON(!ret); - neilb */
}
}
_leave(" = %d", ret);
- return ret;
} /* end afs_file_invalidatepage() */
/*****************************************************************************/
diff ./fs/buffer.c~current~ ./fs/buffer.c
--- ./fs/buffer.c~current~ 2006-03-13 10:46:35.000000000 +1100
+++ ./fs/buffer.c 2006-03-13 10:46:55.000000000 +1100
@@ -1603,11 +1603,10 @@ EXPORT_SYMBOL(try_to_release_page);
* point. Because the caller is about to free (and possibly reuse) those
* blocks on-disk.
*/
-int block_invalidatepage(struct page *page, unsigned long offset)
+void block_invalidatepage(struct page *page, unsigned long offset)
{
struct buffer_head *head, *bh, *next;
unsigned int curr_off = 0;
- int ret = 1;
BUG_ON(!PageLocked(page));
if (!page_has_buffers(page))
@@ -1633,20 +1632,21 @@ int block_invalidatepage(struct page *pa
* The get_block cached value has been unconditionally invalidated,
* so real IO is not possible anymore.
*/
- if (offset == 0)
- ret = try_to_release_page(page, 0);
+ if (offset == 0) {
+ int ret = try_to_release_page(page, 0);
+ BUG_ON(!ret);
+ }
out:
- return ret;
+ return;
}
EXPORT_SYMBOL(block_invalidatepage);
-int do_invalidatepage(struct page *page, unsigned long offset)
+void do_invalidatepage(struct page *page, unsigned long offset)
{
- int (*invalidatepage)(struct page *, unsigned long);
- invalidatepage = page->mapping->a_ops->invalidatepage;
- if (invalidatepage == NULL)
- invalidatepage = block_invalidatepage;
- return (*invalidatepage)(page, offset);
+ void (*invalidatepage)(struct page *, unsigned long);
+ invalidatepage = page->mapping->a_ops->invalidatepage ? :
+ block_invalidatepage;
+ (*invalidatepage)(page, offset);
}
/*
diff ./fs/ext3/inode.c~current~ ./fs/ext3/inode.c
--- ./fs/ext3/inode.c~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/ext3/inode.c 2006-03-13 10:46:55.000000000 +1100
@@ -1583,7 +1583,7 @@ ext3_readpages(struct file *file, struct
return mpage_readpages(mapping, pages, nr_pages, ext3_get_block);
}
-static int ext3_invalidatepage(struct page *page, unsigned long offset)
+static void ext3_invalidatepage(struct page *page, unsigned long offset)
{
journal_t *journal = EXT3_JOURNAL(page->mapping->host);
@@ -1593,7 +1593,7 @@ static int ext3_invalidatepage(struct pa
if (offset == 0)
ClearPageChecked(page);
- return journal_invalidatepage(journal, page, offset);
+ journal_invalidatepage(journal, page, offset);
}
static int ext3_releasepage(struct page *page, gfp_t wait)
diff ./fs/jbd/transaction.c~current~ ./fs/jbd/transaction.c
--- ./fs/jbd/transaction.c~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/jbd/transaction.c 2006-03-13 10:46:55.000000000 +1100
@@ -1873,16 +1873,15 @@ zap_buffer_unlocked:
}
/**
- * int journal_invalidatepage()
+ * void journal_invalidatepage()
* @journal: journal to use for flush...
* @page: page to flush
* @offset: length of page to invalidate.
*
* Reap page buffers containing data after offset in page.
*
- * Return non-zero if the page's buffers were successfully reaped.
*/
-int journal_invalidatepage(journal_t *journal,
+void journal_invalidatepage(journal_t *journal,
struct page *page,
unsigned long offset)
{
@@ -1893,7 +1892,7 @@ int journal_invalidatepage(journal_t *jo
if (!PageLocked(page))
BUG();
if (!page_has_buffers(page))
- return 1;
+ return;
/* We will potentially be playing with lists other than just the
* data lists (especially for journaled data mode), so be
@@ -1916,11 +1915,10 @@ int journal_invalidatepage(journal_t *jo
} while (bh != head);
if (!offset) {
- if (!may_free || !try_to_free_buffers(page))
- return 0;
+ /* Maybe should BUG_ON !may_free - neilb */
+ try_to_free_buffers(page);
J_ASSERT(!page_has_buffers(page));
}
- return 1;
}
/*
diff ./fs/jfs/jfs_metapage.c~current~ ./fs/jfs/jfs_metapage.c
--- ./fs/jfs/jfs_metapage.c~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/jfs/jfs_metapage.c 2006-03-13 10:46:55.000000000 +1100
@@ -578,14 +578,13 @@ static int metapage_releasepage(struct p
return 0;
}
-static int metapage_invalidatepage(struct page *page, unsigned long offset)
+static void metapage_invalidatepage(struct page *page, unsigned long offset)
{
BUG_ON(offset);
- if (PageWriteback(page))
- return 0;
+ BUG_ON(PageWriteback(page));
- return metapage_releasepage(page, 0);
+ metapage_releasepage(page, 0);
}
struct address_space_operations jfs_metapage_aops = {
diff ./fs/reiser4/as_ops.c~current~ ./fs/reiser4/as_ops.c
--- ./fs/reiser4/as_ops.c~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/reiser4/as_ops.c 2006-03-13 10:46:55.000000000 +1100
@@ -170,7 +170,7 @@ reiser4_readpages(struct file *file, str
* @offset: starting offset for partial invalidation
*
*/
-int reiser4_invalidatepage(struct page *page, unsigned long offset)
+void reiser4_invalidatepage(struct page *page, unsigned long offset)
{
int ret = 0;
reiser4_context *ctx;
@@ -208,11 +208,11 @@ int reiser4_invalidatepage(struct page *
* them. Check for this, and do nothing.
*/
if (get_super_fake(inode->i_sb) == inode)
- return 0;
+ return;
if (get_cc_fake(inode->i_sb) == inode)
- return 0;
+ return;
if (get_bitmap_fake(inode->i_sb) == inode)
- return 0;
+ return;
assert("vs-1426", PagePrivate(page));
assert("vs-1427",
page->mapping == jnode_get_mapping(jnode_by_page(page)));
@@ -222,7 +222,7 @@ int reiser4_invalidatepage(struct page *
ctx = init_context(inode->i_sb);
if (IS_ERR(ctx))
- return PTR_ERR(ctx);
+ return;
node = jprivate(page);
spin_lock_jnode(node);
@@ -236,7 +236,7 @@ int reiser4_invalidatepage(struct page *
unhash_unformatted_jnode(node);
jput(node);
reiser4_exit_context(ctx);
- return 0;
+ return;
}
spin_unlock_jnode(node);
@@ -265,7 +265,6 @@ int reiser4_invalidatepage(struct page *
}
reiser4_exit_context(ctx);
- return ret;
}
/* help function called from reiser4_releasepage(). It returns true if jnode
diff ./fs/reiser4/vfs_ops.h~current~ ./fs/reiser4/vfs_ops.h
--- ./fs/reiser4/vfs_ops.h~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/reiser4/vfs_ops.h 2006-03-13 10:46:55.000000000 +1100
@@ -24,7 +24,7 @@ int reiser4_writepage(struct page *, str
int reiser4_set_page_dirty(struct page *);
int reiser4_readpages(struct file *, struct address_space *,
struct list_head *pages, unsigned nr_pages);
-int reiser4_invalidatepage(struct page *, unsigned long offset);
+void reiser4_invalidatepage(struct page *, unsigned long offset);
int reiser4_releasepage(struct page *, gfp_t);
extern int reiser4_update_sd(struct inode *);
diff ./fs/reiserfs/inode.c~current~ ./fs/reiserfs/inode.c
--- ./fs/reiserfs/inode.c~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/reiserfs/inode.c 2006-03-13 10:46:55.000000000 +1100
@@ -2792,7 +2792,7 @@ static int invalidatepage_can_drop(struc
}
/* clm -- taken from fs/buffer.c:block_invalidate_page */
-static int reiserfs_invalidatepage(struct page *page, unsigned long offset)
+static void reiserfs_invalidatepage(struct page *page, unsigned long offset)
{
struct buffer_head *head, *bh, *next;
struct inode *inode = page->mapping->host;
@@ -2831,10 +2831,12 @@ static int reiserfs_invalidatepage(struc
* The get_block cached value has been unconditionally invalidated,
* so real IO is not possible anymore.
*/
- if (!offset && ret)
+ if (!offset && ret) {
ret = try_to_release_page(page, 0);
+ /* maybe should BUG_ON(!ret); - neilb */
+ }
out:
- return ret;
+ return;
}
static int reiserfs_set_page_dirty(struct page *page)
diff ./fs/xfs/linux-2.6/xfs_aops.c~current~ ./fs/xfs/linux-2.6/xfs_aops.c
--- ./fs/xfs/linux-2.6/xfs_aops.c~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./fs/xfs/linux-2.6/xfs_aops.c 2006-03-13 10:46:55.000000000 +1100
@@ -1370,14 +1370,14 @@ out_unlock:
return error;
}
-STATIC int
+STATIC void
linvfs_invalidate_page(
struct page *page,
unsigned long offset)
{
xfs_page_trace(XFS_INVALIDPAGE_ENTER,
page->mapping->host, page, offset);
- return block_invalidatepage(page, offset);
+ block_invalidatepage(page, offset);
}
/*
diff ./include/linux/buffer_head.h~current~ ./include/linux/buffer_head.h
--- ./include/linux/buffer_head.h~current~ 2006-03-13 10:46:35.000000000 +1100
+++ ./include/linux/buffer_head.h 2006-03-13 10:46:55.000000000 +1100
@@ -192,8 +192,8 @@ extern int buffer_heads_over_limit;
* address_spaces.
*/
int try_to_release_page(struct page * page, gfp_t gfp_mask);
-int block_invalidatepage(struct page *page, unsigned long offset);
-int do_invalidatepage(struct page *page, unsigned long offset);
+void block_invalidatepage(struct page *page, unsigned long offset);
+void do_invalidatepage(struct page *page, unsigned long offset);
int block_write_full_page(struct page *page, get_block_t *get_block,
struct writeback_control *wbc);
int block_read_full_page(struct page*, get_block_t*);
diff ./include/linux/fs.h~current~ ./include/linux/fs.h
--- ./include/linux/fs.h~current~ 2006-03-13 10:46:35.000000000 +1100
+++ ./include/linux/fs.h 2006-03-13 10:46:55.000000000 +1100
@@ -364,7 +364,7 @@ struct address_space_operations {
int (*commit_write)(struct file *, struct page *, unsigned, unsigned);
/* Unfortunately this kludge is needed for FIBMAP. Don't use it */
sector_t (*bmap)(struct address_space *, sector_t);
- int (*invalidatepage) (struct page *, unsigned long);
+ void (*invalidatepage) (struct page *, unsigned long);
int (*releasepage) (struct page *, gfp_t);
ssize_t (*direct_IO)(int, struct kiocb *, const struct iovec *iov,
loff_t offset, unsigned long nr_segs);
diff ./include/linux/jbd.h~current~ ./include/linux/jbd.h
--- ./include/linux/jbd.h~current~ 2006-03-09 17:29:35.000000000 +1100
+++ ./include/linux/jbd.h 2006-03-13 10:46:55.000000000 +1100
@@ -895,7 +895,7 @@ extern int journal_dirty_metadata (hand
extern void journal_release_buffer (handle_t *, struct buffer_head *);
extern int journal_forget (handle_t *, struct buffer_head *);
extern void journal_sync_buffer (struct buffer_head *);
-extern int journal_invalidatepage(journal_t *,
+extern void journal_invalidatepage(journal_t *,
struct page *, unsigned long);
extern int journal_try_to_free_buffers(journal_t *, struct page *, gfp_t);
extern int journal_stop(handle_t *);
On Mon, 2006-03-13 at 10:53 +1100, NeilBrown wrote:
> diff ./fs/jfs/jfs_metapage.c~current~ ./fs/jfs/jfs_metapage.c
> --- ./fs/jfs/jfs_metapage.c~current~ 2006-03-09 17:29:35.000000000
> +1100
> +++ ./fs/jfs/jfs_metapage.c 2006-03-13 10:46:55.000000000 +1100
> @@ -578,14 +578,13 @@ static int metapage_releasepage(struct p
> return 0;
> }
>
> -static int metapage_invalidatepage(struct page *page, unsigned long
> offset)
> +static void metapage_invalidatepage(struct page *page, unsigned long
> offset)
> {
> BUG_ON(offset);
>
> - if (PageWriteback(page))
> - return 0;
> + BUG_ON(PageWriteback(page));
I'm a little concerned about adding a BUG_ON for something this function
used to allow, but it looks like the BUG_ON is valid. I'm asking myself
why did I add the test for PageWriteback in the first place.
>
> - return metapage_releasepage(page, 0);
> + metapage_releasepage(page, 0);
> }
>
> struct address_space_operations jfs_metapage_aops = {
I'll try to stress test jfs with these patches to see if I can trigger
the an oops here.
--
David Kleikamp
IBM Linux Technology Center
On Mon, 2006-03-13 at 10:32 -0600, Dave Kleikamp wrote:
> I'll try to stress test jfs with these patches to see if I can trigger
> the an oops here.
While stress testing on a jfs volume (dbench), I hit an assert in jbd:
Assertion failure in journal_invalidatepage() at fs/jbd/transaction.c:1920: "!page_has_buffers(page)"
------------[ cut here ]------------
kernel BUG at fs/jbd/transaction.c:1920!
invalid opcode: 0000 [#1]
PREEMPT
last sysfs file: /block/hda/hda11/stat
Modules linked in: radeon irda crc_ccitt airo e1000 pcmcia yenta_socket rsrc_nonstatic pcmcia_core ntfs jfs rtc
CPU: 0
EIP: 0060:[<c01c2782>] Not tainted VLI
EFLAGS: 00010282 (2.6.16-rc5-mm3-nb #1)
EIP is at journal_invalidatepage+0xba/0xcb
eax: 00000069 ebx: db6e67f0 ecx: d5806dc8 edx: c0452aa3
esi: c1106620 edi: db6e67f0 ebp: db6e67f0 esp: d5806dc4
ds: 007b es: 007b ss: 0068
Process evolution (pid: 11320, threadinfo=d5806000 task=d5731570)
Stack: <0>c0452aa3 c041bc55 c0454d69 00000780 c04551ac 00001000 c1106620 00000000
c1106620 00000000 c01b4d10 df30b800 c1106620 00000000 c0141369 c1106620
00000000 00000001 c01414fb dbdca3e8 c1106620 00000000 ffffffff 00000000
Call Trace:
<c01b4d10> ext3_invalidatepage+0x2f/0x33 <c0141369> truncate_complete_page+0x1d/0x42
<c01414fb> truncate_inode_pages_range+0xbe/0x27a <c017977c> inotify_inode_is_dead+0x14/0x6e
<c0413061> __mutex_lock_slowpath+0x25e/0x375 <c04130cd> __mutex_lock_slowpath+0x2ca/0x375
<c017977c> inotify_inode_is_dead+0x14/0x6e <c01b34d2> ext3_delete_inode+0x0/0xc9
<c01416cc> truncate_inode_pages+0x15/0x19 <c01b34e8> ext3_delete_inode+0x16/0xc9
<c01b34d2> ext3_delete_inode+0x0/0xc9 <c016cd2d> generic_delete_inode+0x89/0x10e
<c016a193> dput+0x19e/0x1b7 <c0164a28> do_rename+0x135/0x16c
<c0144d2a> unmap_vmas+0xc7/0x19e <c014452b> free_pgtables+0x5b/0x65
<c024c19b> strncpy_from_user+0x35/0x55 <c016147a> do_getname+0x3e/0x5c
<c0164a9a> sys_renameat+0x3b/0x60 <c0164ad0> sys_rename+0x11/0x15
<c0102c3b> syscall_call+0x7/0xb
Code: e8 d7 6c f9 ff 58 8b 06 f6 c4 08 74 29 68 ac 51 45 c0 68 80 07 00 00 68 69 4d 45 c0 68 55 bc 41 c0 68 a3 2a 45 c0 e8 6a 83 f5 ff <0f> 0b 80 07 69 4d 45 c0 83 c4 14 58 5b 5e 5f 5d c3 55 31 ed 57
BUG: evolution/11320, lock held at task exit time!
[dae25194] {inode_init_once}
.. held by: evolution:11320 [d5731570, 115]
... acquired at: lock_rename+0x8e/0x94
--
David Kleikamp
IBM Linux Technology Center
Dave Kleikamp <[email protected]> wrote:
>
> On Mon, 2006-03-13 at 10:32 -0600, Dave Kleikamp wrote:
> > I'll try to stress test jfs with these patches to see if I can trigger
> > the an oops here.
>
> While stress testing on a jfs volume (dbench), I hit an assert in jbd:
>
> Assertion failure in journal_invalidatepage() at fs/jbd/transaction.c:1920: "!page_has_buffers(page)"
Yes, thanks, that assertion has become wrong.
--- devel/fs/jbd/transaction.c~make-address_space_operations-invalidatepage-return-void-jbd-fix 2006-03-13 13:33:12.000000000 -0800
+++ devel-akpm/fs/jbd/transaction.c 2006-03-13 13:33:12.000000000 -0800
@@ -1915,9 +1915,8 @@ void journal_invalidatepage(journal_t *j
} while (bh != head);
if (!offset) {
- /* Maybe should BUG_ON !may_free - neilb */
- try_to_free_buffers(page);
- J_ASSERT(!page_has_buffers(page));
+ if (may_free && try_to_free_buffers(page))
+ J_ASSERT(!page_has_buffers(page));
}
}
However I'm more inclined to drop the whole patch, really - having
->invalidatepage() return a success indication makes sense. The fact that
we're currently not using that return value doesn't mean that we shouldn't,
didn't and won't.
On Monday March 13, [email protected] wrote:
> Dave Kleikamp <[email protected]> wrote:
> >
> > On Mon, 2006-03-13 at 10:32 -0600, Dave Kleikamp wrote:
> > > I'll try to stress test jfs with these patches to see if I can trigger
> > > the an oops here.
> >
> > While stress testing on a jfs volume (dbench), I hit an assert in jbd:
> >
> > Assertion failure in journal_invalidatepage() at fs/jbd/transaction.c:1920: "!page_has_buffers(page)"
>
> Yes, thanks, that assertion has become wrong.
>
> --- devel/fs/jbd/transaction.c~make-address_space_operations-invalidatepage-return-void-jbd-fix 2006-03-13 13:33:12.000000000 -0800
> +++ devel-akpm/fs/jbd/transaction.c 2006-03-13 13:33:12.000000000 -0800
> @@ -1915,9 +1915,8 @@ void journal_invalidatepage(journal_t *j
> } while (bh != head);
>
> if (!offset) {
> - /* Maybe should BUG_ON !may_free - neilb */
> - try_to_free_buffers(page);
> - J_ASSERT(!page_has_buffers(page));
> + if (may_free && try_to_free_buffers(page))
> + J_ASSERT(!page_has_buffers(page));
> }
> }
>
>
> However I'm more inclined to drop the whole patch, really - having
> ->invalidatepage() return a success indication makes sense. The fact that
> we're currently not using that return value doesn't mean that we shouldn't,
> didn't and won't.
->invalidatepage is called either when truncating a file or when
purging the mapping of a file from the page cache.
The VM has waited for read or write back to complete and has ensured
that no new io will happen on the page. The page, at least from
'offset' onwards, is idle.
Immediately after ->invalidatepage with offset==0 completes, the page
is removed from the pagecache. It's a goner!
So I really don't think there is any sense for invalidation to be able
to fail. The VM is saying "this page (or part of it) is going way.
Now." The filesystem really has to let go.
I'm a little bit worried by this behaviour of journal_invalidatepage
in that it doesn't always free the buffers...
I guess it hasn't finished committing a write when a truncate that
discards that data comes along. So it needs to hold on to the page to
write out those data blocks before forgetting about them.
But these machinations are completely "under-the-hood". The VM really
doesn't need to know that ext3 is holding on to the page a bit longer.
A return value to tell the VM still doesn't make sense.
Note that ->releasepage does a similar thing but does have a return
value and here it is very meaningful. Here the VM is say "I'd like
this page if you've finished with it". It isn't that the file is
being truncated. It is just a memory-reclamation action. So a return
value makes sense.
Finally, having a return value leads developers to think they need to
return a value, which gives a wrong impression of what
->invalidatepage is for.
However, if you still don't like it.....
NeilBrown
On Monday March 13, [email protected] wrote:
> On Mon, 2006-03-13 at 10:53 +1100, NeilBrown wrote:
> > diff ./fs/jfs/jfs_metapage.c~current~ ./fs/jfs/jfs_metapage.c
> > --- ./fs/jfs/jfs_metapage.c~current~ 2006-03-09 17:29:35.000000000
> > +1100
> > +++ ./fs/jfs/jfs_metapage.c 2006-03-13 10:46:55.000000000 +1100
> > @@ -578,14 +578,13 @@ static int metapage_releasepage(struct p
> > return 0;
> > }
> >
> > -static int metapage_invalidatepage(struct page *page, unsigned long
> > offset)
> > +static void metapage_invalidatepage(struct page *page, unsigned long
> > offset)
> > {
> > BUG_ON(offset);
> >
> > - if (PageWriteback(page))
> > - return 0;
> > + BUG_ON(PageWriteback(page));
>
> I'm a little concerned about adding a BUG_ON for something this function
> used to allow, but it looks like the BUG_ON is valid. I'm asking myself
> why did I add the test for PageWriteback in the first place.
Yes.... as far as I can tell, ->invalidatepage is only called with
the page locked, and with Writeback clear, so PageWriteback can never
be true. So the BUG_ON should be a no-op.
>
> >
> > - return metapage_releasepage(page, 0);
> > + metapage_releasepage(page, 0);
> > }
> >
> > struct address_space_operations jfs_metapage_aops = {
>
> I'll try to stress test jfs with these patches to see if I can trigger
> the an oops here.
Thanks. I'd be very interested if you do.
I got on oops with a similar bug_on in the new nfs_invalidatepage and
it turned out to be a bug in radixtree (which I had already found and
fixed, but not in that source tree).
NeilBrown
On Tue, 2006-03-14 at 10:10 +1100, Neil Brown wrote:
> On Monday March 13, [email protected] wrote:
> > I'm a little concerned about adding a BUG_ON for something this function
> > used to allow, but it looks like the BUG_ON is valid. I'm asking myself
> > why did I add the test for PageWriteback in the first place.
>
> Yes.... as far as I can tell, ->invalidatepage is only called with
> the page locked, and with Writeback clear, so PageWriteback can never
> be true. So the BUG_ON should be a no-op.
Either I was being overly paranoid when I put in that test, or some code
previously called invalidatepage with Writeback set and it's since been
fixed.
> > I'll try to stress test jfs with these patches to see if I can trigger
> > the an oops here.
>
> Thanks. I'd be very interested if you do.
> I got on oops with a similar bug_on in the new nfs_invalidatepage and
> it turned out to be a bug in radixtree (which I had already found and
> fixed, but not in that source tree).
Aside from the jbd assert that Andrew fixed, my stress testing of jfs
was successful. I ACK the jfs-specific part of the patch.
--
David Kleikamp
IBM Linux Technology Center