Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-28 Thread Ben Hutchings 
On Sun, 2013-07-28 at 07:40 -0400, Theodore Ts'o wrote:
> On Sat, Jul 27, 2013 at 10:33:31PM +0100, Ben Hutchings wrote:
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
> > >  
> > >   last_block = (inode->i_size + sb->s_blocksize - 1)
> > >   >> EXT4_BLOCK_SIZE_BITS(sb);
> > > +retry:
> > >   err = ext4_es_remove_extent(inode, last_block,
> > >   EXT_MAX_BLOCKS - last_block);
> > > + if (err == ENOMEM) {
> > 
> > Positive ENOMEM?!  It looks like this value is bubbled up from
> > __es_insert_extent() which returns the usual negative error codes.
> 
> Nice catch.  Yup, that's a problem.  I'll fix this upstream and mark
> it cc:stable.
> 
> Until this goes upstream stable kernel maintainers can either: (a) fix
> up this patch by making the line read "err == -ENOMEM", (b) hold back
> this patch until the companion patch to fix this goes upstream, or (c)
> apply this now, since it's otherwise harmless and it does add error
> checking to the ext4_ext_remove_space() call.

We do need that second fix upstream first.  I agree that this seems to
fix a bug and doesn't make the ENOMEM case worse.  If this is applicable
to 3.2.y (I think it isn't but haven't checked yet) then I'll go with
(c).

Ben.

-- 
Ben Hutchings
All extremists should be taken out and shot.


signature.asc
Description: This is a digitally signed message part

Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-28 Thread Greg Kroah-Hartman 
On Sun, Jul 28, 2013 at 07:40:53AM -0400, Theodore Ts'o wrote:
> On Sat, Jul 27, 2013 at 10:33:31PM +0100, Ben Hutchings wrote:
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
> > >  
> > >   last_block = (inode->i_size + sb->s_blocksize - 1)
> > >   >> EXT4_BLOCK_SIZE_BITS(sb);
> > > +retry:
> > >   err = ext4_es_remove_extent(inode, last_block,
> > >   EXT_MAX_BLOCKS - last_block);
> > > + if (err == ENOMEM) {
> > 
> > Positive ENOMEM?!  It looks like this value is bubbled up from
> > __es_insert_extent() which returns the usual negative error codes.
> 
> Nice catch.  Yup, that's a problem.  I'll fix this upstream and mark
> it cc:stable.
> 
> Until this goes upstream stable kernel maintainers can either: (a) fix
> up this patch by making the line read "err == -ENOMEM", (b) hold back
> this patch until the companion patch to fix this goes upstream, or (c)
> apply this now, since it's otherwise harmless and it does add error
> checking to the ext4_ext_remove_space() call.

I'll take it as-is and expect to see the fix come through Linus's tree
soon.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-28 Thread Theodore Ts'o 
On Sat, Jul 27, 2013 at 10:33:31PM +0100, Ben Hutchings wrote:
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
> >  
> > last_block = (inode->i_size + sb->s_blocksize - 1)
> > >> EXT4_BLOCK_SIZE_BITS(sb);
> > +retry:
> > err = ext4_es_remove_extent(inode, last_block,
> > EXT_MAX_BLOCKS - last_block);
> > +   if (err == ENOMEM) {
> 
> Positive ENOMEM?!  It looks like this value is bubbled up from
> __es_insert_extent() which returns the usual negative error codes.

Nice catch.  Yup, that's a problem.  I'll fix this upstream and mark
it cc:stable.

Until this goes upstream stable kernel maintainers can either: (a) fix
up this patch by making the line read "err == -ENOMEM", (b) hold back
this patch until the companion patch to fix this goes upstream, or (c)
apply this now, since it's otherwise harmless and it does add error
checking to the ext4_ext_remove_space() call.

> 
> Ben.
> 
> > +   cond_resched();
> > +   congestion_wait(BLK_RW_ASYNC, HZ/50);
> > +   goto retry;
> > +   }
> > +   if (err) {
> > +   ext4_std_error(inode->i_sb, err);
> > +   return;
> > +   }
> > err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
> > +   ext4_std_error(inode->i_sb, err);
> >  }
> >  
> >  static void ext4_falloc_update_inode(struct inode *inode,
> > 
> 
> -- 
> Ben Hutchings
> Once a job is fouled up, anything done to improve it makes it worse.


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

Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-28 Thread Theodore Ts'o 
On Sat, Jul 27, 2013 at 10:33:31PM +0100, Ben Hutchings wrote:
  --- a/fs/ext4/extents.c
  +++ b/fs/ext4/extents.c
  @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
   
  last_block = (inode-i_size + sb-s_blocksize - 1)
   EXT4_BLOCK_SIZE_BITS(sb);
  +retry:
  err = ext4_es_remove_extent(inode, last_block,
  EXT_MAX_BLOCKS - last_block);
  +   if (err == ENOMEM) {
 
 Positive ENOMEM?!  It looks like this value is bubbled up from
 __es_insert_extent() which returns the usual negative error codes.

Nice catch.  Yup, that's a problem.  I'll fix this upstream and mark
it cc:stable.

Until this goes upstream stable kernel maintainers can either: (a) fix
up this patch by making the line read err == -ENOMEM, (b) hold back
this patch until the companion patch to fix this goes upstream, or (c)
apply this now, since it's otherwise harmless and it does add error
checking to the ext4_ext_remove_space() call.

 
 Ben.
 
  +   cond_resched();
  +   congestion_wait(BLK_RW_ASYNC, HZ/50);
  +   goto retry;
  +   }
  +   if (err) {
  +   ext4_std_error(inode-i_sb, err);
  +   return;
  +   }
  err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
  +   ext4_std_error(inode-i_sb, err);
   }
   
   static void ext4_falloc_update_inode(struct inode *inode,
  
 
 -- 
 Ben Hutchings
 Once a job is fouled up, anything done to improve it makes it worse.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-28 Thread Greg Kroah-Hartman 
On Sun, Jul 28, 2013 at 07:40:53AM -0400, Theodore Ts'o wrote:
 On Sat, Jul 27, 2013 at 10:33:31PM +0100, Ben Hutchings wrote:
   --- a/fs/ext4/extents.c
   +++ b/fs/ext4/extents.c
   @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,

 last_block = (inode-i_size + sb-s_blocksize - 1)
  EXT4_BLOCK_SIZE_BITS(sb);
   +retry:
 err = ext4_es_remove_extent(inode, last_block,
 EXT_MAX_BLOCKS - last_block);
   + if (err == ENOMEM) {
  
  Positive ENOMEM?!  It looks like this value is bubbled up from
  __es_insert_extent() which returns the usual negative error codes.
 
 Nice catch.  Yup, that's a problem.  I'll fix this upstream and mark
 it cc:stable.
 
 Until this goes upstream stable kernel maintainers can either: (a) fix
 up this patch by making the line read err == -ENOMEM, (b) hold back
 this patch until the companion patch to fix this goes upstream, or (c)
 apply this now, since it's otherwise harmless and it does add error
 checking to the ext4_ext_remove_space() call.

I'll take it as-is and expect to see the fix come through Linus's tree
soon.

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-28 Thread Ben Hutchings 
On Sun, 2013-07-28 at 07:40 -0400, Theodore Ts'o wrote:
 On Sat, Jul 27, 2013 at 10:33:31PM +0100, Ben Hutchings wrote:
   --- a/fs/ext4/extents.c
   +++ b/fs/ext4/extents.c
   @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,

 last_block = (inode-i_size + sb-s_blocksize - 1)
  EXT4_BLOCK_SIZE_BITS(sb);
   +retry:
 err = ext4_es_remove_extent(inode, last_block,
 EXT_MAX_BLOCKS - last_block);
   + if (err == ENOMEM) {
  
  Positive ENOMEM?!  It looks like this value is bubbled up from
  __es_insert_extent() which returns the usual negative error codes.
 
 Nice catch.  Yup, that's a problem.  I'll fix this upstream and mark
 it cc:stable.
 
 Until this goes upstream stable kernel maintainers can either: (a) fix
 up this patch by making the line read err == -ENOMEM, (b) hold back
 this patch until the companion patch to fix this goes upstream, or (c)
 apply this now, since it's otherwise harmless and it does add error
 checking to the ext4_ext_remove_space() call.

We do need that second fix upstream first.  I agree that this seems to
fix a bug and doesn't make the ENOMEM case worse.  If this is applicable
to 3.2.y (I think it isn't but haven't checked yet) then I'll go with
(c).

Ben.

-- 
Ben Hutchings
All extremists should be taken out and shot.


signature.asc
Description: This is a digitally signed message part

Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-27 Thread Ben Hutchings 
On Fri, 2013-07-26 at 13:48 -0700, Greg Kroah-Hartman wrote:
> 3.10-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Theodore Ts'o 
> 
> commit 8acd5e9b1217e58a57124d9e225afa12efeae20d upstream.
> 
> Previously ext4_ext_truncate() was ignoring potential error returns
> from ext4_es_remove_extent() and ext4_ext_remove_space().  This can
> lead to the on-diks extent tree and the extent status tree cache
> getting out of sync, which is particuarlly bad, and can lead to file
> system corruption and potential data loss.
> 
> Signed-off-by: "Theodore Ts'o" 
> Signed-off-by: Greg Kroah-Hartman 
> 
> ---
>  fs/ext4/extents.c |   11 +++
>  1 file changed, 11 insertions(+)
> 
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
>  
>   last_block = (inode->i_size + sb->s_blocksize - 1)
>   >> EXT4_BLOCK_SIZE_BITS(sb);
> +retry:
>   err = ext4_es_remove_extent(inode, last_block,
>   EXT_MAX_BLOCKS - last_block);
> + if (err == ENOMEM) {

Positive ENOMEM?!  It looks like this value is bubbled up from
__es_insert_extent() which returns the usual negative error codes.

Ben.

> + cond_resched();
> + congestion_wait(BLK_RW_ASYNC, HZ/50);
> + goto retry;
> + }
> + if (err) {
> + ext4_std_error(inode->i_sb, err);
> + return;
> + }
>   err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
> + ext4_std_error(inode->i_sb, err);
>  }
>  
>  static void ext4_falloc_update_inode(struct inode *inode,
> 

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.


signature.asc
Description: This is a digitally signed message part

Re: [ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-27 Thread Ben Hutchings 
On Fri, 2013-07-26 at 13:48 -0700, Greg Kroah-Hartman wrote:
 3.10-stable review patch.  If anyone has any objections, please let me know.
 
 --
 
 From: Theodore Ts'o ty...@mit.edu
 
 commit 8acd5e9b1217e58a57124d9e225afa12efeae20d upstream.
 
 Previously ext4_ext_truncate() was ignoring potential error returns
 from ext4_es_remove_extent() and ext4_ext_remove_space().  This can
 lead to the on-diks extent tree and the extent status tree cache
 getting out of sync, which is particuarlly bad, and can lead to file
 system corruption and potential data loss.
 
 Signed-off-by: Theodore Ts'o ty...@mit.edu
 Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
 
 ---
  fs/ext4/extents.c |   11 +++
  1 file changed, 11 insertions(+)
 
 --- a/fs/ext4/extents.c
 +++ b/fs/ext4/extents.c
 @@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
  
   last_block = (inode-i_size + sb-s_blocksize - 1)
EXT4_BLOCK_SIZE_BITS(sb);
 +retry:
   err = ext4_es_remove_extent(inode, last_block,
   EXT_MAX_BLOCKS - last_block);
 + if (err == ENOMEM) {

Positive ENOMEM?!  It looks like this value is bubbled up from
__es_insert_extent() which returns the usual negative error codes.

Ben.

 + cond_resched();
 + congestion_wait(BLK_RW_ASYNC, HZ/50);
 + goto retry;
 + }
 + if (err) {
 + ext4_std_error(inode-i_sb, err);
 + return;
 + }
   err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
 + ext4_std_error(inode-i_sb, err);
  }
  
  static void ext4_falloc_update_inode(struct inode *inode,
 

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.


signature.asc
Description: This is a digitally signed message part

[ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-26 Thread Greg Kroah-Hartman 
3.10-stable review patch.  If anyone has any objections, please let me know.

--

From: Theodore Ts'o 

commit 8acd5e9b1217e58a57124d9e225afa12efeae20d upstream.

Previously ext4_ext_truncate() was ignoring potential error returns
from ext4_es_remove_extent() and ext4_ext_remove_space().  This can
lead to the on-diks extent tree and the extent status tree cache
getting out of sync, which is particuarlly bad, and can lead to file
system corruption and potential data loss.

Signed-off-by: "Theodore Ts'o" 
Signed-off-by: Greg Kroah-Hartman 

---
 fs/ext4/extents.c |   11 +++
 1 file changed, 11 insertions(+)

--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
 
last_block = (inode->i_size + sb->s_blocksize - 1)
>> EXT4_BLOCK_SIZE_BITS(sb);
+retry:
err = ext4_es_remove_extent(inode, last_block,
EXT_MAX_BLOCKS - last_block);
+   if (err == ENOMEM) {
+   cond_resched();
+   congestion_wait(BLK_RW_ASYNC, HZ/50);
+   goto retry;
+   }
+   if (err) {
+   ext4_std_error(inode->i_sb, err);
+   return;
+   }
err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
+   ext4_std_error(inode->i_sb, err);
 }
 
 static void ext4_falloc_update_inode(struct inode *inode,


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

[ 71/79] ext4: fix error handling in ext4_ext_truncate()

2013-07-26 Thread Greg Kroah-Hartman 
3.10-stable review patch.  If anyone has any objections, please let me know.

--

From: Theodore Ts'o ty...@mit.edu

commit 8acd5e9b1217e58a57124d9e225afa12efeae20d upstream.

Previously ext4_ext_truncate() was ignoring potential error returns
from ext4_es_remove_extent() and ext4_ext_remove_space().  This can
lead to the on-diks extent tree and the extent status tree cache
getting out of sync, which is particuarlly bad, and can lead to file
system corruption and potential data loss.

Signed-off-by: Theodore Ts'o ty...@mit.edu
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org

---
 fs/ext4/extents.c |   11 +++
 1 file changed, 11 insertions(+)

--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4386,9 +4386,20 @@ void ext4_ext_truncate(handle_t *handle,
 
last_block = (inode-i_size + sb-s_blocksize - 1)
 EXT4_BLOCK_SIZE_BITS(sb);
+retry:
err = ext4_es_remove_extent(inode, last_block,
EXT_MAX_BLOCKS - last_block);
+   if (err == ENOMEM) {
+   cond_resched();
+   congestion_wait(BLK_RW_ASYNC, HZ/50);
+   goto retry;
+   }
+   if (err) {
+   ext4_std_error(inode-i_sb, err);
+   return;
+   }
err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
+   ext4_std_error(inode-i_sb, err);
 }
 
 static void ext4_falloc_update_inode(struct inode *inode,


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/