diff mbox

[v3,08/18] gfs2: use ->invalidatepage() length argument

Message ID 1365498867-27782-9-git-send-email-lczerner@redhat.com
State Superseded, archived
Headers show

Commit Message

Lukas Czerner April 9, 2013, 9:14 a.m. UTC 
->invalidatepage() aop now accepts range to invalidate so we can make
use of it in gfs2_invalidatepage().

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Cc: cluster-devel@redhat.com
---
 fs/gfs2/aops.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Comments

Steven Whitehouse April 9, 2013, 9:29 a.m. UTC | #1 
Hi,

On Tue, 2013-04-09 at 11:14 +0200, Lukas Czerner wrote:
> ->invalidatepage() aop now accepts range to invalidate so we can make
> use of it in gfs2_invalidatepage().
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Cc: cluster-devel@redhat.com
Acked-by: Steven Whitehouse <swhiteho@redhat.com>

Steve.

> ---
>  fs/gfs2/aops.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 37093ba..ea920bf 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -947,24 +947,29 @@ static void gfs2_invalidatepage(struct page *page, unsigned int offset,
>  				unsigned int length)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host);
> +	unsigned int stop = offset + length;
> +	int partial_page = (offset || length < PAGE_CACHE_SIZE);
>  	struct buffer_head *bh, *head;
>  	unsigned long pos = 0;
>  
>  	BUG_ON(!PageLocked(page));
> -	if (offset == 0)
> +	if (!partial_page)
>  		ClearPageChecked(page);
>  	if (!page_has_buffers(page))
>  		goto out;
>  
>  	bh = head = page_buffers(page);
>  	do {
> +		if (pos + bh->b_size > stop)
> +			return;
> +
>  		if (offset <= pos)
>  			gfs2_discard(sdp, bh);
>  		pos += bh->b_size;
>  		bh = bh->b_this_page;
>  	} while (bh != head);
>  out:
> -	if (offset == 0)
> +	if (!partial_page)
>  		try_to_release_page(page, 0);
>  }
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bob Peterson April 9, 2013, 1:09 p.m. UTC | #2 
Hi,

----- Original Message -----
| ->invalidatepage() aop now accepts range to invalidate so we can make
| use of it in gfs2_invalidatepage().
| 
| Signed-off-by: Lukas Czerner <lczerner@redhat.com>
| Cc: cluster-devel@redhat.com
| ---
|  fs/gfs2/aops.c |    9 +++++++--
|  1 files changed, 7 insertions(+), 2 deletions(-)
| 
| diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
| index 37093ba..ea920bf 100644
| --- a/fs/gfs2/aops.c
| +++ b/fs/gfs2/aops.c
| @@ -947,24 +947,29 @@ static void gfs2_invalidatepage(struct page *page,
| unsigned int offset,
|  				unsigned int length)
|  {
|  	struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host);
| +	unsigned int stop = offset + length;
| +	int partial_page = (offset || length < PAGE_CACHE_SIZE);
|  	struct buffer_head *bh, *head;
|  	unsigned long pos = 0;
|  
|  	BUG_ON(!PageLocked(page));
| -	if (offset == 0)
| +	if (!partial_page)
|  		ClearPageChecked(page);
|  	if (!page_has_buffers(page))
|  		goto out;
|  
|  	bh = head = page_buffers(page);
|  	do {
| +		if (pos + bh->b_size > stop)
| +			return;
| +

I always regret it when I review something this early in the morning,
or post before the caffeine has taken full effect. But...
Shouldn't the statement above be (1) ">= stop" and (2) goto out;?

|  		if (offset <= pos)
|  			gfs2_discard(sdp, bh);
|  		pos += bh->b_size;
|  		bh = bh->b_this_page;
|  	} while (bh != head);
|  out:
| -	if (offset == 0)
| +	if (!partial_page)
|  		try_to_release_page(page, 0);
|  }

Regards,

Bob Peterson
Red Hat File Systems
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner April 9, 2013, 1:27 p.m. UTC | #3 
On Tue, 9 Apr 2013, Bob Peterson wrote:

