Cleanup invalidate page

PATCH In review Milosz Tanski 2 participants 7 messages Aug 9, 2013

Subsystems (2)

VFS Ceph client

lore

Share & tools

lore

Milosz Tanski

author

2013-08-09 16:59 UTC #0 lore raw

cover Cleanup invalidate page

Currently ceph_invalidatepage has is overly eger with it's checks which are moot. The second change cleans up the case where offset is non zero.

Please pull the from:
https://bitbucket.org/adfin/linux-fs.git wip-invalidatepage

This simple patchset came from the changes I made while working on fscache support for cephfs. Per Sage's request I split this up into smaller bites for testing and review.

Milosz Tanski (2):
ceph: Remove bogus check in invalidatepage
ceph: cleanup the logic in ceph_invalidatepage

 fs/ceph/addr.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

Signature · 1 line

1.8.1.2

Milosz Tanski

author

2013-08-09 16:59 UTC #1 lore raw

PATCH 1/2 ceph: Remove bogus check in invalidatepage

The early bug checks are moot because the VMA layer ensures those things.

1. It will not call invalidatepage unless PagePrivate (or PagePrivate2) are set
2. It will not call invalidatepage without taking a PageLock first.
3. Guantrees that the inode page is mapped.

Signed-off-by Milosz Tanski <[email protected]>

---
 fs/ceph/addr.c | 4 ----
 1 file changed, 4 deletions(-)

fs/ceph/addr.c +0 -4

index afb2fc2..f1d6c60 100644
@@ -149,10 +149,6 @@ static void ceph_invalidatepage(struct page *page, unsigned long offset)
 	struct ceph_inode_info *ci;
 	struct ceph_snap_context *snapc = page_snap_context(page);
 
-	BUG_ON(!PageLocked(page));
-	BUG_ON(!PagePrivate(page));
-	BUG_ON(!page->mapping);
-
 	inode = page->mapping->host;
 
 	/*

Signature · 1 line

1.8.1.2

Milosz Tanski

author

2013-08-09 16:59 UTC #2 lore raw

PATCH 2/2 ceph: cleanup the logic in ceph_invalidatepage

In reply to Milosz Tanski

The invalidatepage code bails if it encounters a non-zero page offset. The current logic that does is non-obvious with multiple if statements.

This should be logically and functionally equivalent.

Signed-off-by Milosz Tanski <[email protected]>

---
 fs/ceph/addr.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

fs/ceph/addr.c +15 ~~-14~~

index f1d6c60..341f998 100644
@@ -150,6 +150,13 @@ static void ceph_invalidatepage(struct page *page, unsigned long offset)
 	struct ceph_snap_context *snapc = page_snap_context(page);
 
 	inode = page->mapping->host;
+	ci = ceph_inode(inode);
+
+	if (offset != 0) {
+		dout("%p invalidatepage %p idx %lu partial dirty page\n",
+		     inode, page, page->index);
+		return;
+	}
 
 	/*
 	 * We can get non-dirty pages here due to races between

@@ -159,21 +166,15 @@ static void ceph_invalidatepage(struct page *page, unsigned long offset)
 	if (!PageDirty(page))
 		pr_err("%p invalidatepage %p page not dirty\n", inode, page);
 
-	if (offset == 0)
-		ClearPageChecked(page);
+	ClearPageChecked(page);
 
-	ci = ceph_inode(inode);
-	if (offset == 0) {
-		dout("%p invalidatepage %p idx %lu full dirty page %lu\n",
-		     inode, page, page->index, offset);
-		ceph_put_wrbuffer_cap_refs(ci, 1, snapc);
-		ceph_put_snap_context(snapc);
-		page->private = 0;
-		ClearPagePrivate(page);
-	} else {
-		dout("%p invalidatepage %p idx %lu partial dirty page\n",
-		     inode, page, page->index);
-	}
+	dout("%p invalidatepage %p idx %lu full dirty page %lu\n",
+	     inode, page, page->index, offset);
+
+	ceph_put_wrbuffer_cap_refs(ci, 1, snapc);
+	ceph_put_snap_context(snapc);
+	page->private = 0;
+	ClearPagePrivate(page);
 }
 
 /* just a sanity check */

Signature · 1 line

