[PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Ashish Sangwan]

The current implementation of hole punch uses the same code path as for
truncate in which the block removal starts from the end of the file and
continue backwards removing extents and in the process decreases the
count of valid entries in the extent header of extent index.
If all the extents under current extent index are removed than this index
is also removed and the number of valid entries in the "depth -1"
extent header is decreased by 1.

Whether to continue removing extents or not is decided by the return value
of function ext4_ext_more_to_rm() which checks 2 conditions:
a) if there are no more indexes to process.
b) if the number of entries are decreased in the header of "depth -1".

In case of hole punch, if the last block to be removed is not part of the
last extent index than this index will not be deleted, hence the number of
valid entries in the extent header of "depth - 1" will remain as it is and
ext4_ext_more_to_rm will return 0 although the required blocks are not
yet removed.

This patch fixes the above mentioned problem as instead of removing the
extents from the end of file, it starts removing the blocks from the
particular extent from which removing blocks is actually required and
continue backward until done.

Signed-off-by: Ashish Sangwan <ashish.sangwan2@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
---
 fs/ext4/extents.c |   43 +++++++++++++++++++++++++++----------------
 1 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 91341ec..27c3a30 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2570,10 +2570,10 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
 {
 	struct super_block *sb = inode->i_sb;
 	int depth = ext_depth(inode);
-	struct ext4_ext_path *path;
+	struct ext4_ext_path *path = NULL;
 	ext4_fsblk_t partial_cluster = 0;
 	handle_t *handle;
-	int i, err;
+	int i = 0, err;
 
 	ext_debug("truncate since %u to %u\n", start, end);
 
@@ -2606,8 +2606,12 @@ again:
 		}
 		depth = ext_depth(inode);
 		ex = path[depth].p_ext;
-		if (!ex)
+		if (!ex) {
+			ext4_ext_drop_refs(path);
+			kfree(path);
+			path = NULL;
 			goto cont;
+		}
 
 		ee_block = le32_to_cpu(ex->ee_block);
 
@@ -2637,8 +2641,6 @@ again:
 			if (err < 0)
 				goto out;
 		}
-		ext4_ext_drop_refs(path);
-		kfree(path);
 	}
 cont:
 
@@ -2647,19 +2649,27 @@ cont:
 	 * 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 (path) {
+		int k = i = depth;
+		while (--k >= 0)
+			path[k].p_block =
+			le16_to_cpu(path[k].p_hdr->eh_entries)+1;
+	} else {
+		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;
+		if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
+			err = -EIO;
+			goto out;
+		}
 	}
