On Fri, Nov 12, 2010 at 12:19:34PM -0500, Josef Bacik wrote:
> When trying to add a test for hole punching I noticed that the "bmap" command
> only works on XFS, which makes testing hole punching on other fs's kind of
> difficult. To fix this I've added an fiemap command that works almost exactly
> like bmap. It is formatted similarly and takes similar flags, the only thing
> thats different is obviously it doesn't spit out AG info and it doesn't make
> finding prealloc space optional. This is my first foray into all of this stuff
> so a good hard look would be appreciated. I tested it with a few different
> files to make sure bmap and fiemap both looked the same. Thanks,
>
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxx>
> ---
> V1->V2: add checks to make sure the system supports fiemap so xfsprogs builds on
> things other than linux :).
> +static void
> +fiemap_help(void)
> +{
> + printf(_(
> +"\n"
> +" prints the block mapping for a file's data or attribute forks"
> +"\n"
> +" Example:\n"
> +" 'bmap -vp' - tabular format verbose map, including unwritten extents\n"
'fiemap -vp' ?
> +"\n"
> +" bmap prints the map of disk blocks used by the current file.\n"
fiemap prints...
> +" The map lists each extent used by the file, as well as regions in the\n"
> +" file that do not have any corresponding blocks (holes).\n"
> +" By default, each line of the listing takes the following form:\n"
> +" extent: [startoffset..endoffset]: startblock..endblock\n"
> +" Holes are marked by replacing the startblock..endblock with 'hole'.\n"
> +" All the file offsets and disk blocks are in units of 512-byte blocks.\n"
> +" -a -- prints the attribute fork map instead of the data fork.\n"
> +" -l -- also displays the length of each extent in 512-byte blocks.\n"
> +" -n -- query n extents.\n"
> +" -v -- Verbose information\n"
> +" Note: the bmap for non-regular files can be obtained provided the file\n"
> +" was opened appropriately (in particular, must be opened read-only).\n"
> +"\n"));
This last note about non-regular files is not true for fiemap,
right?
> +}
> +
> +static int
> +numlen(
> + __u64 val,
> + int base)
> +{
> + __u64 tmp;
> + int len;
> +
> + for (len = 0, tmp = val; tmp > 0; tmp = tmp/base)
> + len++;
> + return (len == 0 ? 1 : len);
> +}
> +
> +static void
> +print_verbose(
> + struct fiemap_extent *extent,
> + int blocksize,
> + int foff_w,
> + int boff_w,
> + int tot_w,
> + int flg_w,
> + int *cur_extent,
> + __u64 *last_logical)
> +{
> + __u64 lstart;
> + __u64 len;
> + __u64 block;
> + char lbuf[32];
> + char bbuf[32];
I don't think these two buffers are large enough to hold 2
64 bit integers as strings.
> +static void
> +print_plain(
> + struct fiemap_extent *extent,
> + int lflag,
> + int blocksize,
> + int *cur_extent,
> + __u64 *last_logical)
> +{
> + __u64 lstart;
> + __u64 block;
> + __u64 len;
> +
> + lstart = extent->fe_logical / blocksize;
> + len = extent->fe_length / blocksize;
> + block = extent->fe_physical / blocksize;
> +
> + if (lstart != *last_logical) {
> + printf("\t%d: [%llu..%llu]: hole", *cur_extent,
> + *last_logical, lstart - 1LL);
> + if (lflag)
> + printf(_(" %llu blocks\n"),
> + lstart - *last_logical);
> + else
> + printf("\n");
> + (*cur_extent)++;
> + }
> +
> + printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
> + lstart, lstart + len - 1LL, block,
> + block + len - 1);
Why the "1LL" here and not anywhere else?
> +
> + if (lflag)
> + printf(_(" %llu blocks\n"), len);
> + else
> + printf("\n");
> + (*cur_extent)++;
> + *last_logical = lstart + len;
> +}
> +
> +int
> +fiemap_f(
> + int argc,
> + char **argv)
> +{
> + struct fiemap *fiemap;
> + int num_extents = 32;
> + int nflag = 0;
> + int lflag = 0;
> + int vflag = 0;
> + int fiemap_flags = FIEMAP_FLAG_SYNC;
> + int c;
> + int i;
> + int map_size;
> + int ret;
> + int cur_extent = 0;
> + int foff_w = 16; /* 16 just for a good minimum range */
> + int boff_w = 16;
> + int tot_w = 5; /* 5 since its just one number */
> + int flg_w = 5;
> + __u64 blocksize = 512;
> + __u64 last_logical = 0;
> + struct stat st;
> +
> + while ((c = getopt(argc, argv, "aln:v")) != EOF) {
> + switch (c) {
> + case 'a':
> + fiemap_flags |= FIEMAP_FLAG_XATTR;
> + break;
> + case 'l':
> + lflag = 1;
> + break;
> + case 'n':
> + num_extents = atoi(optarg);
> + nflag = 1;
> + break;
> + case 'v':
> + vflag++;
> + break;
> + default:
> + return command_usage(&fiemap_cmd);
> + }
> + }
> +
> + map_size = sizeof(struct fiemap) +
> + (num_extents * sizeof(struct fiemap_extent));
Need to validate num_extents before using it.
> + fiemap = malloc(map_size);
> + if (!fiemap) {
> + fprintf(stderr, _("%s: malloc of %d bytes failed.\n"),
> + progname, map_size);
> + exitcode = 1;
> + return 0;
> + }
> +
> + memset(fiemap, 0, map_size);
> + fiemap->fm_flags = fiemap_flags;
> + fiemap->fm_length = -1;
> + fiemap->fm_extent_count = nflag ? 0 : num_extents;
> +
> + ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> + if (ret < 0) {
> + fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
> + "%s\n", progname, file->name, strerror(errno));
> + free(fiemap);
> + exitcode = 1;
> + return 0;
> + }
> +
> + if (!nflag) {
> + if (fiemap->fm_mapped_extents > num_extents) {
> + num_extents = fiemap->fm_mapped_extents;
> + map_size = sizeof(struct fiemap) +
> + (num_extents * sizeof(struct fiemap_extent));
> + fiemap = realloc(fiemap, map_size);
> + if (!fiemap) {
> + fprintf(stderr, _("%s: realloc of %d bytes "
> + "failed.\n"), progname,
> + map_size);
> + exitcode = 1;
> + return 0;
> + }
> + }
> + memset(fiemap, 0, map_size);
> + fiemap->fm_flags = fiemap_flags;
> + fiemap->fm_length = -1;
> + fiemap->fm_extent_count = num_extents;
> +
> + ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> + if (ret < 0) {
> + fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
> + "%s\n", progname, file->name, strerror(errno));
> + free(fiemap);
> + exitcode = 1;
> + return 0;
> + }
> + }
Hmmm. I'd prefer to see a loop mapping and printing N extents at a
time rather than one massive allocation and hoping it fits in memory
(think of a file with hundreds of thousands of extents). That's a
problem with the existing xfs_bmap code - should we be duplicating
that problem here?
Otherwise seems fine.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