1.8.1.2

Sage Weil

2013-08-09 17:44 UTC #3 lore raw

In reply to Milosz Tanski

Hi Milosz,

I pulled both these into the testing branch. Thanks!

sage

Quoted 26 lines · Milosz Tanski

Currently ceph_invalidatepage has is overly eger with it's checks which are moot. The second change cleans up the case where offset is non zero.
Please pull the from:
https://bitbucket.org/adfin/linux-fs.git wip-invalidatepage
This simple patchset came from the changes I made while working on fscache support for cephfs. Per Sage's request I split this up into smaller bites for testing and review.
Milosz Tanski (2):
ceph: Remove bogus check in invalidatepage
ceph: cleanup the logic in ceph_invalidatepage
 fs/ceph/addr.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
Signature · 6 lines
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Milosz Tanski

author

2013-08-09 19:43 UTC #4 lore raw

Sage,

Great. Is there some automated testing system that looks for regressions in cephfs that I can be watching for?

- Milosz

Quoted 34 lines · Sage Weil

Hi Milosz,
I pulled both these into the testing branch. Thanks!
sage
Quoted 26 lines · Milosz Tanski
Currently ceph_invalidatepage has is overly eger with it's checks which are moot. The second change cleans up the case where offset is non zero.
Please pull the from:
https://bitbucket.org/adfin/linux-fs.git wip-invalidatepage
This simple patchset came from the changes I made while working on fscache support for cephfs. Per Sage's request I split this up into smaller bites for testing and review.
Milosz Tanski (2):
ceph: Remove bogus check in invalidatepage
ceph: cleanup the logic in ceph_invalidatepage
 fs/ceph/addr.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sage Weil

2013-08-09 19:44 UTC #5 lore raw

Quoted 4 lines · Milosz Tanski

Sage,
Great. Is there some automated testing system that looks for regressions in cephfs that I can be watching for?

Yep, you can join the [email protected] email list and watch for the kcephfs suite results (see http://ceph.com/resources/mailing-list-irc/).

BTW I made a few tweaks to the second patch due to a conflict (added handling for the length arg to invalidatepage).

Thanks!
sage

Quoted 44 lines

- Milosz
Quoted 34 lines · Sage Weil
Hi Milosz,
I pulled both these into the testing branch. Thanks!
sage
Quoted 26 lines · Milosz Tanski
Currently ceph_invalidatepage has is overly eger with it's checks which are moot. The second change cleans up the case where offset is non zero.
Please pull the from:
https://bitbucket.org/adfin/linux-fs.git wip-invalidatepage
This simple patchset came from the changes I made while working on fscache support for cephfs. Per Sage's request I split this up into smaller bites for testing and review.
Milosz Tanski (2):
ceph: Remove bogus check in invalidatepage
ceph: cleanup the logic in ceph_invalidatepage
 fs/ceph/addr.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Milosz Tanski

author

2013-08-09 19:51 UTC #6 lore raw

Quoted 8 lines · Sage Weil

Quoted 4 lines · Milosz Tanski
Sage,
Great. Is there some automated testing system that looks for regressions in cephfs that I can be watching for?
Yep, you can join the [email protected] email list and watch for the kcephfs suite results (see http://ceph.com/resources/mailing-list-irc/).

I'll brace my self for the spam.

Quoted 3 lines

BTW I made a few tweaks to the second patch due to a conflict (added handling for the length arg to invalidatepage).

Thanks and sorry for the oversight.

Best,
-Milosz

Quoted 48 lines

Thanks!
sage
Quoted 44 lines
- Milosz
Quoted 34 lines · Sage Weil
Hi Milosz,
I pulled both these into the testing branch. Thanks!
sage
Quoted 26 lines · Milosz Tanski
Currently ceph_invalidatepage has is overly eger with it's checks which are moot. The second change cleans up the case where offset is non zero.
Please pull the from:
https://bitbucket.org/adfin/linux-fs.git wip-invalidatepage
This simple patchset came from the changes I made while working on fscache support for cephfs. Per Sage's request I split this up into smaller bites for testing and review.
Milosz Tanski (2):
ceph: Remove bogus check in invalidatepage
ceph: cleanup the logic in ceph_invalidatepage
 fs/ceph/addr.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html