On Thu, Nov 18, 2010 at 04:10:29PM +1100, Dave Chinner wrote:
> 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?
>
Heh you know thats what I did before but decided it was better to do it the way
bmap did it. Thanks for the review, I'll fix all these things up,
Josef
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
