Re: [PATCH] ext4: fix extent tree corruption that incurred by hole punch [V2]

[Forrest Liu <forrestl@synology.com>]

@Ashish
   I have modified the patch with your suggestion, could you help to review.

@Ted
   I wrote a script to verify this problem.
   This script needs tst_extents and fallocate executables from e2fsprogs, and
fallocate needs your patch with bug fix to do hole punch.

   Steps to run the script
1) assign variable blkdev to path of block device that filesystem mount on
2) cd to mount point
3) run script

Thanks,
Forrest

# verify hole punch bug
#
blkdev=/dev/sda1
file=punch_test
cmdfile=cmds
filesize=`expr 1024 \* 1024 \* 1024`
blksize=`tst_extents -R "stats" $blkdev | grep "Block size" | cut -d
':'  -f 2 | tr -d ' '`
maxlblks=`expr $filesize \/ $blksize`

rm $file
touch $file
fallocate -l $filesize $file
sync

inode=`tst_extents -R "stat ${file}" $blkdev | grep Inode | cut -d ' ' -f 2`
echo "Inode number: $inode"

# punch every even blocks
echo "Punch out every even blocks"
i=0
while [ "${i}" -lt "${maxlblks}" ]
do
	offset=`expr $i \* $blksize`
	fallocate -p -o $offset -l $blksize $file
	i=$(($i+2))
done

# get lblk from root->ns->down
echo "inode <$inode>" > cmds
echo "root" >> cmds
echo "ns" >> cmds

a=`tst_extents -f $cmdfile $blkdev`
echo "down" >> cmds
b=`tst_extents -f $cmdfile $blkdev`

# get output form command "down"
c=${b:${#a}}
echo "Out put from command \"down\""
echo $c

a=`echo ${c#*lblk} | cut -d ',' -f 1`
lblk_low=`echo $a | cut -d '-' -f 1`
lblk_hi=`echo $a | cut -d '-' -f 3`

echo ""
echo "Punch out blocks $lblk_hi, ..., $lblk_low"

i=$lblk_hi
while [ $i -ge $lblk_low ]
do
	offset=`expr $i \* $blksize`
	fallocate -p -o $offset -l $blksize $file
	i=$(($i-1))
done

# verify logical start value is correct or not
echo "inode <$inode>" > cmds
echo "root" >> cmds
a=`tst_extents -f $cmdfile $blkdev`
echo "ns" >> cmds
b=`tst_extents -f $cmdfile $blkdev`

c=${b:${#a}}
c=`echo ${c#*lblk} | cut -d ',' -f 1`
lblk_start0=`echo $c | cut -d '-' -f 1`

echo "down" >> cmds
c=`tst_extents -f $cmdfile $blkdev`
c=${c:${#b}}
c=`echo ${c#*lblk} | cut -d ',' -f 1`
lblk_start1=`echo $c | cut -d '-' -f 1`

if [ $lblk_start0 -eq $lblk_start1 ]
then
	echo "logical start valie is consistency:$lblk_start0,$lblk_start1"
else
	echo "logical start valie is not consistency:$lblk_start0,$lblk_start1"
fi



diff --git a/contrib/fallocate.c b/contrib/fallocate.c
index 0e8319f..c1c08e1 100644
--- a/contrib/fallocate.c
+++ b/contrib/fallocate.c
@@ -35,10 +35,11 @@

 // #include <linux/falloc.h>
 #define FALLOC_FL_KEEP_SIZE	0x01
+#define FALLOC_FL_PUNCH_HOLE	0x02 /* de-allocates range */

 void usage(void)
 {
-	printf("Usage: fallocate [-nt] [-o offset] -l length filename\n");
+	printf("Usage: fallocate [-npt] [-o offset] -l length filename\n");
 	exit(EXIT_FAILURE);
 }

@@ -56,6 +57,7 @@ cvtnum(char *s)
 	char		*sp;
 	int		c;

+
 	i = strtoll(s, &sp, 0);
 	if (i == 0 && sp == s)
 		return -1LL;
@@ -94,12 +96,18 @@ int main(int argc, char **argv)
 	int	error;
 	int	tflag = 0;

-	while ((opt = getopt(argc, argv, "nl:ot")) != -1) {
+	while ((opt = getopt(argc, argv, "npl:o:t")) != -1) {
 		switch(opt) {
 		case 'n':
 			/* do not change filesize */
 			falloc_mode = FALLOC_FL_KEEP_SIZE;
 			break;
+		case 'p':
+			/* punch mode */
+			falloc_mode = (FALLOC_FL_PUNCH_HOLE |
+				FALLOC_FL_KEEP_SIZE);
+			break;
+
 		case 'l':
 			length = cvtnum(optarg);
 			break;

2012/12/13 Forrest Liu <forrestl@synology.com>:
> When depth of extent tree is greater than 1, logical start value
> of interior node didn't updated correctly in ext4_ext_rm_idx.
>
> Signed-off-by: Forrest Liu <forrestl@synology.com>
> ---
>  fs/ext4/extents.c |   22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index d3dd618..496952e 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2190,13 +2190,14 @@ errout:
>   * removes index from the index block.
>   */
>  static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
> -                       struct ext4_ext_path *path)
> +                       struct ext4_ext_path *path, int depth)
>  {
>         int err;
>         ext4_fsblk_t leaf;
>
>         /* free index block */
> -       path--;
> +       depth--;
> +       path = path + depth;
>         leaf = ext4_idx_pblock(path->p_idx);
>         if (unlikely(path->p_hdr->eh_entries == 0)) {
>                 EXT4_ERROR_INODE(inode, "path->p_hdr->eh_entries == 0");
> @@ -2221,6 +2222,19 @@ static int ext4_ext_rm_idx(handle_t *handle,
> struct inode *inode,
>
>         ext4_free_blocks(handle, inode, NULL, leaf, 1,
>                          EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET);
> +
> +       while (--depth >= 0) {
> +               if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))
> +                       break;
> +               path--;
> +               err = ext4_ext_get_access(handle, inode, path);
> +               if (err)
> +                       break;
> +               path->p_idx->ei_block = (path+1)->p_idx->ei_block;
> +               err = ext4_ext_dirty(handle, inode, path);
> +               if (err)
> +                       break;
> +       }
>         return err;
>  }
>
> @@ -2557,7 +2571,7 @@ ext4_ext_rm_leaf(handle_t *handle, struct inode *inode,
>         /* if this leaf is free, then we should
>          * remove it from index block above */
>         if (err == 0 && eh->eh_entries == 0 && path[depth].p_bh != NULL)
> -               err = ext4_ext_rm_idx(handle, inode, path + depth);
> +               err = ext4_ext_rm_idx(handle, inode, path, depth);
>
>  out:
>         return err;
> @@ -2760,7 +2774,7 @@ again:
>                                 /* index is empty, remove it;
>                                  * handle must be already prepared by the
>                                  * truncatei_leaf() */
> -                               err = ext4_ext_rm_idx(handle, inode, path + i);
> +                               err = ext4_ext_rm_idx(handle, inode, path, i);
>                         }
>                         /* root level has p_bh == NULL, brelse() eats this */
>                         brelse(path[i].p_bh);
> --
> 1.7.9.5