Re: [PATCH] Btrfs: fix wrong argument for btrfs_lookup_ordered_range

[Caitlyn Mason]

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: caitm...@fb.com
 

 
 facebook.com/careers
 

On 1/26/17, 9:28 AM, "David Sterba"  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 
> > > ---
> > > 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,
> > >_state, GFP_NOFS);
> > > 
> > 
> > Thanks for fixing this,
> > Reviewed-by: Chandan Rajendra 
> > 
> > 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.

Re: [PATCH] Btrfs: fix wrong argument for btrfs_lookup_ordered_range

[David Sterba]

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 
> > > ---
> > > 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,
> > >_state, GFP_NOFS);
> > > 
> > 
> > Thanks for fixing this,
> > Reviewed-by: Chandan Rajendra 
> > 
> > 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix wrong argument for btrfs_lookup_ordered_range

[Liu Bo]

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 
> > ---
> > 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,
> >  _state, GFP_NOFS);
> > 
> 
> Thanks for fixing this,
> Reviewed-by: Chandan Rajendra 
> 
> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix wrong argument for btrfs_lookup_ordered_range

[Chandan Rajendra]

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 
> ---
> 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,
>_state, GFP_NOFS);
> 

Thanks for fixing this,
Reviewed-by: Chandan Rajendra 

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.

-- 
chandan

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

[PATCH] Btrfs: fix wrong argument for btrfs_lookup_ordered_range

[Liu Bo]

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 
---
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,
 _state, GFP_NOFS);
-- 
2.5.5

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