-	i = err = 0;
+	err = 0;
 
 	while (i >= 0 && err == 0) {
 		if (i == depth) {
@@ -2773,6 +2783,7 @@ cont:
 out:
 	ext4_ext_drop_refs(path);
 	kfree(path);
+	path = NULL;
 	if (err == -EAGAIN)
 		goto again;
 	ext4_journal_stop(handle);
-- 
1.7.2.3

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Namjae Jeon]

Hi. Eric.

This is the patch for punch hole issue.

Thanks.

2012/6/14, Ashish Sangwan <ashishsangwan2@gmail.com>:
> The current implementation of hole punch uses the same code path as for
> truncate in which the block removal starts from the end of the file and
> continue backwards removing extents and in the process decreases the
> count of valid entries in the extent header of extent index.
> If all the extents under current extent index are removed than this index
> is also removed and the number of valid entries in the "depth -1"
> extent header is decreased by 1.
>
> Whether to continue removing extents or not is decided by the return value
> of function ext4_ext_more_to_rm() which checks 2 conditions:
> a) if there are no more indexes to process.
> b) if the number of entries are decreased in the header of "depth -1".
>
> In case of hole punch, if the last block to be removed is not part of the
> last extent index than this index will not be deleted, hence the number of
> valid entries in the extent header of "depth - 1" will remain as it is and
> ext4_ext_more_to_rm will return 0 although the required blocks are not
> yet removed.
>
> This patch fixes the above mentioned problem as instead of removing the
> extents from the end of file, it starts removing the blocks from the
> particular extent from which removing blocks is actually required and
> continue backward until done.
>
> Signed-off-by: Ashish Sangwan <ashish.sangwan2@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
> ---
>  fs/ext4/extents.c |   43 +++++++++++++++++++++++++++----------------
>  1 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 91341ec..27c3a30 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2570,10 +2570,10 @@ static int ext4_ext_remove_space(struct inode
> *inode, ext4_lblk_t start,
>  {
>  	struct super_block *sb = inode->i_sb;
>  	int depth = ext_depth(inode);
> -	struct ext4_ext_path *path;
> +	struct ext4_ext_path *path = NULL;
>  	ext4_fsblk_t partial_cluster = 0;
>  	handle_t *handle;
> -	int i, err;
> +	int i = 0, err;
>
>  	ext_debug("truncate since %u to %u\n", start, end);
>
> @@ -2606,8 +2606,12 @@ again:
>  		}
>  		depth = ext_depth(inode);
>  		ex = path[depth].p_ext;
> -		if (!ex)
> +		if (!ex) {
> +			ext4_ext_drop_refs(path);
> +			kfree(path);
> +			path = NULL;
>  			goto cont;
> +		}
>
>  		ee_block = le32_to_cpu(ex->ee_block);
>
> @@ -2637,8 +2641,6 @@ again:
>  			if (err < 0)
>  				goto out;
>  		}
> -		ext4_ext_drop_refs(path);
> -		kfree(path);
>  	}
>  cont:
>
> @@ -2647,19 +2649,27 @@ cont:
>  	 * 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 (path) {
> +		int k = i = depth;
> +		while (--k >= 0)
> +			path[k].p_block =
> +			le16_to_cpu(path[k].p_hdr->eh_entries)+1;
> +	} else {
> +		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;
> +		if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
> +			err = -EIO;
> +			goto out;
> +		}
>  	}
> -	i = err = 0;
> +	err = 0;
>
>  	while (i >= 0 && err == 0) {
>  		if (i == depth) {
> @@ -2773,6 +2783,7 @@ cont:
>  out:
>  	ext4_ext_drop_refs(path);
>  	kfree(path);
> +	path = NULL;
>  	if (err == -EAGAIN)
>  		goto again;
>  	ext4_journal_stop(handle);
> --
> 1.7.2.3
>
>

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Ashish Sangwan]

Hi Ted, Lukas, Eric,

Did any of you get a chance to look at it?

On Fri, Jun 22, 2012 at 6:19 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
> Hi. Eric.
>
> This is the patch for punch hole issue.
>
> Thanks.
>

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 858 bytes --]

On Mon, 2 Jul 2012, Ashish Sangwan wrote:

> Date: Mon, 2 Jul 2012 11:03:26 +0530
> From: Ashish Sangwan <ashishsangwan2@gmail.com>
> To: sandeen@redhat.com, Lukáš Czerner <lczerner@redhat.com>,
>     Ted Tso <tytso@mit.edu>
> Cc: linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org,
>     Namjae Jeon <linkinjeon@gmail.com>
> Subject: Re: [PATCH v2] ext4: fix hole punch failure when depth is greater
>     than 0
> 
> Hi Ted, Lukas, Eric,
> 
> Did any of you get a chance to look at it?

I am sorry for the delay. I have been trying to reproduce this
problem without any success so far. But is was on ppc64 machine, so
I'll try that on x86_64 and then review the patch.

Thanks!
-Lukas

> 
> On Fri, Jun 22, 2012 at 6:19 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
> > Hi. Eric.
> >
> > This is the patch for punch hole issue.
> >
> > Thanks.
> >
> 

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Ashish Sangwan]

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

I will try to elaborate more about the problem and steps to reproduce:
Linux version 3.4.0 on X86 box.

Step1:
First create a small partition as it would be easy to fragment quickly.
The main intent for fragmenting the partition is to create a file with
at least 2 extent indexes.
You can also choose some other method to create such a file.
I created 200Mb partition and while formating used blocksize as 4KB
and used attached script to fragment.
This script fragments the partition such that each extent is exactly
6FS blocks OR 24KB in length.
Linux#> ./fragment.sh /mnt/
6+0 records in
6+0 records out
24576 bytes (24.0KB) copied, 0.087997 seconds, 272.7KB/s
cp: write error: No space left on device
Partition filled
rm: can't remove '/mnt//file1.7564': No such file or directory
fragmented partition /mnt/ with 24KB files

