[PATCH 0/2] Cleanup invalidate page

[PATCH 0/2] Cleanup invalidate page

[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

[PATCH 1/2] ceph: Remove bogus check in invalidatepage

[Milosz Tanski]

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 <milosz@adfin.com>
---
 fs/ceph/addr.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index afb2fc2..f1d6c60 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -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;
 
 	/*
-- 
1.8.1.2

[PATCH 2/2] ceph: cleanup the logic in ceph_invalidatepage

[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 <milosz@adfin.com>
---
 fs/ceph/addr.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index f1d6c60..341f998 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -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 */
-- 
1.8.1.2

Re: [PATCH 0/2] Cleanup invalidate page

[Sage Weil]

Hi Milosz,

I pulled both these into the testing branch.  Thanks!

sage

On Fri, 9 Aug 2013, Milosz Tanski wrote:

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 0/2] Cleanup invalidate page

[Milosz Tanski]

Sage,

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

- Milosz

On Fri, Aug 9, 2013 at 1:44 PM, Sage Weil <sage@inktank.com> wrote:
> Hi Milosz,
>
> I pulled both these into the testing branch.  Thanks!
>
> sage
>
> On Fri, 9 Aug 2013, Milosz Tanski wrote:
>
>> 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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>

Re: [PATCH 0/2] Cleanup invalidate page

[Sage Weil]

On Fri, 9 Aug 2013, Milosz Tanski wrote:
> 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 ceph-qa@ceph.com 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

> 
> - Milosz
> 
> On Fri, Aug 9, 2013 at 1:44 PM, Sage Weil <sage@inktank.com> wrote:
> > Hi Milosz,
> >
> > I pulled both these into the testing branch.  Thanks!
> >
> > sage
> >
> > On Fri, 9 Aug 2013, Milosz Tanski wrote:
> >
> >> 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 majordomo@vger.kernel.org
> >> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 0/2] Cleanup invalidate page

[Milosz Tanski]

On Fri, Aug 9, 2013 at 3:44 PM, Sage Weil <sage@inktank.com> wrote:
> On Fri, 9 Aug 2013, Milosz Tanski wrote:
>> 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 ceph-qa@ceph.com 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.

>
> 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

>
> Thanks!
> sage
>
>>
>> - Milosz
>>
>> On Fri, Aug 9, 2013 at 1:44 PM, Sage Weil <sage@inktank.com> wrote:
>> > Hi Milosz,
>> >
>> > I pulled both these into the testing branch.  Thanks!
>> >
>> > sage
>> >
>> > On Fri, 9 Aug 2013, Milosz Tanski wrote:
>> >
>> >> 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 majordomo@vger.kernel.org
>> >> 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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>