xfs
[Top] [All Lists]

Re: [PATCH] Xfsprogs: add fiemap command to xfs_io V2

To: Josef Bacik <josef@xxxxxxxxxx>
Subject: Re: [PATCH] Xfsprogs: add fiemap command to xfs_io V2
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 18 Nov 2010 16:10:29 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1289582374-19324-1-git-send-email-josef@xxxxxxxxxx>
References: <1289582374-19324-1-git-send-email-josef@xxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
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

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