[PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[Dmitry Monakhov]

If i_data_sem was internally dropped due to transaction restart, it is
necessary to restart path look-up because extents tree was possibly
modified by ext4_get_block().

https://bugzilla.kernel.org/show_bug.cgi?id=15827

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/ext4.h    |    1 +
 fs/ext4/extents.c |   18 +++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 6641c58..c69efb2 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1089,6 +1089,7 @@ enum {
 	EXT4_STATE_DA_ALLOC_CLOSE,	/* Alloc DA blks on close */
 	EXT4_STATE_EXT_MIGRATE,		/* Inode is migrating */
 	EXT4_STATE_DIO_UNWRITTEN,	/* need convert on dio done*/
+	EXT4_STATE_EXT_TRUNC,		/* truncate is in progress, modified under i_data_sem */
 };
 
 #define EXT4_INODE_BIT_FNS(name, field)					\
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 4fa103c..6856272 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -107,11 +107,8 @@ static int ext4_ext_truncate_extend_restart(handle_t *handle,
 	if (err <= 0)
 		return err;
 	err = ext4_truncate_restart_trans(handle, inode, needed);
-	/*
-	 * We have dropped i_data_sem so someone might have cached again
-	 * an extent we are going to truncate.
-	 */
-	ext4_ext_invalidate_cache(inode);
+	if (!err && !ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
+		err = -EAGAIN;
 
 	return err;
 }
@@ -2370,12 +2367,15 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
+again:
 	ext4_ext_invalidate_cache(inode);
 
 	/*
 	 * We start scanning from right side, freeing all the blocks
 	 * after i_size and walking into the tree depth-wise.
 	 */
+	ext4_set_inode_state(inode, EXT4_STATE_EXT_TRUNC);
+	depth = ext_depth(inode);
 	path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
 	if (path == NULL) {
 		ext4_journal_stop(handle);
@@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 out:
 	ext4_ext_drop_refs(path);
 	kfree(path);
+	if (err == EAGAIN) {
+		err = 0;
+		goto again;
+	}
+	ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
 	ext4_journal_stop(handle);
 
 	return err;
@@ -3338,6 +3343,9 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 	ext_debug("blocks %u/%u requested for inode %lu\n",
 			iblock, max_blocks, inode->i_ino);
 
+	if (unlikely((flags & EXT4_GET_BLOCKS_CREATE)) &&
+		ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
+		ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
 	/* check in cache */
 	cache_type = ext4_ext_in_cache(inode, iblock, &newex);
 	if (cache_type) {
-- 
1.6.6.1

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[Dmitry Monakhov]

Dmitry Monakhov <dmonakhov@openvz.org> writes:

Oops. i've missed last 'u' in 'edu', the Ted's email.
Add him to cc.
BTW seems that there is only one reproducible bug is left after
227's xfstest case. The bug with wrong i_blocks count:
Pass 1: Checking inodes, blocks, and sizes
Inode 3855, i_blocks is 288, should be 304.  Fix? no
This will be the hardest one because i'm able to discover wrong blocks
count only on fsck stage. I'm suspecting what quota charge is missed
somewhere for metablocks. Continue digging. 
> If i_data_sem was internally dropped due to transaction restart, it is
> necessary to restart path look-up because extents tree was possibly
> modified by ext4_get_block().
>
> https://bugzilla.kernel.org/show_bug.cgi?id=15827
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/ext4.h    |    1 +
>  fs/ext4/extents.c |   18 +++++++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 6641c58..c69efb2 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1089,6 +1089,7 @@ enum {
>  	EXT4_STATE_DA_ALLOC_CLOSE,	/* Alloc DA blks on close */
>  	EXT4_STATE_EXT_MIGRATE,		/* Inode is migrating */
>  	EXT4_STATE_DIO_UNWRITTEN,	/* need convert on dio done*/
> +	EXT4_STATE_EXT_TRUNC,		/* truncate is in progress, modified under i_data_sem */
>  };
>  
>  #define EXT4_INODE_BIT_FNS(name, field)					\
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 4fa103c..6856272 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -107,11 +107,8 @@ static int ext4_ext_truncate_extend_restart(handle_t *handle,
>  	if (err <= 0)
>  		return err;
>  	err = ext4_truncate_restart_trans(handle, inode, needed);
> -	/*
> -	 * We have dropped i_data_sem so someone might have cached again
> -	 * an extent we are going to truncate.
> -	 */
> -	ext4_ext_invalidate_cache(inode);
> +	if (!err && !ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
> +		err = -EAGAIN;
>  
>  	return err;
>  }
> @@ -2370,12 +2367,15 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>  	if (IS_ERR(handle))
>  		return PTR_ERR(handle);
>  
> +again:
>  	ext4_ext_invalidate_cache(inode);
>  
>  	/*
>  	 * We start scanning from right side, freeing all the blocks
>  	 * after i_size and walking into the tree depth-wise.
>  	 */
> +	ext4_set_inode_state(inode, EXT4_STATE_EXT_TRUNC);
> +	depth = ext_depth(inode);
>  	path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
>  	if (path == NULL) {
>  		ext4_journal_stop(handle);
> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>  out:
>  	ext4_ext_drop_refs(path);
>  	kfree(path);
> +	if (err == EAGAIN) {
> +		err = 0;
> +		goto again;
> +	}
> +	ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
>  	ext4_journal_stop(handle);
>  
>  	return err;
> @@ -3338,6 +3343,9 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
>  	ext_debug("blocks %u/%u requested for inode %lu\n",
>  			iblock, max_blocks, inode->i_ino);
>  
> +	if (unlikely((flags & EXT4_GET_BLOCKS_CREATE)) &&
> +		ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
> +		ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
>  	/* check in cache */
>  	cache_type = ext4_ext_in_cache(inode, iblock, &newex);
>  	if (cache_type) {

>  LocalWords:  xfstest

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[Jan Kara]

On Thu 22-04-10 08:31:11, Dmitry Monakhov wrote:
> If i_data_sem was internally dropped due to transaction restart, it is
> necessary to restart path look-up because extents tree was possibly
> modified by ext4_get_block().
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=15827
  Nice spotting. The patch looks good to me.

Acked-by: Jan Kara <jack@suse.cz>

								Honza
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/ext4.h    |    1 +
>  fs/ext4/extents.c |   18 +++++++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 6641c58..c69efb2 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1089,6 +1089,7 @@ enum {
>  	EXT4_STATE_DA_ALLOC_CLOSE,	/* Alloc DA blks on close */
>  	EXT4_STATE_EXT_MIGRATE,		/* Inode is migrating */
>  	EXT4_STATE_DIO_UNWRITTEN,	/* need convert on dio done*/
> +	EXT4_STATE_EXT_TRUNC,		/* truncate is in progress, modified under i_data_sem */
>  };
>  
>  #define EXT4_INODE_BIT_FNS(name, field)					\
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 4fa103c..6856272 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -107,11 +107,8 @@ static int ext4_ext_truncate_extend_restart(handle_t *handle,
>  	if (err <= 0)
>  		return err;
>  	err = ext4_truncate_restart_trans(handle, inode, needed);
> -	/*
> -	 * We have dropped i_data_sem so someone might have cached again
> -	 * an extent we are going to truncate.
> -	 */
> -	ext4_ext_invalidate_cache(inode);
> +	if (!err && !ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
> +		err = -EAGAIN;
>  
>  	return err;
>  }
> @@ -2370,12 +2367,15 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>  	if (IS_ERR(handle))
>  		return PTR_ERR(handle);
>  
> +again:
>  	ext4_ext_invalidate_cache(inode);
>  
>  	/*
>  	 * We start scanning from right side, freeing all the blocks
>  	 * after i_size and walking into the tree depth-wise.
>  	 */
> +	ext4_set_inode_state(inode, EXT4_STATE_EXT_TRUNC);
> +	depth = ext_depth(inode);
>  	path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
>  	if (path == NULL) {
>  		ext4_journal_stop(handle);
> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>  out:
>  	ext4_ext_drop_refs(path);
>  	kfree(path);
> +	if (err == EAGAIN) {
> +		err = 0;
> +		goto again;
> +	}
> +	ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
>  	ext4_journal_stop(handle);
>  
>  	return err;
> @@ -3338,6 +3343,9 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
>  	ext_debug("blocks %u/%u requested for inode %lu\n",
>  			iblock, max_blocks, inode->i_ino);
>  
> +	if (unlikely((flags & EXT4_GET_BLOCKS_CREATE)) &&
> +		ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
> +		ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
>  	/* check in cache */
>  	cache_type = ext4_ext_in_cache(inode, iblock, &newex);
>  	if (cache_type) {
> -- 
> 1.6.6.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[tytso]

On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>  out:
>  	ext4_ext_drop_refs(path);
>  	kfree(path);
> +	if (err == EAGAIN) {

Surely this should be "err == -EAGAIN", no?  I'm curious how this
patch worked for with this typo....

> +		err = 0;
> +		goto again;
> +	}
> +	ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
>  	ext4_journal_stop(handle);
>  
>  	return err;

					- Ted

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[Dmitry Monakhov]

tytso@mit.edu writes:

> On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
>> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>>  out:
>>  	ext4_ext_drop_refs(path);
>>  	kfree(path);
>> +	if (err == EAGAIN) {
>
> Surely this should be "err == -EAGAIN", no?  I'm curious how this
> patch worked for with this typo....
As usually it fix one thing, and broke another :(.
So in case of alloc/truncate restart truncate will be aborted,
so i_size != i_disk_size which must be caught by fsck (my test run
it every time) but this never happens which is very strange.
The only reason i can explain this that truncate was called second
time which is probable due to should_retry_alloc logic.
Even more than this, i've changed the mistypo and have got massive
complain from fsck
Block bitmap differences:  -7954 -(33836--33854)
Fix<y>? yes

Free blocks count wrong for group #0 (24170, counted=24171).
Fix<y>? yes
...
Currently i'm digging the issue.

>
>> +		err = 0;
>> +		goto again;
>> +	}
>> +	ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
>>  	ext4_journal_stop(handle);
>>  
>>  	return err;
>
> 					- Ted

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[tytso]

On Tue, May 25, 2010 at 06:28:29PM +0400, Dmitry Monakhov wrote:
> tytso@mit.edu writes:
> 
> > On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
> >> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
> >>  out:
> >>  	ext4_ext_drop_refs(path);
> >>  	kfree(path);
> >> +	if (err == EAGAIN) {
> >
> > Surely this should be "err == -EAGAIN", no?  I'm curious how this
> > patch worked for with this typo....
> As usually it fix one thing, and broke another :(.
> So in case of alloc/truncate restart truncate will be aborted,
> so i_size != i_disk_size which must be caught by fsck (my test run
> it every time) but this never happens which is very strange.
> The only reason i can explain this that truncate was called second
> time which is probable due to should_retry_alloc logic.

Does adding the optimization I suggested help?  I was nervous because
we don't immediately abort the loop after the rm_leaf function returns
-EAGAIN.  And disentangling the code to free the buffer references
from the other processing that was happening was difficult, and I was
worried about other potential side effects when the code tried to
modify blocks that were already added to the transaction.

       	      	   		      	     - Ted

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[Dmitry Monakhov]

tytso@mit.edu writes:

> On Tue, May 25, 2010 at 06:28:29PM +0400, Dmitry Monakhov wrote:
>> tytso@mit.edu writes:
>> 
>> > On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
>> >> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>> >>  out:
>> >>  	ext4_ext_drop_refs(path);
>> >>  	kfree(path);
>> >> +	if (err == EAGAIN) {
>> >
>> > Surely this should be "err == -EAGAIN", no?  I'm curious how this
>> > patch worked for with this typo....
>> As usually it fix one thing, and broke another :(.
>> So in case of alloc/truncate restart truncate will be aborted,
>> so i_size != i_disk_size which must be caught by fsck (my test run
>> it every time) but this never happens which is very strange.
>> The only reason i can explain this that truncate was called second
>> time which is probable due to should_retry_alloc logic.
>
> Does adding the optimization I suggested help?  I was nervous because
> we don't immediately abort the loop after the rm_leaf function returns
> -EAGAIN.
Sorry, but seems i don't get your idea.
we have following code:
2394: while (i >= 0 && err == 0) {
                if (i == depth) {
                        /* this is leaf block */
                        err = ext4_ext_rm_leaf(handle, inode, path,
                        start);
                        /* root level has p_bh == NULL, brelse() eats this */
                        brelse(path[i].p_bh);
                        path[i].p_bh = NULL;
                        i--;
                        continue;
                       ^^^^^^^^^^^^^^^^^
<<< So if rm_leaf has failed we will quit from while loop
                }

>And disentangling the code to free the buffer references
> from the other processing that was happening was difficult, and I was
> worried about other potential side effects when the code tried to
> modify blocks that were already added to the transaction.
>
>        	      	   		      	     - Ted

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[tytso]

On Tue, May 25, 2010 at 06:28:29PM +0400, Dmitry Monakhov wrote:
> tytso@mit.edu writes:
> 
> > On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
> >> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
> >>  out:
> >>  	ext4_ext_drop_refs(path);
> >>  	kfree(path);
> >> +	if (err == EAGAIN) {
> >
> > Surely this should be "err == -EAGAIN", no?  I'm curious how this
> > patch worked for with this typo....
> As usually it fix one thing, and broke another :(.
> So in case of alloc/truncate restart truncate will be aborted,
> so i_size != i_disk_size which must be caught by fsck (my test run
> it every time) but this never happens which is very strange.

What test case are you using?  And does it require a system crash to
show up, or are you seeing an fsck problem after the test completes
and you unmount the file system?

						- Ted

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[Dmitry Monakhov]

tytso@mit.edu writes:

> On Tue, May 25, 2010 at 06:28:29PM +0400, Dmitry Monakhov wrote:
>> tytso@mit.edu writes:
>> 
>> > On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
>> >> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>> >>  out:
>> >>  	ext4_ext_drop_refs(path);
>> >>  	kfree(path);
>> >> +	if (err == EAGAIN) {
>> >
>> > Surely this should be "err == -EAGAIN", no?  I'm curious how this
>> > patch worked for with this typo....
>> As usually it fix one thing, and broke another :(.
>> So in case of alloc/truncate restart truncate will be aborted,
>> so i_size != i_disk_size which must be caught by fsck (my test run
>> it every time) but this never happens which is very strange.
Ohh i ment to say blocks beyond i_disk_size due to aborted truncate.
> What test case are you using?  And does it require a system crash to
> show up, or are you seeing an fsck problem after the test completes
> and you unmount the file system?
crash is not required.
I use proposed xfsqa tests from the bug, may be i've changed some 
numbers, but core idea stays the same.
mount /dev/sdb1 /mnt
fsstress ..... &
sleep 300; killall -9 fsstress
umount /mnt
fsck -f /dev/sdb1
After you have spotted the mistypo i've add explicit fault injection 
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -98,9 +98,15 @@ static int ext4_ext_truncate_extend_restart(handle_t
>> > *handle,                                            int needed)
 {
        int err;
+       static int fault = 0;

        if (!ext4_handle_valid(handle))
                return 0;
+       if (inode->i_size % 1234 == 0 && fault++ % 2) {
+               printk("EXT4 TRUNC fault inject inode:%ld\n",inode->i_ino);
+               dump_stack();
+               return -EAGAIN;
+       }

And i've got complain from fsck about incorrect i_size which should be
increased due to block beyond i_disk_size as expected.
And when i've fixed the mistypo i've had different complain due to
bitmap  difference.

[PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

Dmitry Monakhov <dmonakhov@openvz.org> writes:

> tytso@mit.edu writes:
>
>> On Tue, May 25, 2010 at 06:28:29PM +0400, Dmitry Monakhov wrote:
>>> tytso@mit.edu writes:
>>> 
>>> > On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
>>> >> @@ -2480,6 +2480,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>>> >>  out:
>>> >>  	ext4_ext_drop_refs(path);
>>> >>  	kfree(path);
>>> >> +	if (err == EAGAIN) {
>>> >
>>> > Surely this should be "err == -EAGAIN", no?  I'm curious how this
>>> > patch worked for with this typo....
>>> As usually it fix one thing, and broke another :(.
>>> So in case of alloc/truncate restart truncate will be aborted,
>>> so i_size != i_disk_size which must be caught by fsck (my test run
>>> it every time) but this never happens which is very strange.
> Ohh i ment to say blocks beyond i_disk_size due to aborted truncate.
>> What test case are you using?  And does it require a system crash to
>> show up, or are you seeing an fsck problem after the test completes
>> and you unmount the file system?
> crash is not required.
> I use proposed xfsqa tests from the bug, may be i've changed some 
> numbers, but core idea stays the same.
> mount /dev/sdb1 /mnt
> fsstress ..... &
> sleep 300; killall -9 fsstress
> umount /mnt
> fsck -f /dev/sdb1
> After you have spotted the mistypo i've add explicit fault injection 
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -98,9 +98,15 @@ static int ext4_ext_truncate_extend_restart(handle_t
>>> > *handle,                                            int needed)
>  {
>         int err;
> +       static int fault = 0;
>
>         if (!ext4_handle_valid(handle))
>                 return 0;
> +       if (inode->i_size % 1234 == 0 && fault++ % 2) {
> +               printk("EXT4 TRUNC fault inject inode:%ld\n",inode->i_ino);
> +               dump_stack();
> +               return -EAGAIN;
> +       }
>
> And i've got complain from fsck about incorrect i_size which should be
> increased due to block beyond i_disk_size as expected.
> And when i've fixed the mistypo i've had different complain due to
> bitmap  difference.
This is more than just a bad luck, seems what my brain wasn't enabled
yesterday and at the time i wrote the patch.
I've added 'again' label but forgot to reinitialize "i" variable to zero
again :( . Sorry for wasting you time for this sort of foolishness.
Now it is pass all my tests:
1) fsstress -p100
2) fsstress -p100 with fault injection from journal_restart.
See correct version attached. 

[-- Attachment #2: 0001-ext4-restart-ext4_ext_remove_space-after-transaction.patch --]
[-- Type: text/plain, Size: 3227 bytes --]

>From da147cf458b2b68486b063725afa2d2a2f8d6e2e Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Wed, 26 May 2010 15:37:03 +0400
Subject: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart v2

If i_data_sem was internally dropped due to transaction restart, it is
necessary to restart path look-up because extents tree was possibly
modified by ext4_get_block().

https://bugzilla.kernel.org/show_bug.cgi?id=15827

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/ext4.h    |    1 +
 fs/ext4/extents.c |   21 +++++++++++++++------
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 3b63837..36e6a32 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1162,6 +1162,7 @@ enum {
 	EXT4_STATE_DA_ALLOC_CLOSE,	/* Alloc DA blks on close */
 	EXT4_STATE_EXT_MIGRATE,		/* Inode is migrating */
 	EXT4_STATE_DIO_UNWRITTEN,	/* need convert on dio done*/
+	EXT4_STATE_EXT_TRUNC,		/* truncate is in progress, modified under i_data_sem */
 };
 
 #define EXT4_INODE_BIT_FNS(name, field)					\
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index c7c304f..3321f57 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -107,11 +107,8 @@ static int ext4_ext_truncate_extend_restart(handle_t *handle,
 	if (err <= 0)
 		return err;
 	err = ext4_truncate_restart_trans(handle, inode, needed);
-	/*
-	 * We have dropped i_data_sem so someone might have cached again
-	 * an extent we are going to truncate.
-	 */
-	ext4_ext_invalidate_cache(inode);
+	if (!err && !ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
+		err = -EAGAIN;
 
 	return err;
 }
@@ -2359,7 +2356,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 	int depth = ext_depth(inode);
 	struct ext4_ext_path *path;
 	handle_t *handle;
-	int i = 0, err = 0;
+	int i, err = 0;
 
 	ext_debug("truncate since %u\n", start);
 
@@ -2368,12 +2365,16 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
+again:
 	ext4_ext_invalidate_cache(inode);
 
 	/*
 	 * We start scanning from right side, freeing all the blocks
 	 * after i_size and walking into the tree depth-wise.
 	 */
+	i = 0;
+	ext4_set_inode_state(inode, EXT4_STATE_EXT_TRUNC);
+	depth = ext_depth(inode);
 	path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
 	if (path == NULL) {
 		ext4_journal_stop(handle);
@@ -2478,6 +2479,11 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 out:
 	ext4_ext_drop_refs(path);
 	kfree(path);
+	if (err == -EAGAIN) {
+		err = 0;
+		goto again;
+	}
+	ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
 	ext4_journal_stop(handle);
 
 	return err;
@@ -3327,6 +3333,9 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	ext_debug("blocks %u/%u requested for inode %lu\n",
 		  map->m_lblk, map->m_len, inode->i_ino);
 
+	if (unlikely((flags & EXT4_GET_BLOCKS_CREATE)) &&
+		ext4_test_inode_state(inode, EXT4_STATE_EXT_TRUNC))
+		ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC);
 	/* check in cache */
 	cache_type = ext4_ext_in_cache(inode, map->m_lblk, &newex);
 	if (cache_type) {
-- 
1.6.6.1

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

[tytso]

One more thing.  Why do you need EXT4_STATE_EXT_TRUNC?

The only place which tests it in any kind of real way is
ext4_ext_truncate_extend_restart(), and it is only called by one
function, ext4_ext_rm_leaf(), and *it* is only called in one place,
inside ext4_ext_remove_space(), and *it* surronds the call with
ext4_set_inode_state(inode, EXT4_STATE_EXT_TRUNC) and
ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC).

And while a truncate is happening, no other block allocation can
happen, so the test in ext4_ext_map_blocks() doesn't seem to do much.
(It only clears STATE_EXT_TRUNC if it is set and if the flags
EXT4_GET_BLOCKS_CREATE is set.  I'm not sure what the point of that
is, either.)

					- Ted

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

[Jan Kara]

On Wed 26-05-10 09:23:52, tytso@mit.edu wrote:
> One more thing.  Why do you need EXT4_STATE_EXT_TRUNC?
> 
> The only place which tests it in any kind of real way is
> ext4_ext_truncate_extend_restart(), and it is only called by one
> function, ext4_ext_rm_leaf(), and *it* is only called in one place,
> inside ext4_ext_remove_space(), and *it* surronds the call with
> ext4_set_inode_state(inode, EXT4_STATE_EXT_TRUNC) and
> ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC).
> 
> And while a truncate is happening, no other block allocation can
> happen, so the test in ext4_ext_map_blocks() doesn't seem to do much.
  This is false. As soon as we drop i_data_sem, allocation *can* happen
from writeback path. Because truncate has already invalidated all the pages
past new_size, it must be for some page before new_size but still it could
modify an extent tree node we passed through when looking up our extent...

> (It only clears STATE_EXT_TRUNC if it is set and if the flags
> EXT4_GET_BLOCKS_CREATE is set.  I'm not sure what the point of that
> is, either.)
  I think the idea Dmitry tries to implement is:
When allocation like I describe above happens while we droppped i_data_sem,
restart the whole truncation.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

[Dmitry Monakhov]

tytso@mit.edu writes:

> One more thing.  Why do you need EXT4_STATE_EXT_TRUNC?
>
> The only place which tests it in any kind of real way is
> ext4_ext_truncate_extend_restart(), and it is only called by one
> function, ext4_ext_rm_leaf(), and *it* is only called in one place,
> inside ext4_ext_remove_space(), and *it* surronds the call with
> ext4_set_inode_state(inode, EXT4_STATE_EXT_TRUNC) and
> ext4_clear_inode_state(inode, EXT4_STATE_EXT_TRUNC).
>
> And while a truncate is happening, no other block allocation can
> happen, so the test in ext4_ext_map_blocks() doesn't seem to do much.
This is the biggest myth about truncate. Personally i always use to
thought like this. But later i've found that it is not so. See later.
> (It only clears STATE_EXT_TRUNC if it is set and if the flags
> EXT4_GET_BLOCKS_CREATE is set.  I'm not sure what the point of that
> is, either.)
This is the core idea of the patch.

*Truncate* task set the bit to signal that truncate is under progress for
inode.

Later if we face a needs to restart transaction which result in
i_data_sem indernal drop/acquire.
And when the sem was dropped new block may be allocated by a
flusher task (delay allocation writeback in the middle of the file)

*Flusher* will discover than STATE_EXT_TRUNC is set and clear it
is allocation is really necessary(EXT4_GET_BLOCKS_CREATE is set)
By clearing STATE_EXT_TRUNC bit flusher let truncate task to know
what it have to restart it's job. 

*Back to truncate task*. We can may sure what allocation not happens by
testing STATE_EXT_TRUNC bit. And it it was cleared we have to
restart truncate from very beginning because all data we have collected
may not longer be valid, even depth of the file may increase.

For example if we about to truncate inode with following leaf block
{ee_block:1000, ee_len:100}
So if block allocation happens while we are restarting transaction
leaf block may looks like follows:
{ee_block:500, ee_len:10} {ee_block:1000, ee_len:10} 
See that latest extent has changed it's position.
This was not an issue for ext3 because it's blocks placed in deterministic
positions.

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

[tytso]

On Wed, May 26, 2010 at 06:23:55PM +0400, Dmitry Monakhov wrote:
> 
> Later if we face a needs to restart transaction which result in
> i_data_sem indernal drop/acquire.
> And when the sem was dropped new block may be allocated by a
> flusher task (delay allocation writeback in the middle of the file)
> 
> *Flusher* will discover than STATE_EXT_TRUNC is set and clear it
> is allocation is really necessary(EXT4_GET_BLOCKS_CREATE is set)
> By clearing STATE_EXT_TRUNC bit flusher let truncate task to know
> what it have to restart it's job. 

OK, but why not have the truncate *always* restart its job after
restarting the transaction?  #1, it's relatively rare in most
workloads that we need to restart the transaction at all in the first
place, and #2, it's easier to test if we always restart the truncate,
and #3, it's not like we'll be doing that much extra work if we
restart the truncate and the file wasn't extended significantly...

	    	     	     	  - Ted

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

[tytso]

On Wed, May 26, 2010 at 10:45:45AM -0400, tytso@MIT.EDU wrote:
> OK, but why not have the truncate *always* restart its job after
> restarting the transaction?  #1, it's relatively rare in most
> workloads that we need to restart the transaction at all in the first
> place, and #2, it's easier to test if we always restart the truncate,
> and #3, it's not like we'll be doing that much extra work if we
> restart the truncate and the file wasn't extended significantly...
> 

Like this.... (note how much simpler this patch is compared to earlier
versions)

						- Ted

ext4: restart ext4_ext_remove_space() after transaction restart

From: Dmitry Monakhov <dmonakhov@openvz.org>

If i_data_sem was internally dropped due to transaction restart, it is
necessary to restart path look-up because extents tree was possibly
modified by ext4_get_block().

https://bugzilla.kernel.org/show_bug.cgi?id=15827

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Acked-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/extents.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ffcaa11..0a47bec 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -107,11 +107,8 @@ static int ext4_ext_truncate_extend_restart(handle_t *handle,
 	if (err <= 0)
 		return err;
 	err = ext4_truncate_restart_trans(handle, inode, needed);
-	/*
-	 * We have dropped i_data_sem so someone might have cached again
-	 * an extent we are going to truncate.
-	 */
-	ext4_ext_invalidate_cache(inode);
+	if (err == 0)
+		err = -EAGAIN;
 
 	return err;
 }
@@ -2359,7 +2356,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 	int depth = ext_depth(inode);
 	struct ext4_ext_path *path;
 	handle_t *handle;
-	int i = 0, err = 0;
+	int i, err;
 
 	ext_debug("truncate since %u\n", start);
 
@@ -2368,23 +2365,26 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
+again:
 	ext4_ext_invalidate_cache(inode);
 
 	/*
 	 * We start scanning from right side, freeing all the blocks
 	 * after i_size and walking into the tree depth-wise.
 	 */
+	depth = ext_depth(inode);
 	path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
 	if (path == NULL) {
 		ext4_journal_stop(handle);
 		return -ENOMEM;
 	}
+	path[0].p_depth = depth;
 	path[0].p_hdr = ext_inode_hdr(inode);
 	if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
 		err = -EIO;
 		goto out;
 	}
-	path[0].p_depth = depth;
+	i = err = 0;
 
 	while (i >= 0 && err == 0) {
 		if (i == depth) {
@@ -2478,6 +2478,8 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 out:
 	ext4_ext_drop_refs(path);
 	kfree(path);
+	if (err == -EAGAIN)
+		goto again;
 	ext4_journal_stop(handle);
 
 	return err;

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart V2

[Dmitry Monakhov]

On Wed, May 26, 2010 at 6:47 PM,  <tytso@mit.edu> wrote:
> On Wed, May 26, 2010 at 10:45:45AM -0400, tytso@MIT.EDU wrote:
>> OK, but why not have the truncate *always* restart its job after
>> restarting the transaction?  #1, it's relatively rare in most
>> workloads that we need to restart the transaction at all in the first
>> place, and #2, it's easier to test if we always restart the truncate,
>> and #3, it's not like we'll be doing that much extra work if we
>> restart the truncate and the file wasn't extended significantly...
>>
>
> Like this.... (note how much simpler this patch is compared to earlier
> versions)
I've added statistic counters to truncate code and found that each
4'th rm_leaf() result in
actual transaction restart, and number of truncate/get_block races is
less than 0.001 from total
restart number and may be neglected.  So simplified version perform
false restart for each 4'th truncate.
But fairly to say what this is still invisible in terms of total truncate time.
Do we care about performance in truncate? IMHO it is not the hottest
path so it is possible to end up with
simplified version.

>
>                                                - Ted
>
> ext4: restart ext4_ext_remove_space() after transaction restart
>
> From: Dmitry Monakhov <dmonakhov@openvz.org>
>
> If i_data_sem was internally dropped due to transaction restart, it is
> necessary to restart path look-up because extents tree was possibly
> modified by ext4_get_block().
>
> https://bugzilla.kernel.org/show_bug.cgi?id=15827
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Acked-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/extents.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ffcaa11..0a47bec 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -107,11 +107,8 @@ static int ext4_ext_truncate_extend_restart(handle_t *handle,
>        if (err <= 0)
>                return err;
>        err = ext4_truncate_restart_trans(handle, inode, needed);
> -       /*
> -        * We have dropped i_data_sem so someone might have cached again
> -        * an extent we are going to truncate.
> -        */
> -       ext4_ext_invalidate_cache(inode);
> +       if (err == 0)
> +               err = -EAGAIN;
>
>        return err;
>  }
> @@ -2359,7 +2356,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>        int depth = ext_depth(inode);
>        struct ext4_ext_path *path;
>        handle_t *handle;
> -       int i = 0, err = 0;
> +       int i, err;
>
>        ext_debug("truncate since %u\n", start);
>
> @@ -2368,23 +2365,26 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>        if (IS_ERR(handle))
>                return PTR_ERR(handle);
>
> +again:
>        ext4_ext_invalidate_cache(inode);
>
>        /*
>         * We start scanning from right side, freeing all the blocks
>         * after i_size and walking into the tree depth-wise.
>         */
> +       depth = ext_depth(inode);
>        path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
>        if (path == NULL) {
>                ext4_journal_stop(handle);
>                return -ENOMEM;
>        }
> +       path[0].p_depth = depth;
>        path[0].p_hdr = ext_inode_hdr(inode);
>        if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
>                err = -EIO;
>                goto out;
>        }
> -       path[0].p_depth = depth;
> +       i = err = 0;
>
>        while (i >= 0 && err == 0) {
>                if (i == depth) {
> @@ -2478,6 +2478,8 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
>  out:
>        ext4_ext_drop_refs(path);
>        kfree(path);
> +       if (err == -EAGAIN)
> +               goto again;
>        ext4_journal_stop(handle);
>
>        return err;
>
--
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

Re: [PATCH] ext4: restart ext4_ext_remove_space() after transaction restart

[tytso]

On Thu, Apr 22, 2010 at 08:31:11AM +0400, Dmitry Monakhov wrote:
> If i_data_sem was internally dropped due to transaction restart, it is
> necessary to restart path look-up because extents tree was possibly
> modified by ext4_get_block().
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=15827
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>

I *think* it would be more efficient to add the additional change:

  	  if (ext4_ext_more_to_rm(path + i)) {

to:

  	  if ((err != -EAGAIN) && ext4_ext_more_to_rm(path + i)) {

in ext4_ext_remove_space() but I would like your opinion...

If we do this optimization I'll probably do it in a separate patch.
It just seems that we're doing a lot of extra work once we fail to
extend the transaction, so it would be good to optimize more of this
out.  It also makes it easier to convince oneself that that all of the
spinning around that happens after returning EAGAIN won't cause any
harm.  After going through the patch carefully I'm pretty sure the
extra work is pointless, but not harmful, but it's better to skip it
entirely if it's not needed.

					 - Ted