xfs
[Top] [All Lists]

Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.

To: josef@xxxxxxxxxx
Subject: Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
From: Yongqiang Yang <xiaoqiangnk@xxxxxxxxx>
Date: Thu, 19 May 2011 15:24:01 +0800
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs@xxxxxxxxxxx, linux-ext4@xxxxxxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=iuYGPG5hNSl1g6DBt6vAVHabEIfSsIZzkgjsE4mI/NQ=; b=v3DXAGK50ZvLfYP27r1vwynYfEFPhqb/nvcdCKhQLATfOcRJ4RsYSyJ5ezisbLAkD+ xwn/shqG4wAQXS1owq53vjjA3QjNh1l2YC/gfjPJqmMo/6PpeuXVD0XEMzZ7JOshmI7n ++rMrN0PpRgbGfl3jl/cJwYQqfxoRf/VLOQ6s=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=mCgdgm+xhTNGX1R8t7yteEbHLu9J4ob7dCeLbuBqHGORNxLJyiwrEZtan9O6D1pLP/ aUWAfRzN5I3AU7Git8EPB0C95AVmrjpnTWlbZeEj2Ha0G+u7MAAxGrRYaQ0Y4UOrMJR5 HAkokHyeSYBTTed52amHeSS2oNHWw3xs5vHQU=
In-reply-to: <1305789665-32743-1-git-send-email-xiaoqiangnk@xxxxxxxxx>
References: <1305789665-32743-1-git-send-email-xiaoqiangnk@xxxxxxxxx>
Hi Josef,

I updated the patch, if you think it is ok now and 'Reviewed-by: Josef
<josef@xxxxxxxxxx>' can be added, please throw a word to Eric.

Yongqiang.

