[PATCH] Xfsprogs: add fiemap command to xfs_io V2

[Josef Bacik]

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 at redhat.com>
> > ---
> > 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