Step2:
Create a file with 342 extents i.e 2 extent indexes
[root@sputnik ashish]# dd if=linux-3.4.tar.bz2 of=check_1 bs=24576 count=342
342+0 records in
342+0 records out
8404992 bytes (8.4 MB) copied, 0.0913289 s, 92.0 MB/s
[root@sputnik ashish]# cp check_1 /mnt/check_2
[root@sputnik ashish]# debugfs  /dev/sdb1
debugfs 1.41.14 (22-Dec-2010)
debugfs:  dump_extents check_2
Level Entries       Logical      Physical Length Flags
 0/ 1   1/  2     0 -  2039 32792             2040
 1/ 1   1/340     0 -     5  5771 -  5776       6
 1/ 1   2/340     6 -    11  5783 -  5788      6
<------- continue like wise till 340 ------------->
 1/ 1 340/340  2034 -  2039  9835 -  9840   6
 0/ 1   2/  2  2040 -  2051  5764              12
 1/ 1   1/  2  2040 -  2045  9847 -  9852     6
 1/ 1   2/  2  2046 -  2051  9859 -  9864     6

Step3: Punch hole at offset 4KB and length of hole is also 4KB
There is attached fallocate.c
[root@sputnik ashish]# ./fallacote.x86 /mnt/check_2
ret = 0
[root@sputnik ashish]#

Check extent tree:
[root@sputnik ashish]# debugfs  /dev/sdb1
debugfs 1.41.14 (22-Dec-2010)
debugfs:  dump_extents check_2
Level Entries       Logical      Physical Length Flags
 0/ 1   1/  3     0 -     5 32792                 6
 1/ 1   1/  2     0 -     1  5771 -  5772      2
 1/ 1   2/  2     2 -     5  5773 -  5776      4
 0/ 1   2/  3     6 -  2039  9871           2034
 1/ 1   1/339     6 -    11  5783 -  5788    6
<------- continue like wise till 339 ------------->
 1/ 1 339/339  2034 -  2039  9835 -  9840   6
 0/ 1   3/  3  2040 -  2051  5764             12
 1/ 1   1/  2  2040 -  2045  9847 -  9852    6
 1/ 1   2/  2  2046 -  2051  9859 -  9864    6

It is clearly visible that punching hole just split the first extent into 2
but failed to remove the required blocks.

Step4: To cross check, take diff of 2 files.
[root@sputnik ashish]# diff check_1 /mnt/check_2
[root@sputnik ashish]#

The 2 files are exactly same.

After applying this patch, correct results will be observed.

On Mon, Jul 2, 2012 at 2:42 PM, Lukáš Czerner <lczerner@redhat.com> wrote:
> On Mon, 2 Jul 2012, Ashish Sangwan wrote:
>
>> Date: Mon, 2 Jul 2012 11:03:26 +0530
>> From: Ashish Sangwan <ashishsangwan2@gmail.com>
>> To: sandeen@redhat.com, Lukáš Czerner <lczerner@redhat.com>,
>>     Ted Tso <tytso@mit.edu>
>> Cc: linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org,
>>     Namjae Jeon <linkinjeon@gmail.com>
>> Subject: Re: [PATCH v2] ext4: fix hole punch failure when depth is greater
>>     than 0
>>
>> Hi Ted, Lukas, Eric,
>>
>> Did any of you get a chance to look at it?
>
> I am sorry for the delay. I have been trying to reproduce this
> problem without any success so far. But is was on ppc64 machine, so
> I'll try that on x86_64 and then review the patch.
>
> Thanks!
> -Lukas
>
>>
>> On Fri, Jun 22, 2012 at 6:19 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>> > Hi. Eric.
>> >
>> > This is the patch for punch hole issue.
>> >
>> > Thanks.
>> >
>>