On Thu, May 19, 2011 at 3:21 PM, Yongqiang Yang <xiaoqiangnk@xxxxxxxxx> wrote:
> Due to my carelessness, I induced a ugly patch to ext4's fiemap, but
> 225 could not find it.  So I looked into the 225 and could not figure out
> logic in compare_map_and_fiemap(), which seemed to mix extents with
> blocks.  Then I made 225 compare map and fiemap at each block, the new
> 225 can find another bug in ext4 with 1k block.
>
> The new 225 works well on ext3, ext4 and xfs with both 1K and 4K block.
>
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@xxxxxxxxx>
> ---
> v1->v2:
> Josef <josef@xxxxxxxxxx> reviewed the v1 patch and pointed out the bug which
> made xfs not work and several coding styles.
>
> According to reply from Josef, I
>  fixed the bug which added ';' after an if statement.
>  removed the trailing whitespace.
>
> Apart from things above, I
>  made check_weird_fs_hole() verify bytes read by pread().
>
>  src/fiemap-tester.c |  251 
> ++++++++++++++++++++++++++++-----------------------
>  1 files changed, 140 insertions(+), 111 deletions(-)
>
> diff --git a/src/fiemap-tester.c b/src/fiemap-tester.c
> index 1663f84..319a9bb 100644
> --- a/src/fiemap-tester.c
> +++ b/src/fiemap-tester.c
> @@ -14,6 +14,9 @@
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, write the Free Software Foundation,
>  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + * Compare map and fiemap at each block,
> + * Yongqiang Yang <xiaoqiangnk@xxxxxxxxx>, 2011
>  */
>
>  #include <stdio.h>
> @@ -57,7 +60,7 @@ generate_file_mapping(int blocks, int prealloc)
>        int num_types = 2, cur_block = 0;
>        int i = 0;
>
> -       map = malloc(sizeof(char) * blocks);
> +       map = malloc(sizeof(char) * (blocks + 1));
>        if (!map)
>                return NULL;
>
> @@ -81,6 +84,7 @@ generate_file_mapping(int blocks, int prealloc)
>                cur_block++;
>        }
>
> +       map[blocks] = 0;
>        return map;
>  }
>
> @@ -247,55 +251,36 @@ check_flags(struct fiemap *fiemap, int blocksize)
>  }
>
>  static int
> -check_data(struct fiemap *fiemap, __u64 logical_offset, int blocksize,
> +check_data(struct fiemap_extent *extent ,  __u64 logical_offset, int 
> blocksize,
>           int last, int prealloc)
>  {
> -       struct fiemap_extent *extent;
> -       __u64 orig_offset = logical_offset;
> -       int c, found = 0;
> -
> -       for (c = 0; c < fiemap->fm_mapped_extents; c++) {
> -               __u64 start, end;
> -               extent = &fiemap->fm_extents[c];
> -
> -               start = extent->fe_logical;
> -               end = extent->fe_logical + extent->fe_length;
> -
> -               if (logical_offset > end)
> -                       continue;
> -
> -               if (logical_offset + blocksize < start)
> -                       break;
> -
> -               if (logical_offset >= start &&
> -                   logical_offset < end) {
> -                       if (prealloc &&
> -                           !(extent->fe_flags & FIEMAP_EXTENT_UNWRITTEN)) {
> -                               printf("ERROR: preallocated extent is not "
> -                                      "marked with FIEMAP_EXTENT_UNWRITTEN: "
> -                                      "%llu\n",
> -                                      (unsigned long long)
> -                                      (start / blocksize));
> -                               return -1;
> -                       }
> -
> -                       if (logical_offset + blocksize > end) {
> -                               logical_offset = end+1;
> -                               continue;
> -                       } else {
> -                               found = 1;
> -                               break;
> -                       }
> +       int found = 0;
> +       __u64 start, end;
> +
> +       start = extent->fe_logical;
> +       end = extent->fe_logical + extent->fe_length;
> +
> +       if (logical_offset >= start &&
> +           logical_offset < end) {
> +               if (prealloc &&
> +                   !(extent->fe_flags & FIEMAP_EXTENT_UNWRITTEN)) {
> +                       printf("ERROR: preallocated extent is not "
> +                              "marked with FIEMAP_EXTENT_UNWRITTEN: "
> +                              "%llu\n",
> +                              (unsigned long long)
> +                              (start / blocksize));
> +                       return -1;
>                }
> +               found = 1;
>        }
>
>        if (!found) {
>                printf("ERROR: couldn't find extent at %llu\n",
> -                      (unsigned long long)(orig_offset / blocksize));
> +                      (unsigned long long)(logical_offset / blocksize));
>        } else if (last &&
> -                  !(fiemap->fm_extents[c].fe_flags & FIEMAP_EXTENT_LAST)) {
> +                  !(extent->fe_flags & FIEMAP_EXTENT_LAST)) {
>                printf("ERROR: last extent not marked as last: %llu\n",
> -                      (unsigned long long)(orig_offset / blocksize));
> +                      (unsigned long long)(logical_offset / blocksize));
>                found = 0;
>        }
>
> @@ -306,7 +291,7 @@ static int
>  check_weird_fs_hole(int fd, __u64 logical_offset, int blocksize)
>  {
>        static int warning_printed = 0;
> -       int block, i;
> +       int block, i, count;
>        size_t buf_len = sizeof(char) * blocksize;
>        char *buf;
>
> @@ -330,15 +315,16 @@ check_weird_fs_hole(int fd, __u64 logical_offset, int 
> blocksize)
>                return -1;
>        }
>
> -       if (pread(fd, buf, buf_len, (off_t)logical_offset) < 0) {
> +       count = pread(fd, buf, buf_len, (off_t)logical_offset);
> +       if (count < 0) {
>                perror("Error reading from file");
>                free(buf);
>                return -1;
>        }
>
> -       for (i = 0; i < buf_len; i++) {
> +       for (i = 0; i < count; i++) {
>                if (buf[i] != 0) {
> -                       printf("ERROR: FIEMAP claimed there was data (%c) at "
> +                       printf("ERROR: FIEMAP claimed there was data (%x) at "
>                               "block %llu that should have been a hole, and "
>                               "FIBMAP confirmed that it was allocated, but "
>                               "it should be filled with 0's, but it was not "
> @@ -370,35 +356,25 @@ check_weird_fs_hole(int fd, __u64 logical_offset, int 
> blocksize)
>  }
>
>  static int
> -check_hole(struct fiemap *fiemap, int fd, __u64 logical_offset, int 
> blocksize)
> +check_hole(struct fiemap_extent *extent, int fd, __u64 logical_offset,
> +          int blocksize)
>  {
> -       struct fiemap_extent *extent;
> -       int c;
> +       __u64 start, end;
>
> -       for (c = 0; c < fiemap->fm_mapped_extents; c++) {
> -               __u64 start, end;
> -               extent = &fiemap->fm_extents[c];
> -
> -               start = extent->fe_logical;
> -               end = extent->fe_logical + extent->fe_length;
> -
> -               if (logical_offset > end)
> -                       continue;
> -               if (logical_offset + blocksize < start)
> -                       break;
> +       start = extent->fe_logical;
> +       end = extent->fe_logical + extent->fe_length;
>
> -               if (logical_offset >= start &&
> -                   logical_offset < end) {
> +       if (logical_offset >= start &&
> +           logical_offset < end) {
>
> -                       if (check_weird_fs_hole(fd, logical_offset,
> -                                               blocksize) == 0)
> -                               break;
> +               if (check_weird_fs_hole(fd, logical_offset,
> +                                       blocksize) == 0)
> +                       return 0;
>
> -                       printf("ERROR: found an allocated extent where a hole 
> "
> -                              "should be: %llu\n",
> -                              (unsigned long long)(start / blocksize));
> -                       return -1;
> -               }
> +               printf("ERROR: found an allocated extent where a hole "
> +                      "should be: %llu\n",
> +                      (unsigned long long)(logical_offset / blocksize));
> +               return -1;
>        }
>
>        return 0;
> @@ -423,9 +399,11 @@ compare_fiemap_and_map(int fd, char *map, int blocks, 
> int blocksize, int syncfil
>  {
>        struct fiemap *fiemap;
>        char *fiebuf;
> -       int blocks_to_map, ret, cur_extent = 0, last_data;
> +       int blocks_to_map, ret, last_data = -1;
>        __u64 map_start, map_length;
>        int i, c;
> +       int cur_block = 0;
> +       int last_found = 0;
>
>        if (query_fiemap_count(fd, blocks, blocksize) < 0)
>                return -1;
> @@ -451,8 +429,11 @@ compare_fiemap_and_map(int fd, char *map, int blocks, 
> int blocksize, int syncfil
>        fiemap->fm_extent_count = blocks_to_map;
>        fiemap->fm_mapped_extents = 0;
>
> +       /* check fiemap by looking at each block. */
>        do {
> -               fiemap->fm_start = map_start;
> +               int nr_extents;
> +
> +               fiemap->fm_start = cur_block * blocksize;
>                fiemap->fm_length = map_length;
>
>                ret = ioctl(fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> @@ -465,45 +446,93 @@ compare_fiemap_and_map(int fd, char *map, int blocks, 
> int blocksize, int syncfil
>                if (check_flags(fiemap, blocksize))
>                        goto error;
>
> -               for (i = cur_extent, c = 1; i < blocks; i++, c++) {
> -                       __u64 logical_offset = i * blocksize;
> -
> -                       if (c > fiemap->fm_mapped_extents) {
> -                               i++;
> -                               break;
> +               nr_extents = fiemap->fm_mapped_extents;
> +               if (nr_extents == 0) {
> +                       int block = cur_block + (map_length - 1) / blocksize;
> +                       for (; cur_block <= block &&
> +                              cur_block < blocks; cur_block++) {
> +                               /* check hole */
> +                               if (map[cur_block] != 'H') {
> +                                       printf("ERROR: map[%d] should not be "
> +                                              "a hole\n", cur_block);
> +                                       goto error;
> +                               }
>                        }
> +                       continue;
> +               }
>
> -                       switch (map[i]) {
> -                       case 'D':
> -                               if (check_data(fiemap, logical_offset,
> -                                              blocksize, last_data == i, 0))
> +               for (c = 0; c < nr_extents; c++) {
> +                       __u64 offset;
> +                       int block;
> +                       struct fiemap_extent *extent;
> +
> +
> +                       extent = &fiemap->fm_extents[c];
> +                       offset = extent->fe_logical;
> +                       block = offset / blocksize;
> +
> +                       /* check hole. */
> +                       for (; cur_block < block && cur_block < blocks;
> +                            cur_block++) {
> +                               if (map[cur_block] != 'H') {
> +                                       printf("ERROR: map[%d] should not be "
> +                                              "a hole\n", cur_block);
>                                        goto error;
> -                               break;
> -                       case 'H':
> -                               if (check_hole(fiemap, fd, logical_offset,
> -                                              blocksize))
> +                               }
> +                       }
> +
> +                       offset = extent->fe_logical + extent->fe_length;
> +                       block = offset / blocksize;
> +                       /* check data */
> +                       for (; cur_block < block && cur_block < blocks;
> +                            cur_block++) {
> +                               int last;
> +                               offset = (__u64)cur_block * blocksize;
> +                               last = (cur_block == last_data);
> +                               last_found = last_found ? last_found : last;
> +                               switch (map[cur_block]) {
> +                               case 'D':
> +                                       if (check_data(extent, offset,
> +                                                      blocksize, last, 0))
> +                                               goto error;
> +                                       break;
> +                               case 'H':
> +                                       if (check_hole(extent, fd, offset,
> +                                                      blocksize))
> +                                               goto error;
> +                                       break;
> +
> +                               case 'P':
> +                                       if (check_data(extent, offset,
> +                                                      blocksize, last, 1))
> +                                               goto error;
> +                                       break;
> +                               default:
> +                                       printf("ERROR: weird value in "
> +                                              "map: %c\n", map[i]);
>                                        goto error;
> -                               break;
> -                       case 'P':
> -                               if (check_data(fiemap, logical_offset,
> -                                              blocksize, last_data == i, 1))
> +                               }
> +                       }
> +
> +                       for (; cur_block < block; cur_block++) {
> +                               offset = (__u64)cur_block * blocksize;
> +                               if (check_hole(extent, fd, offset, blocksize))
>                                        goto error;
> -                               break;
> -                       default:
> -                               printf("ERROR: weird value in map: %c\n",
> -                                      map[i]);
> -                               goto error;
>                        }
>                }
> -               cur_extent = i;
> -               map_start = i * blocksize;
> -       } while (cur_extent < blocks);
> +       } while (cur_block < blocks);
>
> -       ret = 0;
> -       return ret;
> +       if (!last_found && last_data != -1) {
> +               printf("ERROR: find no last extent\n");
> +               goto error;
> +       }
> +
> +       free(fiebuf);
> +       return 0;
>  error:
>        printf("map is '%s'\n", map);
>        show_extents(fiemap, blocksize);
> +       free(fiebuf);
>        return -1;
>  }
>
> @@ -605,6 +634,18 @@ main(int argc, char **argv)
>                printf("Starting infinite run, if you don't see any output "
>                       "then its working properly.\n");
>        do {
> +               if (ftruncate(fd, 0)) {
> +                       perror("Could not truncate file\n");
> +                       close(fd);
> +                       exit(1);
> +               }
> +
> +               if (lseek(fd, 0, SEEK_SET)) {
> +                       perror("Could not seek set\n");
> +                       close(fd);
> +                       exit(1);
> +               }
> +
>                if (!map) {
>                        blocks = random() % maxblocks;
>                        if (blocks == 0) {
> @@ -639,18 +680,6 @@ main(int argc, char **argv)
>                free(map);
>                map = NULL;
>
> -               if (ftruncate(fd, 0)) {
> -                       perror("Could not truncate file\n");
> -                       close(fd);
> -                       exit(1);
> -               }
> -
> -               if (lseek(fd, 0, SEEK_SET)) {
> -                       perror("Could not seek set\n");
> -                       close(fd);
> -                       exit(1);
> -               }
> -
>                if (runs) runs--;
>        } while (runs != 0);
>
> --
> 1.7.5.1
>
>



-- 
Best Wishes
Yongqiang Yang

<Prev in Thread] Current Thread [Next in Thread>