> Date: Tue, 9 Apr 2013 09:09:20 -0400 (EDT)
> From: Bob Peterson <rpeterso@redhat.com>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
>     linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
>     cluster-devel@redhat.com
> Subject: Re: [PATCH v3 08/18] gfs2: use ->invalidatepage() length argument
> 
> Hi,
> 
> ----- Original Message -----
> | ->invalidatepage() aop now accepts range to invalidate so we can make
> | use of it in gfs2_invalidatepage().
> | 
> | Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> | Cc: cluster-devel@redhat.com
> | ---
> |  fs/gfs2/aops.c |    9 +++++++--
> |  1 files changed, 7 insertions(+), 2 deletions(-)
> | 
> | diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> | index 37093ba..ea920bf 100644
> | --- a/fs/gfs2/aops.c
> | +++ b/fs/gfs2/aops.c
> | @@ -947,24 +947,29 @@ static void gfs2_invalidatepage(struct page *page,
> | unsigned int offset,
> |  				unsigned int length)
> |  {
> |  	struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host);
> | +	unsigned int stop = offset + length;
> | +	int partial_page = (offset || length < PAGE_CACHE_SIZE);
> |  	struct buffer_head *bh, *head;
> |  	unsigned long pos = 0;
> |  
> |  	BUG_ON(!PageLocked(page));
> | -	if (offset == 0)
> | +	if (!partial_page)
> |  		ClearPageChecked(page);
> |  	if (!page_has_buffers(page))
> |  		goto out;
> |  
> |  	bh = head = page_buffers(page);
> |  	do {
> | +		if (pos + bh->b_size > stop)
> | +			return;
> | +
> 
> I always regret it when I review something this early in the morning,
> or post before the caffeine has taken full effect. But...
> Shouldn't the statement above be (1) ">= stop" and (2) goto out;?

Hi,

1: No, because "stop = offset + length" and we're comparing it with
   "pos + bh->b_size". If it would be ">=" then we would always miss
   the last buffer. Let's assume pos = 0, b_size = 1024 and we're
   invalidating offset = 0 and length = 1024 .. the rest you can try
   for yourself :)

2: No, because if this condition is true, we can never have full page
   invalidate. With full page invalidation we'll walk all the
   buffers within the page hence we would jump our of the cycle when
   "bh == head".

Thanks!
-Lukas

> 
> |  		if (offset <= pos)
> |  			gfs2_discard(sdp, bh);
> |  		pos += bh->b_size;
> |  		bh = bh->b_this_page;
> |  	} while (bh != head);
> |  out:
> | -	if (offset == 0)
> | +	if (!partial_page)
> |  		try_to_release_page(page, 0);
> |  }
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o April 23, 2013, 2:16 p.m. UTC | #4 
On Tue, Apr 09, 2013 at 11:14:17AM +0200, Lukas Czerner wrote:
> ->invalidatepage() aop now accepts range to invalidate so we can make
> use of it in gfs2_invalidatepage().
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Cc: cluster-devel@redhat.com

To the gfs2 development team, 

Since half of this patch series modifies ext4 extensively, and changes
to the other file systems are relatively small, I plan to carry the
invalidatepage patch set in the ext4 tree for the next development
cycle (i.e., not the upcoming merge window, but the next one).  To
that end, it would be great if you take a look at this patch set and
send us an Acked-by signoff.

Thanks!!

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o April 23, 2013, 2:17 p.m. UTC | #5 
On Tue, Apr 23, 2013 at 10:16:31AM -0400, Theodore Ts'o wrote:
> On Tue, Apr 09, 2013 at 11:14:17AM +0200, Lukas Czerner wrote:
> > ->invalidatepage() aop now accepts range to invalidate so we can make
> > use of it in gfs2_invalidatepage().
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > Cc: cluster-devel@redhat.com
> 
> To the gfs2 development team, 

Whoops, I missed Steven Whitehouse's Acked-by.  Sorry for the noise,

	  	 	 	    	  - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 37093ba..ea920bf 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -947,24 +947,29 @@  static void gfs2_invalidatepage(struct page *page, unsigned int offset,
 				unsigned int length)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host);
+	unsigned int stop = offset + length;
+	int partial_page = (offset || length < PAGE_CACHE_SIZE);
 	struct buffer_head *bh, *head;
 	unsigned long pos = 0;
 
 	BUG_ON(!PageLocked(page));
-	if (offset == 0)
+	if (!partial_page)
 		ClearPageChecked(page);
 	if (!page_has_buffers(page))
 		goto out;
 
 	bh = head = page_buffers(page);
 	do {
+		if (pos + bh->b_size > stop)
+			return;
+
 		if (offset <= pos)
 			gfs2_discard(sdp, bh);
 		pos += bh->b_size;
 		bh = bh->b_this_page;
 	} while (bh != head);
 out:
-	if (offset == 0)
+	if (!partial_page)
 		try_to_release_page(page, 0);
 }