diff mbox

[RFC,V11,12/21] Btrfs: subpagesize-blocksize: Search for all ordered extents that could span across a page.

Message ID 1433172176-8742-13-git-send-email-chandan@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chandan Rajendra June 1, 2015, 3:22 p.m. UTC 
In subpagesize-blocksize scenario it is not sufficient to search using the
first byte of the page to make sure that there are no ordered extents
present across the page. Fix this.

Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
---
 fs/btrfs/extent_io.c | 3 ++-
 fs/btrfs/inode.c     | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

Comments

Liu Bo July 1, 2015, 2:47 p.m. UTC | #1 
On Mon, Jun 01, 2015 at 08:52:47PM +0530, Chandan Rajendra wrote:
> In subpagesize-blocksize scenario it is not sufficient to search using the
> first byte of the page to make sure that there are no ordered extents
> present across the page. Fix this.
> 
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> ---
>  fs/btrfs/extent_io.c | 3 ++-
>  fs/btrfs/inode.c     | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 14b4e05..0b017e1 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3244,7 +3244,8 @@ static int __extent_read_full_page(struct extent_io_tree *tree,
>  
>  	while (1) {
>  		lock_extent(tree, start, end);
> -		ordered = btrfs_lookup_ordered_extent(inode, start);
> +		ordered = btrfs_lookup_ordered_range(inode, start,
> +						PAGE_CACHE_SIZE);

A minor suggestion, it'd be better to include the new prototype in the
same patch, which will be benefit to later cherry-picking or reverting.

Thanks,

-liubo
>  		if (!ordered)
>  			break;
>  		unlock_extent(tree, start, end);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e9bab73..8b4aaed 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1976,7 +1976,7 @@ again:
>  	if (PagePrivate2(page))
>  		goto out;
>  
> -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> +	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_CACHE_SIZE);
>  	if (ordered) {
>  		unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start,
>  				     page_end, &cached_state, GFP_NOFS);
> @@ -8513,7 +8513,7 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset,
>  
>  	if (!inode_evicting)
>  		lock_extent_bits(tree, page_start, page_end, 0, &cached_state);
> -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> +	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_CACHE_SIZE);
>  	if (ordered) {
>  		/*
>  		 * IO on this page will never be started, so we need
> -- 
> 2.1.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chandan Rajendra July 3, 2015, 10:08 a.m. UTC | #2 
On Wednesday 01 Jul 2015 22:47:10 Liu Bo wrote:
> On Mon, Jun 01, 2015 at 08:52:47PM +0530, Chandan Rajendra wrote:
> > In subpagesize-blocksize scenario it is not sufficient to search using the
> > first byte of the page to make sure that there are no ordered extents
> > present across the page. Fix this.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > ---
> > 
> >  fs/btrfs/extent_io.c | 3 ++-
> >  fs/btrfs/inode.c     | 4 ++--
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 14b4e05..0b017e1 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -3244,7 +3244,8 @@ static int __extent_read_full_page(struct
> > extent_io_tree *tree,> 
> >  	while (1) {
> >  	
> >  		lock_extent(tree, start, end);
> > 
> > -		ordered = btrfs_lookup_ordered_extent(inode, start);
> > +		ordered = btrfs_lookup_ordered_range(inode, start,
> > +						PAGE_CACHE_SIZE);
> 
> A minor suggestion, it'd be better to include the new prototype in the
> same patch, which will be benefit to later cherry-picking or reverting.
> 

Liu, The definition of btrfs_lookup_ordered_range() is already part of
the mainline kernel.

> 
> >  		if (!ordered)
> >  		
> >  			break;
> >  		
> >  		unlock_extent(tree, start, end);
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index e9bab73..8b4aaed 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > 
> > @@ -1976,7 +1976,7 @@ again:
> >  	if (PagePrivate2(page))
> >  	
> >  		goto out;
> > 
> > -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> > +	ordered = btrfs_lookup_ordered_range(inode, page_start,
> > PAGE_CACHE_SIZE);
> > 
> >  	if (ordered) {
> >  	
> >  		unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start,
> >  		
> >  				     page_end, &cached_state, GFP_NOFS);
> > 
> > @@ -8513,7 +8513,7 @@ static void btrfs_invalidatepage(struct page *page,
> > unsigned int offset,> 
> >  	if (!inode_evicting)
> >  	
> >  		lock_extent_bits(tree, page_start, page_end, 0, 
&cached_state);
> > 
> > -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> > +	ordered = btrfs_lookup_ordered_range(inode, page_start,
> > PAGE_CACHE_SIZE);
> > 
> >  	if (ordered) {
> >  	
> >  		/*
> >  		
> >  		 * IO on this page will never be started, so we need
Liu Bo July 6, 2015, 3:17 a.m. UTC | #3 
On Fri, Jul 03, 2015 at 03:38:00PM +0530, Chandan Rajendra wrote:
> On Wednesday 01 Jul 2015 22:47:10 Liu Bo wrote:
> > On Mon, Jun 01, 2015 at 08:52:47PM +0530, Chandan Rajendra wrote:
> > > In subpagesize-blocksize scenario it is not sufficient to search using the
> > > first byte of the page to make sure that there are no ordered extents
> > > present across the page. Fix this.
> > > 
> > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > > ---
> > > 
> > >  fs/btrfs/extent_io.c | 3 ++-
> > >  fs/btrfs/inode.c     | 4 ++--
> > >  2 files changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > index 14b4e05..0b017e1 100644
> > > --- a/fs/btrfs/extent_io.c
> > > +++ b/fs/btrfs/extent_io.c
> > > @@ -3244,7 +3244,8 @@ static int __extent_read_full_page(struct
> > > extent_io_tree *tree,> 
> > >  	while (1) {
> > >  	
> > >  		lock_extent(tree, start, end);
> > > 
> > > -		ordered = btrfs_lookup_ordered_extent(inode, start);
> > > +		ordered = btrfs_lookup_ordered_range(inode, start,
> > > +						PAGE_CACHE_SIZE);
> > 
> > A minor suggestion, it'd be better to include the new prototype in the
> > same patch, which will be benefit to later cherry-picking or reverting.
> > 
> 
> Liu, The definition of btrfs_lookup_ordered_range() is already part of
> the mainline kernel.

Ah, I didn't recognize the difference of btrfs_lookup_ordered_extent and
btrfs_lookup_ordered_range, sorry.

> 
> > 
> > >  		if (!ordered)
> > >  		
> > >  			break;
> > >  		
> > >  		unlock_extent(tree, start, end);
> > > 
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index e9bab73..8b4aaed 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > 
> > > @@ -1976,7 +1976,7 @@ again:
> > >  	if (PagePrivate2(page))
> > >  	
> > >  		goto out;
> > > 
> > > -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> > > +	ordered = btrfs_lookup_ordered_range(inode, page_start,
> > > PAGE_CACHE_SIZE);
> > > 
> > >  	if (ordered) {
> > >  	
> > >  		unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start,
> > >  		
> > >  				     page_end, &cached_state, GFP_NOFS);
> > > 
> > > @@ -8513,7 +8513,7 @@ static void btrfs_invalidatepage(struct page *page,
> > > unsigned int offset,> 
> > >  	if (!inode_evicting)
> > >  	
> > >  		lock_extent_bits(tree, page_start, page_end, 0, 
> &cached_state);
> > > 
> > > -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> > > +	ordered = btrfs_lookup_ordered_range(inode, page_start,
> > > PAGE_CACHE_SIZE);

It's possible for a page to hold two (or more) ordered extents here, a
while loop is necessary to ensure that every ordered extent is processed
properly.


Thanks,

-liubo

> > > 
> > >  	if (ordered) {
> > >  	
> > >  		/*
> > >  		
> > >  		 * IO on this page will never be started, so we need
> 
> -- 
> chandan
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chandan Rajendra July 6, 2015, 10:49 a.m. UTC | #4 
On Monday 06 Jul 2015 11:17:38 Liu Bo wrote:
> On Fri, Jul 03, 2015 at 03:38:00PM +0530, Chandan Rajendra wrote:
> > On Wednesday 01 Jul 2015 22:47:10 Liu Bo wrote:
> > > On Mon, Jun 01, 2015 at 08:52:47PM +0530, Chandan Rajendra wrote:
> > > > In subpagesize-blocksize scenario it is not sufficient to search using
> > > > the
> > > > first byte of the page to make sure that there are no ordered extents
> > > > present across the page. Fix this.
> > > > 
> > > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > > > ---
> > > > 
> > > >  fs/btrfs/extent_io.c | 3 ++-
> > > >  fs/btrfs/inode.c     | 4 ++--
> > > >  2 files changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > > index 14b4e05..0b017e1 100644
> > > > --- a/fs/btrfs/extent_io.c
> > > > +++ b/fs/btrfs/extent_io.c
> > > > @@ -3244,7 +3244,8 @@ static int __extent_read_full_page(struct
> > > > extent_io_tree *tree,>
> > > > 
> > > >  	while (1) {
> > > >  	
> > > >  		lock_extent(tree, start, end);
> > > > 
> > > > -		ordered = btrfs_lookup_ordered_extent(inode, start);
> > > > +		ordered = btrfs_lookup_ordered_range(inode, start,
> > > > +						PAGE_CACHE_SIZE);
> > > 
> > > A minor suggestion, it'd be better to include the new prototype in the
> > > same patch, which will be benefit to later cherry-picking or reverting.
> > 
> > Liu, The definition of btrfs_lookup_ordered_range() is already part of
> > the mainline kernel.
> 
> Ah, I didn't recognize the difference of btrfs_lookup_ordered_extent and
> btrfs_lookup_ordered_range, sorry.
> 
> > > >  		if (!ordered)
> > > >  		
> > > >  			break;
> > > >  		
> > > >  		unlock_extent(tree, start, end);
> > > > 
> > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > > index e9bab73..8b4aaed 100644
> > > > --- a/fs/btrfs/inode.c
> > > > +++ b/fs/btrfs/inode.c
> > > > 
> > > > @@ -1976,7 +1976,7 @@ again:
> > > >  	if (PagePrivate2(page))
> > > >  	
> > > >  		goto out;
> > > > 
> > > > -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> > > > +	ordered = btrfs_lookup_ordered_range(inode, page_start,
> > > > PAGE_CACHE_SIZE);
> > > > 
> > > >  	if (ordered) {
> > > >  	
> > > >  		unlock_extent_cached(&BTRFS_I(inode)->io_tree, 
page_start,
> > > >  		
> > > >  				     page_end, &cached_state, 
GFP_NOFS);
> > > > 
> > > > @@ -8513,7 +8513,7 @@ static void btrfs_invalidatepage(struct page
> > > > *page,
> > > > unsigned int offset,>
> > > > 
> > > >  	if (!inode_evicting)
> > > >  	
> > > >  		lock_extent_bits(tree, page_start, page_end, 0,
> > 
> > &cached_state);
> > 
> > > > -	ordered = btrfs_lookup_ordered_extent(inode, page_start);
> > > > +	ordered = btrfs_lookup_ordered_range(inode, page_start,
> > > > PAGE_CACHE_SIZE);
> 
> It's possible for a page to hold two (or more) ordered extents here, a
> while loop is necessary to ensure that every ordered extent is processed
> properly.
>
Liu, Sorry, I had introduced the loop in the patch
"[RFC PATCH V11 14/21] Btrfs: subpagesize-blocksize: Explicitly Track I/O
status of blocks of an ordered extent". I will pull the loop to this patch for
the next version of that patchset. 

> Thanks,
> 
> -liubo
> 
> > > >  	if (ordered) {
> > > >  	
> > > >  		/*
> > > >  		
> > > >  		 * IO on this page will never be started, so we need
diff mbox

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 14b4e05..0b017e1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3244,7 +3244,8 @@  static int __extent_read_full_page(struct extent_io_tree *tree,
 
 	while (1) {
 		lock_extent(tree, start, end);
-		ordered = btrfs_lookup_ordered_extent(inode, start);
+		ordered = btrfs_lookup_ordered_range(inode, start,
+						PAGE_CACHE_SIZE);
 		if (!ordered)
 			break;
 		unlock_extent(tree, start, end);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e9bab73..8b4aaed 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1976,7 +1976,7 @@  again:
 	if (PagePrivate2(page))
 		goto out;
 
-	ordered = btrfs_lookup_ordered_extent(inode, page_start);
+	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_CACHE_SIZE);
 	if (ordered) {
 		unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start,
 				     page_end, &cached_state, GFP_NOFS);
@@ -8513,7 +8513,7 @@  static void btrfs_invalidatepage(struct page *page, unsigned int offset,
 
 	if (!inode_evicting)
 		lock_extent_bits(tree, page_start, page_end, 0, &cached_state);
-	ordered = btrfs_lookup_ordered_extent(inode, page_start);
+	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_CACHE_SIZE);
 	if (ordered) {
 		/*
 		 * IO on this page will never be started, so we need