[-- Attachment #2: fallacote.c --]
[-- Type: text/x-csrc, Size: 650 bytes --]

#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
#define FALLOC_FL_PUNCH_HOLE		0x02
#define FALLOC_FL_KEEP_SIZE		0X01
int main(int argc, char **argv)
{
	int fd;
	int mode = 0; 
	int ret = 0;
        if (argc < 2) {
                _exit(1);
        }
	mode = (mode | FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE);  
	if ((fd = open(argv[1], O_RDWR)) < 0) {
              fprintf(stderr, "Cannot open file %s\n", argv[1]);
		_exit(1);
        }
	ret= fallocate(fd,mode,4096,4096);
	if( ret != 0)
		perror("Fallocate fail\n");
	printf("ret = %d\n",ret);
	close(fd);
	return 0;
}


[-- Attachment #3: fragment.sh --]
[-- Type: application/x-sh, Size: 279 bytes --]

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6750 bytes --]

So I've finally has some time to look at the patch and reproduce the
problem. Thanks for noticing the problem, the patch seems good,
though I have one question. Is the p_block setting really necessary
? I do not think so, but I might be missing something. Here is
updated patch I've tested, bellow.

Note: there are some indent problems in your patch, like for example
this:

+                       path[k].p_block =
+                       le16_to_cpu(path[k].p_hdr->eh_entries)+1;


Anyway, what do you think about the modification ?

Thanks!
-Lukas


diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index b3300eb..94bc1bd 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2570,7 +2570,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
 {
 	struct super_block *sb = inode->i_sb;
 	int depth = ext_depth(inode);
-	struct ext4_ext_path *path;
+	struct ext4_ext_path *path = NULL;
 	ext4_fsblk_t partial_cluster = 0;
 	handle_t *handle;
 	int i, err;
@@ -2606,8 +2606,12 @@ again:
 		}
 		depth = ext_depth(inode);
 		ex = path[depth].p_ext;
-		if (!ex)
+		if (!ex) {
+			ext4_ext_drop_refs(path);
+			kfree(path);
+			path = NULL;
 			goto cont;
+		}
 
 		ee_block = le32_to_cpu(ex->ee_block);
 
@@ -2637,8 +2641,6 @@ again:
 			if (err < 0)
 				goto out;
 		}
-		ext4_ext_drop_refs(path);
-		kfree(path);
 	}
 cont:
 
@@ -2646,20 +2648,26 @@ cont:
 	 * 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 (path)
+		i = depth;
+	else {
+		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;
+		if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
+			err = -EIO;
+			goto out;
+		}
+		i = 0;
 	}
-	i = err = 0;
+	err = 0;
 
 	while (i >= 0 && err == 0) {
 		if (i == depth) {

On Mon, 2 Jul 2012, Ashish Sangwan wrote:

> Date: Mon, 2 Jul 2012 16:51:43 +0530
> From: Ashish Sangwan <ashishsangwan2@gmail.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: sandeen@redhat.com, Ted Tso <tytso@mit.edu>, linux-kernel@vger.kernel.org,
>     linux-ext4@vger.kernel.org, Namjae Jeon <linkinjeon@gmail.com>
> Subject: Re: [PATCH v2] ext4: fix hole punch failure when depth is greater
>     than 0
> 
> I will try to elaborate more about the problem and steps to reproduce:
> Linux version 3.4.0 on X86 box.
> 
> Step1:
> First create a small partition as it would be easy to fragment quickly.
> The main intent for fragmenting the partition is to create a file with
> at least 2 extent indexes.
> You can also choose some other method to create such a file.
> I created 200Mb partition and while formating used blocksize as 4KB
> and used attached script to fragment.
> This script fragments the partition such that each extent is exactly
> 6FS blocks OR 24KB in length.
> Linux#> ./fragment.sh /mnt/
> 6+0 records in
> 6+0 records out
> 24576 bytes (24.0KB) copied, 0.087997 seconds, 272.7KB/s
> cp: write error: No space left on device
> Partition filled
> rm: can't remove '/mnt//file1.7564': No such file or directory
> fragmented partition /mnt/ with 24KB files
> 
> Step2:
> Create a file with 342 extents i.e 2 extent indexes
> [root@sputnik ashish]# dd if=linux-3.4.tar.bz2 of=check_1 bs=24576 count=342
> 342+0 records in
> 342+0 records out
> 8404992 bytes (8.4 MB) copied, 0.0913289 s, 92.0 MB/s
> [root@sputnik ashish]# cp check_1 /mnt/check_2
> [root@sputnik ashish]# debugfs  /dev/sdb1
> debugfs 1.41.14 (22-Dec-2010)
> debugfs:  dump_extents check_2
> Level Entries       Logical      Physical Length Flags
>  0/ 1   1/  2     0 -  2039 32792             2040
>  1/ 1   1/340     0 -     5  5771 -  5776       6
>  1/ 1   2/340     6 -    11  5783 -  5788      6
> <------- continue like wise till 340 ------------->
>  1/ 1 340/340  2034 -  2039  9835 -  9840   6
>  0/ 1   2/  2  2040 -  2051  5764              12
>  1/ 1   1/  2  2040 -  2045  9847 -  9852     6
>  1/ 1   2/  2  2046 -  2051  9859 -  9864     6
> 
> Step3: Punch hole at offset 4KB and length of hole is also 4KB
> There is attached fallocate.c
> [root@sputnik ashish]# ./fallacote.x86 /mnt/check_2
> ret = 0
> [root@sputnik ashish]#
> 
> Check extent tree:
> [root@sputnik ashish]# debugfs  /dev/sdb1
> debugfs 1.41.14 (22-Dec-2010)
> debugfs:  dump_extents check_2
> Level Entries       Logical      Physical Length Flags
>  0/ 1   1/  3     0 -     5 32792                 6
>  1/ 1   1/  2     0 -     1  5771 -  5772      2
>  1/ 1   2/  2     2 -     5  5773 -  5776      4
>  0/ 1   2/  3     6 -  2039  9871           2034
>  1/ 1   1/339     6 -    11  5783 -  5788    6
> <------- continue like wise till 339 ------------->
>  1/ 1 339/339  2034 -  2039  9835 -  9840   6
>  0/ 1   3/  3  2040 -  2051  5764             12
>  1/ 1   1/  2  2040 -  2045  9847 -  9852    6
>  1/ 1   2/  2  2046 -  2051  9859 -  9864    6
> 
> It is clearly visible that punching hole just split the first extent into 2
> but failed to remove the required blocks.
> 
> Step4: To cross check, take diff of 2 files.
> [root@sputnik ashish]# diff check_1 /mnt/check_2
> [root@sputnik ashish]#
> 
> The 2 files are exactly same.
> 
> After applying this patch, correct results will be observed.
> 
> On Mon, Jul 2, 2012 at 2:42 PM, Lukáš Czerner <lczerner@redhat.com> wrote:
> > On Mon, 2 Jul 2012, Ashish Sangwan wrote:
> >
> >> Date: Mon, 2 Jul 2012 11:03:26 +0530
> >> From: Ashish Sangwan <ashishsangwan2@gmail.com>
> >> To: sandeen@redhat.com, Lukáš Czerner <lczerner@redhat.com>,
> >>     Ted Tso <tytso@mit.edu>
> >> Cc: linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org,
> >>     Namjae Jeon <linkinjeon@gmail.com>
> >> Subject: Re: [PATCH v2] ext4: fix hole punch failure when depth is greater
> >>     than 0
> >>
> >> Hi Ted, Lukas, Eric,
> >>
> >> Did any of you get a chance to look at it?
> >
> > I am sorry for the delay. I have been trying to reproduce this
> > problem without any success so far. But is was on ppc64 machine, so
> > I'll try that on x86_64 and then review the patch.
> >
> > Thanks!
> > -Lukas
> >
> >>
> >> On Fri, Jun 22, 2012 at 6:19 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
> >> > Hi. Eric.
> >> >
> >> > This is the patch for punch hole issue.
> >> >
> >> > Thanks.
> >> >
> >>
> 

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Ashish Sangwan]

On Wed, Jul 4, 2012 at 11:03 PM, Lukáš Czerner <lczerner@redhat.com> wrote:
> So I've finally has some time to look at the patch and reproduce the
> problem. Thanks for noticing the problem, the patch seems good,
> though I have one question. Is the p_block setting really necessary
> ? I do not think so, but I might be missing something. Here is
> updated patch I've tested, bellow.
AFAICS, p_block setting is necessary. As mentioned in the patch description,
whether to continue removing extents or not is decided by the return value
of function ext4_ext_more_to_rm() which checks 2 conditions:
a) if there are no more indexes to process.
b) if the number of entries are decreased in the header of "depth -1".
The p_block setting is important for condition b).

