* [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
@ 2011-05-19 7:21 ` Yongqiang Yang
0 siblings, 0 replies; 10+ messages in thread
From: Yongqiang Yang @ 2011-05-19 7:21 UTC (permalink / raw)
To: josef, sandeen; +Cc: linux-ext4, Yongqiang Yang, xfs
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@gmail.com>
---
v1->v2:
Josef <josef@redhat.com> 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@gmail.com>, 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
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
2011-05-19 7:21 ` Yongqiang Yang
@ 2011-05-19 7:24 ` Yongqiang Yang
-1 siblings, 0 replies; 10+ messages in thread
From: Yongqiang Yang @ 2011-05-19 7:24 UTC (permalink / raw)
To: josef; +Cc: Eric Sandeen, xfs, linux-ext4
Hi Josef,
I updated the patch, if you think it is ok now and 'Reviewed-by: Josef
<josef@redhat.com>' can be added, please throw a word to Eric.
Yongqiang.
On Thu, May 19, 2011 at 3:21 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> 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@gmail.com>
> ---
> v1->v2:
> Josef <josef@redhat.com> 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@gmail.com>, 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
--
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
@ 2011-05-19 7:24 ` Yongqiang Yang
0 siblings, 0 replies; 10+ messages in thread
From: Yongqiang Yang @ 2011-05-19 7:24 UTC (permalink / raw)
To: josef; +Cc: Eric Sandeen, linux-ext4, xfs
Hi Josef,
I updated the patch, if you think it is ok now and 'Reviewed-by: Josef
<josef@redhat.com>' can be added, please throw a word to Eric.
Yongqiang.
On Thu, May 19, 2011 at 3:21 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> 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@gmail.com>
> ---
> v1->v2:
> Josef <josef@redhat.com> 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@gmail.com>, 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
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
2011-05-19 7:21 ` Yongqiang Yang
@ 2011-05-19 11:12 ` Dave Chinner
-1 siblings, 0 replies; 10+ messages in thread
From: Dave Chinner @ 2011-05-19 11:12 UTC (permalink / raw)
To: Yongqiang Yang; +Cc: josef, sandeen, xfs, linux-ext4
On Thu, May 19, 2011 at 03:21:05PM +0800, Yongqiang Yang 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.
The change log could do with some work. What cases did 225 not
cover, what new cases does it cover, what are the impacts of the
change, what bugs were fixed, etc. "I didn't understand the existing
code so I rewrote it" isn't very useful to someone trying to
understand why the changes were made in a couple of years time....
>
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> ---
> v1->v2:
> Josef <josef@redhat.com> 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@gmail.com>, 2011
> */
Changelogs belong in the repository commit message, not the code.
> #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));
What is the bug this is fixing? Nothing in the change log tells me
why this is being done....
> if (!map)
> return NULL;
>
> @@ -81,6 +84,7 @@ generate_file_mapping(int blocks, int prealloc)
> cur_block++;
> }
>
> + map[blocks] = 0;
Why is this necessary?
> 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)
Perhaps adding a comment explaing what the function is actually
checking is in order seeing as you just changed it...
....
> @@ -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++) {
What bug in the test is this fixing? Surely a short read indicates
a filesystem problem? Only checking the bytes returned instead of
the entire hole region will hide the fact that the read didn't cover
the range that was asked for and so we haven't validated the entire
region we were supposed to...
> @@ -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)
> {
Same issue with adding a comment explaining what is being
checked....
> - 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;
error = check_weird_fs_hole(fd, logical_offset,
blocksize);
if (error == 0)
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;
> }
I can't say I like this - the previous way of iterating the map and
checking that the overlapping fiemap extent is of the correct type
seems much simpler to me.
Your code now has to special case fiemaps with nr_extents == 0, has
to handle holes separately both before and after data extents and
adds a bunch of duplicated code. I find it much harder to follow,
and I don't see how rewriting the code like this makes the test
better. Especially as it doesn't tell me what the problems with the
original code were that you've supposedly fixed...
>
> @@ -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);
What bug is this fixing? There's Nothing in the changelog....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
@ 2011-05-19 11:12 ` Dave Chinner
0 siblings, 0 replies; 10+ messages in thread
From: Dave Chinner @ 2011-05-19 11:12 UTC (permalink / raw)
To: Yongqiang Yang; +Cc: sandeen, linux-ext4, josef, xfs
On Thu, May 19, 2011 at 03:21:05PM +0800, Yongqiang Yang 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.
The change log could do with some work. What cases did 225 not
cover, what new cases does it cover, what are the impacts of the
change, what bugs were fixed, etc. "I didn't understand the existing
code so I rewrote it" isn't very useful to someone trying to
understand why the changes were made in a couple of years time....
>
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> ---
> v1->v2:
> Josef <josef@redhat.com> 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@gmail.com>, 2011
> */
Changelogs belong in the repository commit message, not the code.
> #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));
What is the bug this is fixing? Nothing in the change log tells me
why this is being done....
> if (!map)
> return NULL;
>
> @@ -81,6 +84,7 @@ generate_file_mapping(int blocks, int prealloc)
> cur_block++;
> }
>
> + map[blocks] = 0;
Why is this necessary?
> 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)
Perhaps adding a comment explaing what the function is actually
checking is in order seeing as you just changed it...
....
> @@ -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++) {
What bug in the test is this fixing? Surely a short read indicates
a filesystem problem? Only checking the bytes returned instead of
the entire hole region will hide the fact that the read didn't cover
the range that was asked for and so we haven't validated the entire
region we were supposed to...
> @@ -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)
> {
Same issue with adding a comment explaining what is being
checked....
> - 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;
error = check_weird_fs_hole(fd, logical_offset,
blocksize);
if (error == 0)
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;
> }
I can't say I like this - the previous way of iterating the map and
checking that the overlapping fiemap extent is of the correct type
seems much simpler to me.
Your code now has to special case fiemaps with nr_extents == 0, has
to handle holes separately both before and after data extents and
adds a bunch of duplicated code. I find it much harder to follow,
and I don't see how rewriting the code like this makes the test
better. Especially as it doesn't tell me what the problems with the
original code were that you've supposedly fixed...
>
> @@ -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);
What bug is this fixing? There's Nothing in the changelog....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
2011-05-19 11:12 ` Dave Chinner
@ 2011-05-22 12:38 ` Yongqiang Yang
-1 siblings, 0 replies; 10+ messages in thread
From: Yongqiang Yang @ 2011-05-22 12:38 UTC (permalink / raw)
To: Dave Chinner; +Cc: josef, sandeen, xfs, linux-ext4
Hi Dave,
Thank you for your review. I will resend it.
On Thu, May 19, 2011 at 7:12 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, May 19, 2011 at 03:21:05PM +0800, Yongqiang Yang 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.
>
> The change log could do with some work. What cases did 225 not
> cover, what new cases does it cover, what are the impacts of the
> change, what bugs were fixed, etc. "I didn't understand the existing
> code so I rewrote it" isn't very useful to someone trying to
> understand why the changes were made in a couple of years time....
>
>>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>> ---
>> v1->v2:
>> Josef <josef@redhat.com> 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@gmail.com>, 2011
>> */
>
> Changelogs belong in the repository commit message, not the code.
>
>> #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));
>
> What is the bug this is fixing? Nothing in the change log tells me
> why this is being done....
>
>> if (!map)
>> return NULL;
>>
>> @@ -81,6 +84,7 @@ generate_file_mapping(int blocks, int prealloc)
>> cur_block++;
>> }
>>
>> + map[blocks] = 0;
>
> Why is this necessary?
If there are any errors, map is printed out as a string.
>
>> 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)
>
> Perhaps adding a comment explaing what the function is actually
> checking is in order seeing as you just changed it...
>
> ....
>> @@ -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++) {
>
> What bug in the test is this fixing? Surely a short read indicates
> a filesystem problem? Only checking the bytes returned instead of
> the entire hole region will hide the fact that the read didn't cover
> the range that was asked for and so we haven't validated the entire
> region we were supposed to...
I think read operation is verified in other tests, XFS has blocks
after the size, so I rely on read bytes.
>
>> @@ -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)
>> {
>
> Same issue with adding a comment explaining what is being
> checked....
>
>> - 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;
>
> error = check_weird_fs_hole(fd, logical_offset,
> blocksize);
> if (error == 0)
> 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;
>> }
>
> I can't say I like this - the previous way of iterating the map and
> checking that the overlapping fiemap extent is of the correct type
> seems much simpler to me.
>
> Your code now has to special case fiemaps with nr_extents == 0, has
> to handle holes separately both before and after data extents and
> adds a bunch of duplicated code. I find it much harder to follow,
> and I don't see how rewriting the code like this makes the test
> better. Especially as it doesn't tell me what the problems with the
> original code were that you've supposedly fixed...
I could not figure out logic in old code comparing fiemap and map,
I will update the changelog.
>
>>
>> @@ -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);
>
> What bug is this fixing? There's Nothing in the changelog....
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
--
Best Wishes
Yongqiang Yang
--
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
@ 2011-05-22 12:38 ` Yongqiang Yang
0 siblings, 0 replies; 10+ messages in thread
From: Yongqiang Yang @ 2011-05-22 12:38 UTC (permalink / raw)
To: Dave Chinner; +Cc: sandeen, linux-ext4, josef, xfs
Hi Dave,
Thank you for your review. I will resend it.
On Thu, May 19, 2011 at 7:12 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, May 19, 2011 at 03:21:05PM +0800, Yongqiang Yang 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.
>
> The change log could do with some work. What cases did 225 not
> cover, what new cases does it cover, what are the impacts of the
> change, what bugs were fixed, etc. "I didn't understand the existing
> code so I rewrote it" isn't very useful to someone trying to
> understand why the changes were made in a couple of years time....
>
>>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>> ---
>> v1->v2:
>> Josef <josef@redhat.com> 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@gmail.com>, 2011
>> */
>
> Changelogs belong in the repository commit message, not the code.
>
>> #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));
>
> What is the bug this is fixing? Nothing in the change log tells me
> why this is being done....
>
>> if (!map)
>> return NULL;
>>
>> @@ -81,6 +84,7 @@ generate_file_mapping(int blocks, int prealloc)
>> cur_block++;
>> }
>>
>> + map[blocks] = 0;
>
> Why is this necessary?
If there are any errors, map is printed out as a string.
>
>> 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)
>
> Perhaps adding a comment explaing what the function is actually
> checking is in order seeing as you just changed it...
>
> ....
>> @@ -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++) {
>
> What bug in the test is this fixing? Surely a short read indicates
> a filesystem problem? Only checking the bytes returned instead of
> the entire hole region will hide the fact that the read didn't cover
> the range that was asked for and so we haven't validated the entire
> region we were supposed to...
I think read operation is verified in other tests, XFS has blocks
after the size, so I rely on read bytes.
>
>> @@ -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)
>> {
>
> Same issue with adding a comment explaining what is being
> checked....
>
>> - 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;
>
> error = check_weird_fs_hole(fd, logical_offset,
> blocksize);
> if (error == 0)
> 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;
>> }
>
> I can't say I like this - the previous way of iterating the map and
> checking that the overlapping fiemap extent is of the correct type
> seems much simpler to me.
>
> Your code now has to special case fiemaps with nr_extents == 0, has
> to handle holes separately both before and after data extents and
> adds a bunch of duplicated code. I find it much harder to follow,
> and I don't see how rewriting the code like this makes the test
> better. Especially as it doesn't tell me what the problems with the
> original code were that you've supposedly fixed...
I could not figure out logic in old code comparing fiemap and map,
I will update the changelog.
>
>>
>> @@ -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);
>
> What bug is this fixing? There's Nothing in the changelog....
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
--
Best Wishes
Yongqiang Yang
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
2011-05-19 7:21 ` Yongqiang Yang
@ 2011-05-19 18:19 ` Josef Bacik
-1 siblings, 0 replies; 10+ messages in thread
From: Josef Bacik @ 2011-05-19 18:19 UTC (permalink / raw)
To: Yongqiang Yang; +Cc: josef, sandeen, xfs, linux-ext4
On Thu, May 19, 2011 at 03:21:05PM +0800, Yongqiang Yang 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@gmail.com>
Reviewed-by: Josef Bacik <josef@redhat.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xfstests:Make 225 compare map and fiemap at each block.
@ 2011-05-19 18:19 ` Josef Bacik
0 siblings, 0 replies; 10+ messages in thread
From: Josef Bacik @ 2011-05-19 18:19 UTC (permalink / raw)
To: Yongqiang Yang; +Cc: sandeen, linux-ext4, josef, xfs
On Thu, May 19, 2011 at 03:21:05PM +0800, Yongqiang Yang 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@gmail.com>
Reviewed-by: Josef Bacik <josef@redhat.com>
Thanks,
Josef
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread