Re: [PATCH 3/3] ioctl_xfs_ioc_getfsmap.2: document XFS_IOC_GETFSMAP ioctl

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Sep 09, 2016 at 09:38:06AM +1000, Dave Chinner wrote:
> On Tue, Aug 30, 2016 at 12:09:49PM -0700, Darrick J. Wong wrote:
> > > I recall for FIEMAP that some filesystems may not have files aligned
> > > to sector offsets, and we just used byte offsets.  Storage like
> > > NVDIMMs are cacheline granular, so I don't think it makes sense to
> > > tie this to old disk sector sizes.  Alternately, the units could be
> > > in terms of fs blocks as returned by statvfs.st_bsize, but mixing
> > > units for fmv_block, fmv_offset, fmv_length is uneeded complexity.
> > 
> > Ugh.  I'd rather just change the units to bytes rather than force all
> > the users to multiply things. :)
> 
> Yup, units need to be either in disk addresses (i.e. 512 byte units)
> or bytes. If people can't handle disk addresses (seems to be the
> case), the bytes it should be.

<nod>

> > I'd much rather just add more special owner codes for any other
> > filesystem that has distinguishable metadata types that are not
> > covered by the existing OWN_ codes.  We /do/ have 2^64 possible
> > values, so it's not like we're going to run out.
> 
> This is diagnositc information as much as anything, just like
> fiemap is diagnostic information. So if we have specific type
> information, it needs to be reported accurately to be useful.
> 
> Hence I really don't care if the users and developers of other fs
> types don't understand what the special owner codes that a specific
> filesystem returns mean. i.e. it's not useful user information -
> only a tool that groks the specific filesystem is going to be able
> to anything useful with special owner codes. So, IMO, there's little
> point trying to make them generic or to even trying to define and
> explain them in the man page....

<shrug> I'm ok with describing generally what each special owner code
means.  Maybe the manpage could be more explicit about "None of these
codes are useful unless you're a low level filesystem tool"?

> > > It seems like there are several fields in the structure that are used for
> > > only input or only output?  Does it make more sense to have one structure
> > > used only for the input request, and then the array of values returned be
> > > in a different structure?  I'm not necessarily requesting that it be changed,
> > > but it definitely is something I noticed a few times while reading this doc.
> > 
> > I've been thinking about rearranging this a bit, since the flags
> > handling is very awkward with the current array structure.  Each
> > rmap has its own flags; we may someday want to pass operation flags
> > into the ioctl; and we currently have one operation flag to pass back
> > to userspace.  Each of those flags can be a separate field.  I think
> > people will get confused about FMV_OF_* and FMV_HOF_* being referenced
> > in oflags, and iflags has no meaning for returned records.
> 
> Yup, that's what I initially noticed when I glanced at this. The XFS
> getbmap interface is just plain nasty, and we shouldn't be copying
> that API pattern if we can help it.

Lol ok. :)

> > So, this instead?
> > 
> > struct getfsmap_rec {
> > 	u32 device;		/* device id */
> > 	u32 flags;		/* mapping flags */
> > 	u64 block;		/* physical addr, bytes */
> > 	u64 owner;		/* inode or special owner code */
> > 	u64 offset;		/* file offset of mapping, bytes */
> > 	u64 length;		/* length of segment, bytes */
> > 	u64 reserved;		/* will be set to zero */
> > }; /* 48 bytes */
> > 
> > struct getfsmap_head {
> > 	u32 iflags;		/* none defined yet */
> > 	u32 oflags;		/* FMV_HOF_DEV_T */
> > 	u32 count;		/* # entries in recs array */
> > 	u32 entries;		/* # entries filled in (output) */
> > 	u64 reserved[2]; 	/* must be zero */
> > 
> > 	struct getfsmap_rec keys[2]; /* low and high keys for the mapping search */
> > 	struct getfsmap_rec recs[0];
> > }; /* 32 bytes + 2*48 = 128 bytes */
> > 
> > #define XFS_IOC_GETFSMAP	_IOWR('X', 59, struct getfsmap_head)
> > 
> > This also means that userspace can set up for the next ioctl
> > invocation with memcpy(&head->keys[0], &head->recs[head->entries - 1]).
> > 
> > Yes, I think I like this better.  Everyone else, please chime in. :)
> 
> That's pretty much the structure I was going to suggest - it matches
> the fiemap pattern. i.e control parameters are separated from record
> data. I'd dump a bit more reserved space in the structure, though;
> we've got heaps of flag space for future expansion, but if we need
> to pass new parameters into/out of the kernel we'll quickly use the
> reserved space.

I padded struct fsmap with enough reserved space to make it an even 64 bytes,
and padded struct fsmap_head so that the space before keys is 64 bytes in
length.  See v3 patch of the ioctl manpage.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com