In function ext4_ext_more_to_rm, there is this second check:
if (le16_to_cpu(path->p_hdr->eh_entries) == path->p_block)
    return 0;
If the value of p_block would not be correct, the above mentioned
condition could become
true while there are still blocks left to be removed.
>
> Note: there are some indent problems in your patch, like for example
> this:
>
> +                       path[k].p_block =
> +                       le16_to_cpu(path[k].p_hdr->eh_entries)+1;
>
>
Before submitting the patch, I run checkpatch.pl with --strict option.
It did'nt show any error or warning. Should I re-submit
the patch with an extra tab before the second line? The call is yours.

> Anyway, what do you think about the modification ?
>
Also there is 1 modification missing from your patch.
    ext4_ext_drop_refs(path);
        kfree(path);
+       path = NULL;
        if (err == -EAGAIN)
                goto again;
If path is not set to NULL, it will crash in xfstest #13. Ted has
already reported it.

Thanks,
Ashish

> Thanks!
> -Lukas
>
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index b3300eb..94bc1bd 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2570,7 +2570,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
>  {
>         struct super_block *sb = inode->i_sb;
>         int depth = ext_depth(inode);
> -       struct ext4_ext_path *path;
> +       struct ext4_ext_path *path = NULL;
>         ext4_fsblk_t partial_cluster = 0;
>         handle_t *handle;
>         int i, err;
> @@ -2606,8 +2606,12 @@ again:
>                 }
>                 depth = ext_depth(inode);
>                 ex = path[depth].p_ext;
> -               if (!ex)
> +               if (!ex) {
> +                       ext4_ext_drop_refs(path);
> +                       kfree(path);
> +                       path = NULL;
>                         goto cont;
> +               }
>
>                 ee_block = le32_to_cpu(ex->ee_block);
>
> @@ -2637,8 +2641,6 @@ again:
>                         if (err < 0)
>                                 goto out;
>                 }
> -               ext4_ext_drop_refs(path);
> -               kfree(path);
>         }
>  cont:
>
> @@ -2646,20 +2648,26 @@ cont:
>          * 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 (path)
> +               i = depth;
> +       else {
> +               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;
> +               if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
> +                       err = -EIO;
> +                       goto out;
> +               }
> +               i = 0;
>         }
> -       i = err = 0;
> +       err = 0;
>
>         while (i >= 0 && err == 0) {
>                 if (i == depth) {
>
> On Mon, 2 Jul 2012, Ashish Sangwan wrote:
>

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Ashish Sangwan]

On Wed, Jul 4, 2012 at 11:03 PM, Lukáš Czerner <lczerner@redhat.com> wrote:
> So I've finally has some time to look at the patch and reproduce the
> problem. Thanks for noticing the problem, the patch seems good,
> though I have one question. Is the p_block setting really necessary
> ? I do not think so, but I might be missing something. Here is
> updated patch I've tested, bellow.
AFAICS, p_block setting is necessary. As mentioned in the patch description,
whether to continue removing extents or not is decided by the return value
of function ext4_ext_more_to_rm() which checks 2 conditions:
a) if there are no more indexes to process.
b) if the number of entries are decreased in the header of "depth -1".
The p_block setting is important for condition b).

In function ext4_ext_more_to_rm, there is this second check:
if (le16_to_cpu(path->p_hdr->eh_entries) == path->p_block)
    return 0;
If the value of p_block would not be correct, the above mentioned
condition could become
true while there are still blocks left to be removed.
>
> Note: there are some indent problems in your patch, like for example
> this:
>
> +                       path[k].p_block =
> +                       le16_to_cpu(path[k].p_hdr->eh_entries)+1;
>
>
Before submitting the patch, I run checkpatch.pl with --strict option.
It did'nt show any error or warning. Should I re-submit
the patch with an extra tab before the second line? The call is yours.

> Anyway, what do you think about the modification ?
>
Also there is 1 modification missing from your patch.
    ext4_ext_drop_refs(path);
        kfree(path);
