diff mbox

Btrfs: fix wrong argument for btrfs_lookup_ordered_range

Message ID 1485302331-20167-1-git-send-email-bo.li.liu@oracle.com (mailing list archive)
State Accepted
Headers show

Commit Message

Liu Bo Jan. 24, 2017, 11:58 p.m. UTC 
Commit "d0b7da88 Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units"
did this, but btrfs_lookup_ordered_range expects a 'length' rather than a
'page_end'.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
Is this a candidate for stable?

 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Chandan Rajendra Jan. 25, 2017, 8:19 a.m. UTC | #1 
On Tuesday, January 24, 2017 03:58:51 PM Liu Bo wrote:
> Commit "d0b7da88 Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units"
> did this, but btrfs_lookup_ordered_range expects a 'length' rather than a
> 'page_end'.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> Is this a candidate for stable?
> 
>  fs/btrfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4e02426..366cf0b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9023,7 +9023,7 @@ int btrfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	 * we can't set the delalloc bits if there are pending ordered
>  	 * extents.  Drop our locks and wait for them to finish
>  	 */
> -	ordered = btrfs_lookup_ordered_range(inode, page_start, page_end);
> +	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_SIZE);
>  	if (ordered) {
>  		unlock_extent_cached(io_tree, page_start, page_end,
>  				     &cached_state, GFP_NOFS);
> 

Thanks for fixing this,
Reviewed-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

As for the question about whether this commit should be merged into the stable
trees ... I am not sure about that since I don't notice any sort of filesystem
corruption that can be caused by the current code i.e. With the existing code,
apart from any ordered extents that map the page in question, we are most
likely to be *unnecessarily* starting i/o on ordered extents that don't map
the file offset range covered by the page. Chris, Josef or David, Please let
us know your thoughts on this.
Liu Bo Jan. 25, 2017, 3:06 p.m. UTC | #2 
On Wed, Jan 25, 2017 at 01:49:09PM +0530, Chandan Rajendra wrote:
> On Tuesday, January 24, 2017 03:58:51 PM Liu Bo wrote:
> > Commit "d0b7da88 Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units"
> > did this, but btrfs_lookup_ordered_range expects a 'length' rather than a
> > 'page_end'.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> > Is this a candidate for stable?
> > 
> >  fs/btrfs/inode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 4e02426..366cf0b 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -9023,7 +9023,7 @@ int btrfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> >  	 * we can't set the delalloc bits if there are pending ordered
> >  	 * extents.  Drop our locks and wait for them to finish
> >  	 */
> > -	ordered = btrfs_lookup_ordered_range(inode, page_start, page_end);
> > +	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_SIZE);
> >  	if (ordered) {
> >  		unlock_extent_cached(io_tree, page_start, page_end,
> >  				     &cached_state, GFP_NOFS);
> > 
> 
> Thanks for fixing this,
> Reviewed-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> 
> As for the question about whether this commit should be merged into the stable
> trees ... I am not sure about that since I don't notice any sort of filesystem
> corruption that can be caused by the current code i.e. With the existing code,
> apart from any ordered extents that map the page in question, we are most
> likely to be *unnecessarily* starting i/o on ordered extents that don't map
> the file offset range covered by the page. Chris, Josef or David, Please let
> us know your thoughts on this.

It could be a performance regression which causes fault writes have
unnecessary waits instead of a real corruption.

Thanks,

-liubo
--
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
David Sterba Jan. 26, 2017, 5:28 p.m. UTC | #3 
On Wed, Jan 25, 2017 at 07:06:18AM -0800, Liu Bo wrote:
> On Wed, Jan 25, 2017 at 01:49:09PM +0530, Chandan Rajendra wrote:
> > On Tuesday, January 24, 2017 03:58:51 PM Liu Bo wrote:
> > > Commit "d0b7da88 Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units"
> > > did this, but btrfs_lookup_ordered_range expects a 'length' rather than a
> > > 'page_end'.
> > > 
> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > > ---
> > > Is this a candidate for stable?
> > > 
> > >  fs/btrfs/inode.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index 4e02426..366cf0b 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -9023,7 +9023,7 @@ int btrfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> > >  	 * we can't set the delalloc bits if there are pending ordered
> > >  	 * extents.  Drop our locks and wait for them to finish
> > >  	 */
> > > -	ordered = btrfs_lookup_ordered_range(inode, page_start, page_end);
> > > +	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_SIZE);
> > >  	if (ordered) {
> > >  		unlock_extent_cached(io_tree, page_start, page_end,
> > >  				     &cached_state, GFP_NOFS);
> > > 
> > 
> > Thanks for fixing this,
> > Reviewed-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > 
> > As for the question about whether this commit should be merged into the stable
> > trees ... I am not sure about that since I don't notice any sort of filesystem
> > corruption that can be caused by the current code i.e. With the existing code,
> > apart from any ordered extents that map the page in question, we are most
> > likely to be *unnecessarily* starting i/o on ordered extents that don't map
> > the file offset range covered by the page. Chris, Josef or David, Please let
> > us know your thoughts on this.
> 
> It could be a performance regression which causes fault writes have
> unnecessary waits instead of a real corruption.

Does not seem to be urgent for stable, but I'll consider it next time
doing a stable round updates.
--
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
Caitlyn Mason Jan. 26, 2017, 5:49 p.m. UTC | #4 
I don’t think I’m supposed to be on this thread – please move me to bcc! ☺ 

-- 
caitlyn mason
Facebook | University Programs
M: (508) 963-6209
E: caitmase@fb.com
 

 
 facebook.com/careers
 

On 1/26/17, 9:28 AM, "David Sterba" <dsterba@suse.cz> wrote:

    On Wed, Jan 25, 2017 at 07:06:18AM -0800, Liu Bo wrote:
    > On Wed, Jan 25, 2017 at 01:49:09PM +0530, Chandan Rajendra wrote:

    > > On Tuesday, January 24, 2017 03:58:51 PM Liu Bo wrote:

    > > > Commit "d0b7da88 Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units"

    > > > did this, but btrfs_lookup_ordered_range expects a 'length' rather than a

    > > > 'page_end'.

    > > > 

    > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

    > > > ---

    > > > Is this a candidate for stable?

    > > > 

    > > >  fs/btrfs/inode.c | 2 +-

    > > >  1 file changed, 1 insertion(+), 1 deletion(-)

    > > > 

    > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c

    > > > index 4e02426..366cf0b 100644

    > > > --- a/fs/btrfs/inode.c

    > > > +++ b/fs/btrfs/inode.c

    > > > @@ -9023,7 +9023,7 @@ int btrfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)

    > > >  	 * we can't set the delalloc bits if there are pending ordered

    > > >  	 * extents.  Drop our locks and wait for them to finish

    > > >  	 */

    > > > -	ordered = btrfs_lookup_ordered_range(inode, page_start, page_end);

    > > > +	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_SIZE);

    > > >  	if (ordered) {

    > > >  		unlock_extent_cached(io_tree, page_start, page_end,

    > > >  				     &cached_state, GFP_NOFS);

    > > > 

    > > 

    > > Thanks for fixing this,

    > > Reviewed-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

    > > 

    > > As for the question about whether this commit should be merged into the stable

    > > trees ... I am not sure about that since I don't notice any sort of filesystem

    > > corruption that can be caused by the current code i.e. With the existing code,

    > > apart from any ordered extents that map the page in question, we are most

    > > likely to be *unnecessarily* starting i/o on ordered extents that don't map

    > > the file offset range covered by the page. Chris, Josef or David, Please let

    > > us know your thoughts on this.

    > 

    > It could be a performance regression which causes fault writes have

    > unnecessary waits instead of a real corruption.

    
    Does not seem to be urgent for stable, but I'll consider it next time
    doing a stable round updates.
diff mbox

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4e02426..366cf0b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9023,7 +9023,7 @@  int btrfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	 * we can't set the delalloc bits if there are pending ordered
 	 * extents.  Drop our locks and wait for them to finish
 	 */
-	ordered = btrfs_lookup_ordered_range(inode, page_start, page_end);
+	ordered = btrfs_lookup_ordered_range(inode, page_start, PAGE_SIZE);
 	if (ordered) {
 		unlock_extent_cached(io_tree, page_start, page_end,
 				     &cached_state, GFP_NOFS);