From: Forrest Liu Subject: Re: [PATCH] ext4: fix extent tree corruption that incurred by hole punch [V2] Date: Fri, 14 Dec 2012 00:17:41 +0800 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Cc: ext4 development , "Synology/Forrest Liu" To: "Theodore Ts'o" , Ashish Sangwan Return-path: Received: from mail-ia0-f174.google.com ([209.85.210.174]:35686 "EHLO mail-ia0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756166Ab2LMQRm (ORCPT ); Thu, 13 Dec 2012 11:17:42 -0500 Received: by mail-ia0-f174.google.com with SMTP id y25so2114116iay.19 for ; Thu, 13 Dec 2012 08:17:41 -0800 (PST) In-Reply-To: Sender: linux-ext4-owner@vger.kernel.org List-ID: Hi Ted, Your patch for fallocate is no problem, my mistake. Sorry~ -Forrest 2012/12/14 Forrest Liu : > @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 > #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 : >> 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 >> --- >> 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