+       path = NULL;
        if (err == -EAGAIN)
                goto again;
If path is not set to NULL, it will crash in xfstest #13. Ted has
already reported it.

Thanks,
Ashish

> Thanks!
> -Lukas
>
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index b3300eb..94bc1bd 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2570,7 +2570,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
>  {
>         struct super_block *sb = inode->i_sb;
>         int depth = ext_depth(inode);
> -       struct ext4_ext_path *path;
> +       struct ext4_ext_path *path = NULL;
>         ext4_fsblk_t partial_cluster = 0;
>         handle_t *handle;
>         int i, err;
> @@ -2606,8 +2606,12 @@ again:
>                 }
>                 depth = ext_depth(inode);
>                 ex = path[depth].p_ext;
> -               if (!ex)
> +               if (!ex) {
> +                       ext4_ext_drop_refs(path);
> +                       kfree(path);
> +                       path = NULL;
>                         goto cont;
> +               }
>
>                 ee_block = le32_to_cpu(ex->ee_block);
>
> @@ -2637,8 +2641,6 @@ again:
>                         if (err < 0)
>                                 goto out;
>                 }
> -               ext4_ext_drop_refs(path);
> -               kfree(path);
>         }
>  cont:
>
> @@ -2646,20 +2648,26 @@ cont:
>          * 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 (path)
> +               i = depth;
> +       else {
> +               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;
> +               if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
> +                       err = -EIO;
> +                       goto out;
> +               }
> +               i = 0;
>         }
> -       i = err = 0;
> +       err = 0;
>
>         while (i >= 0 && err == 0) {
>                 if (i == depth) {
>
> On Mon, 2 Jul 2012, Ashish Sangwan wrote:
>
--
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 v2] ext4: fix hole punch failure when depth is greater than 0

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6024 bytes --]

On Thu, 5 Jul 2012, Ashish Sangwan wrote:

> Date: Thu, 5 Jul 2012 15:22:04 +0530
> From: Ashish Sangwan <ashishsangwan2@gmail.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: sandeen@redhat.com, Ted Tso <tytso@mit.edu>, linux-kernel@vger.kernel.org,
>     linux-ext4@vger.kernel.org, Namjae Jeon <linkinjeon@gmail.com>
> Subject: Re: [PATCH v2] ext4: fix hole punch failure when depth is greater
>     than 0
> 
> On Wed, Jul 4, 2012 at 11:03 PM, Lukáš Czerner <lczerner@redhat.com> wrote:
> > So I've finally has some time to look at the patch and reproduce the
> > problem. Thanks for noticing the problem, the patch seems good,
> > though I have one question. Is the p_block setting really necessary
> > ? I do not think so, but I might be missing something. Here is
> > updated patch I've tested, bellow.
> AFAICS, p_block setting is necessary. As mentioned in the patch description,
> whether to continue removing extents or not is decided by the return value
> of function ext4_ext_more_to_rm() which checks 2 conditions:
> a) if there are no more indexes to process.
> b) if the number of entries are decreased in the header of "depth -1".
> The p_block setting is important for condition b).
> 
> In function ext4_ext_more_to_rm, there is this second check:
> if (le16_to_cpu(path->p_hdr->eh_entries) == path->p_block)
>     return 0;
> If the value of p_block would not be correct, the above mentioned
> condition could become
> true while there are still blocks left to be removed.

Ok, the code is not very clear, but now I can see it. p_block is
actually misused here to store the number of indexes in the current
node while diving into the tree. Then on the way up, we are checking
that to see if the eh_entries changed or not (which is indicating
that something has been freed deeper in the tree).

That said, it makes sense to set it before the loop itself because
we are actually skipping the path construction while diving into
the tree since patch is already initialized and we're starting
walking back from 'depth' up in this case. So the patch seems fine.
Thanks for catching it and fixing it.

You can add

Reviewed-by: Lukas Czerner <lczerner@redhat.com>


> >
> > Note: there are some indent problems in your patch, like for example
> > this:
> >
> > +                       path[k].p_block =
> > +                       le16_to_cpu(path[k].p_hdr->eh_entries)+1;
> >
> >
> Before submitting the patch, I run checkpatch.pl with --strict option.
> It did'nt show any error or warning. Should I re-submit
> the patch with an extra tab before the second line? The call is yours.

checkpatch.pl does not catch everything. Just look at how wrapping
of long lines is done in the code, there are plenty of examples.

> 
> > Anyway, what do you think about the modification ?
> >
> Also there is 1 modification missing from your patch.
>     ext4_ext_drop_refs(path);
>         kfree(path);
> +       path = NULL;
>         if (err == -EAGAIN)
>                 goto again;
> If path is not set to NULL, it will crash in xfstest #13. Ted has
> already reported it.

Right, I've probably used the old patch as an example.

Thanks!
-Lukas

> 
> Thanks,
> Ashish
> 
> > Thanks!
> > -Lukas
> >
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index b3300eb..94bc1bd 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -2570,7 +2570,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
> >  {
> >         struct super_block *sb = inode->i_sb;
> >         int depth = ext_depth(inode);
> > -       struct ext4_ext_path *path;
> > +       struct ext4_ext_path *path = NULL;
> >         ext4_fsblk_t partial_cluster = 0;
> >         handle_t *handle;
> >         int i, err;
> > @@ -2606,8 +2606,12 @@ again:
> >                 }
> >                 depth = ext_depth(inode);
> >                 ex = path[depth].p_ext;
> > -               if (!ex)
> > +               if (!ex) {
> > +                       ext4_ext_drop_refs(path);
> > +                       kfree(path);
> > +                       path = NULL;
> >                         goto cont;
> > +               }
> >
> >                 ee_block = le32_to_cpu(ex->ee_block);
> >
> > @@ -2637,8 +2641,6 @@ again:
> >                         if (err < 0)
> >                                 goto out;
> >                 }
> > -               ext4_ext_drop_refs(path);
> > -               kfree(path);
> >         }
> >  cont:
> >
> > @@ -2646,20 +2648,26 @@ cont:
> >          * 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 (path)
> > +               i = depth;
> > +       else {
> > +               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;
> > +               if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
> > +                       err = -EIO;
> > +                       goto out;
> > +               }
> > +               i = 0;
> >         }
> > -       i = err = 0;
> > +       err = 0;
> >
> >         while (i >= 0 && err == 0) {
> >                 if (i == depth) {
> >
> > On Mon, 2 Jul 2012, Ashish Sangwan wrote:
> >
> 

Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0

[Ashish Sangwan]

> Ok, the code is not very clear, but now I can see it. p_block is
> actually misused here to store the number of indexes in the current
> node while diving into the tree. Then on the way up, we are checking
> that to see if the eh_entries changed or not (which is indicating
> that something has been freed deeper in the tree).
True, p_block is misused. We tried to fix the problem with
minimum code change.
>
> That said, it makes sense to set it before the loop itself because
> we are actually skipping the path construction while diving into
> the tree since patch is already initialized and we're starting
> walking back from 'depth' up in this case. So the patch seems fine.
> Thanks for catching it and fixing it.
>
> You can add
>
> Reviewed-by: Lukas Czerner <lczerner@redhat.com>
>
Thanks for your review.
>
>> >
>> > Note: there are some indent problems in your patch, like for example
>> > this:
>> >
>> > +                       path[k].p_block =
>> > +                       le16_to_cpu(path[k].p_hdr->eh_entries)+1;
>> >
>> >
>> Before submitting the patch, I run checkpatch.pl with --strict option.
>> It did'nt show any error or warning. Should I re-submit
>> the patch with an extra tab before the second line? The call is yours.
>
> checkpatch.pl does not catch everything. Just look at how wrapping
> of long lines is done in the code, there are plenty of examples.
>
True again. I will resend patch with proper indentation.
>>
>> > Anyway, what do you think about the modification ?
>> >
>> Also there is 1 modification missing from your patch.
>>     ext4_ext_drop_refs(path);
>>         kfree(path);
>> +       path = NULL;
>>         if (err == -EAGAIN)
>>                 goto again;
>> If path is not set to NULL, it will crash in xfstest #13. Ted has
>> already reported it.
>
> Right, I've probably used the old patch as an example.
>
> Thanks!
> -Lukas
>
Thanks,
Ashish