[PATCH 1/2, RFC] convert xfs_getbmap to take formatter functions

[PATCH 1/2, RFC] convert xfs_getbmap to take formatter functions

[Eric Sandeen]

Preliminary work to hook up fiemap, this allows us to pass in an
arbitrary formatter to copy extent data back to userspace.

The formatter takes info for 1 extent, a pointer to the user "thing*"
and a pointer to a "filled" variable to indicate whether a userspace
buffer did get filled in (for fiemap, hole "extents" are skipped).

I'm just using the getbmapx struct as a "common denominator" because
as far as I can see, it holds all info that any formatters will care
about.

("*thing" because fiemap doesn't pass the user pointer around, but rather
has a pointer to a fiemap info structure, and helpers associated with it)

I'm still working on the fiemap hookup but wanted to put this out for comment.

Thanks,
-Eric

 linux-2.6/xfs_ioctl.c |   39 ++++++++----------------
 xfs_bmap.c            |   81 +++++++++++++++++++++++++++-----------------------
 xfs_bmap.h            |   13 +++++---
 xfs_fs.h              |   12 -------
 4 files changed, 67 insertions(+), 78 deletions(-)

Index: linux-2.6.27.x86_64/fs/xfs/xfs_bmap.c
===================================================================
--- linux-2.6.27.x86_64.orig/fs/xfs/xfs_bmap.c
+++ linux-2.6.27.x86_64/fs/xfs/xfs_bmap.c
@@ -5742,9 +5742,9 @@ error0:
 STATIC int
 xfs_getbmapx_fix_eof_hole(
 	xfs_inode_t		*ip,		/* xfs incore inode pointer */
-	struct getbmap		*out,		/* output structure */
+	struct getbmapx		*out,		/* output structure */
 	int			prealloced,	/* this is a file with
-						* preallocated data space */
+						 * preallocated data space */
 	__int64_t		end,		/* last block requested */
 	xfs_fsblock_t		startblock)
 {
@@ -5769,15 +5769,38 @@ xfs_getbmapx_fix_eof_hole(
 	return 1;
 }
 
+int xfs_getbmapx_format(void __user **ap, struct getbmapx *bmv, int *filled)
+{
+	*filled = 0;
+	if (copy_to_user(*ap, bmv, sizeof(struct getbmapx)))
+		return XFS_ERROR(EFAULT);
+
+	*ap += sizeof(struct getbmapx);
+	*filled = 1;
+	return 0;
+}
+
+int xfs_getbmap_format(void __user **ap, struct getbmapx *bmv, int *filled)
+{
+	*filled = 0;
+	/* copy only getbmap portion (not getbmapx) */
+	if (copy_to_user(ap, bmv, sizeof(struct getbmap)))
+		return XFS_ERROR(EFAULT);
+
+	*ap += sizeof(struct getbmap);
+	*filled = 1;
+	return 0;
+}
+
 /*
- * Fcntl interface to xfs_bmapi.
+ * ioctl interface to xfs_bmapi.
  */
 int						/* error code */
 xfs_getbmap(
 	xfs_inode_t		*ip,
-	struct getbmap		*bmv,		/* user bmap structure */
-	void			__user *ap,	/* pointer to user's array */
-	int			interface)	/* interface flags */
+	struct getbmapx		*bmv,		/* user bmap structure */
+	xfs_bmap_format_t	formatter,	/* format to user */
+	void			*arg)		/* formatter arg (user ptr etc) */
 {
 	__int64_t		bmvend;		/* last block requested */
 	int			error;		/* return value */
@@ -5790,19 +5813,20 @@ xfs_getbmap(
 	int			nexleft;	/* # of user extents left */
 	int			subnex;		/* # of bmapi's can do */
 	int			nmap;		/* number of map entries */
-	struct getbmap		out;		/* output structure */
+	struct getbmapx		out;		/* output structure */
 	int			whichfork;	/* data or attr fork */
 	int			prealloced;	/* this is a file with
 						 * preallocated data space */
 	int			sh_unwritten;	/* true, if unwritten */
 						/* extents listed separately */
+	int			iflags;		/* interface flags */
 	int			bmapi_flags;	/* flags for xfs_bmapi */
-	__int32_t		oflags;		/* getbmapx bmv_oflags field */
 
 	mp = ip->i_mount;
+	iflags = bmv->bmv_iflags;
 
-	whichfork = interface & BMV_IF_ATTRFORK ? XFS_ATTR_FORK : XFS_DATA_FORK;
-	sh_unwritten = (interface & BMV_IF_PREALLOC) != 0;
+	whichfork = iflags & BMV_IF_ATTRFORK ? XFS_ATTR_FORK : XFS_DATA_FORK;
+	sh_unwritten = (iflags & BMV_IF_PREALLOC) != 0;
 
 	/*	If the BMV_IF_NO_DMAPI_READ interface bit specified, do not
 	 *	generate a DMAPI read event.  Otherwise, if the DM_EVENT_READ
@@ -5817,7 +5841,7 @@ xfs_getbmap(
 	 *	could misinterpret holes in a DMAPI file as true holes,
 	 *	when in fact they may represent offline user data.
 	 */
-	if ((interface & BMV_IF_NO_DMAPI_READ) == 0 &&
+	if ((iflags & BMV_IF_NO_DMAPI_READ) == 0 &&
 	    DM_EVENT_ENABLED(ip, DM_EVENT_READ) &&
 	    whichfork == XFS_DATA_FORK) {
 		error = XFS_SEND_DATA(mp, DM_EVENT_READ, ip, 0, 0, 0, NULL);
@@ -5924,52 +5948,35 @@ xfs_getbmap(
 		ASSERT(nmap <= subnex);
 
 		for (i = 0; i < nmap && nexleft && bmv->bmv_length; i++) {
-			nexleft--;
-			oflags = (map[i].br_state == XFS_EXT_UNWRITTEN) ?
+			out.bmv_oflags = (map[i].br_state == XFS_EXT_UNWRITTEN) ?
 					BMV_OF_PREALLOC : 0;
 			out.bmv_offset = XFS_FSB_TO_BB(mp, map[i].br_startoff);
 			out.bmv_length = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
+			out.bmv_unused1 = out.bmv_unused2 = 0;
 			ASSERT(map[i].br_startblock != DELAYSTARTBLOCK);
                         if (map[i].br_startblock == HOLESTARTBLOCK &&
 			    whichfork == XFS_ATTR_FORK) {
 				/* came to the end of attribute fork */
 				goto unlock_and_return;
 			} else {
+				int filled;	/* extents filled by formatter */
+
 				if (!xfs_getbmapx_fix_eof_hole(ip, &out,
 							prealloced, bmvend,
 							map[i].br_startblock)) {
 					goto unlock_and_return;
 				}
 
-				/* return either getbmap/getbmapx structure. */
-				if (interface & BMV_IF_EXTENDED) {
-					struct	getbmapx	outx;
-
-					GETBMAP_CONVERT(out,outx);
-					outx.bmv_oflags = oflags;
-					outx.bmv_unused1 = outx.bmv_unused2 = 0;
-					if (copy_to_user(ap, &outx,
-							sizeof(outx))) {
-						error = XFS_ERROR(EFAULT);
-						goto unlock_and_return;
-					}
-				} else {
-					if (copy_to_user(ap, &out,
-							sizeof(out))) {
-						error = XFS_ERROR(EFAULT);
-						goto unlock_and_return;
-					}
-				}
+				/* format results into user's buffer & advance */
+				error = formatter(&arg, &out, &filled);
+				if (error)
+					goto unlock_and_return;
+				nexleft -= filled;
 				bmv->bmv_offset =
 					out.bmv_offset + out.bmv_length;
 				bmv->bmv_length = MAX((__int64_t)0,
 					(__int64_t)(bmvend - bmv->bmv_offset));
 				bmv->bmv_entries++;
-				ap = (interface & BMV_IF_EXTENDED) ?
-						(void __user *)
-					((struct getbmapx __user *)ap + 1) :
-						(void __user *)
-					((struct getbmap __user *)ap + 1);
 			}
 		}
 	} while (nmap && nexleft && bmv->bmv_length);
Index: linux-2.6.27.x86_64/fs/xfs/xfs_bmap.h
===================================================================
--- linux-2.6.27.x86_64.orig/fs/xfs/xfs_bmap.h
+++ linux-2.6.27.x86_64/fs/xfs/xfs_bmap.h
@@ -343,15 +343,20 @@ xfs_bunmapi(
 						   extents */
 	int			*done);		/* set if not done yet */
 
+/* bmap to userspace formatters - copy to user & advance pointer */
+typedef int (*xfs_bmap_format_t)(void __user **, struct getbmapx *, int *filled);
+int xfs_getbmapx_format(void __user **ap, struct getbmapx *bmv, int *filled);
+int xfs_getbmap_format(void __user **ap, struct getbmapx *bmv, int *filled);
+
 /*
- * Fcntl interface to xfs_bmapi.
+ * ioctl interface to xfs_bmapi.
  */
 int						/* error code */
 xfs_getbmap(
 	xfs_inode_t		*ip,
-	struct getbmap		*bmv,		/* user bmap structure */
-	void			__user *ap,	/* pointer to user's array */
-	int			iflags);	/* interface flags */
+	struct getbmapx		*bmv,		/* user bmap structure */
+	xfs_bmap_format_t	formatter,	/* format to user */
+	void			*arg);		/* formatter arg (user ptr etc) */
 
 /*
  * Check if the endoff is outside the last extent. If so the caller will grow
Index: linux-2.6.27.x86_64/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- linux-2.6.27.x86_64.orig/fs/xfs/linux-2.6/xfs_ioctl.c
+++ linux-2.6.27.x86_64/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -1262,25 +1262,26 @@ xfs_ioc_getbmap(
 	unsigned int		cmd,
 	void			__user *arg)
 {
-	struct getbmap		bm;
-	int			iflags;
+	struct getbmapx		bmx;
 	int			error;
 
-	if (copy_from_user(&bm, arg, sizeof(bm)))
+	if (copy_from_user(&bmx, arg, sizeof(struct getbmapx)))
 		return -XFS_ERROR(EFAULT);
 
-	if (bm.bmv_count < 2)
+	if (bmx.bmv_count < 2)
 		return -XFS_ERROR(EINVAL);
 
-	iflags = (cmd == XFS_IOC_GETBMAPA ? BMV_IF_ATTRFORK : 0);
+	bmx.bmv_iflags = (cmd == XFS_IOC_GETBMAPA ? BMV_IF_ATTRFORK : 0);
 	if (ioflags & IO_INVIS)
-		iflags |= BMV_IF_NO_DMAPI_READ;
+		bmx.bmv_iflags |= BMV_IF_NO_DMAPI_READ;
 
-	error = xfs_getbmap(ip, &bm, (struct getbmap __user *)arg+1, iflags);
+	error = xfs_getbmap(ip, &bmx, xfs_getbmap_format,
+			    (struct getbmap __user *)arg+1);
 	if (error)
 		return -error;
 
-	if (copy_to_user(arg, &bm, sizeof(bm)))
+	/* copy back header - only size of getbmap */
+	if (copy_to_user(arg, &bmx, sizeof(struct getbmap)))
 		return -XFS_ERROR(EFAULT);
 	return 0;
 }
@@ -1291,8 +1292,6 @@ xfs_ioc_getbmapx(
 	void			__user *arg)
 {
 	struct getbmapx		bmx;
-	struct getbmap		bm;
-	int			iflags;
 	int			error;
 
 	if (copy_from_user(&bmx, arg, sizeof(bmx)))
@@ -1301,26 +1300,16 @@ xfs_ioc_getbmapx(
 	if (bmx.bmv_count < 2)
 		return -XFS_ERROR(EINVAL);
 
-	/*
-	 * Map input getbmapx structure to a getbmap
-	 * structure for xfs_getbmap.
-	 */
-	GETBMAP_CONVERT(bmx, bm);
-
-	iflags = bmx.bmv_iflags;
-
-	if (iflags & (~BMV_IF_VALID))
+	if (bmx.bmv_iflags & (~BMV_IF_VALID))
 		return -XFS_ERROR(EINVAL);
 
-	iflags |= BMV_IF_EXTENDED;
-
-	error = xfs_getbmap(ip, &bm, (struct getbmapx __user *)arg+1, iflags);
+	error = xfs_getbmap(ip, &bmx, xfs_getbmapx_format,
+			    (struct getbmapx __user *)arg+1);
 	if (error)
 		return -error;
 
-	GETBMAP_CONVERT(bm, bmx);
-
-	if (copy_to_user(arg, &bmx, sizeof(bmx)))
+	/* copy back header */
+	if (copy_to_user(arg, &bmx, sizeof(struct getbmapx)))
 		return -XFS_ERROR(EFAULT);
 
 	return 0;
Index: linux-2.6.27.x86_64/fs/xfs/xfs_fs.h
===================================================================
--- linux-2.6.27.x86_64.orig/fs/xfs/xfs_fs.h
+++ linux-2.6.27.x86_64/fs/xfs/xfs_fs.h
@@ -114,22 +114,10 @@ struct getbmapx {
 #define BMV_IF_NO_DMAPI_READ	0x2	/* Do not generate DMAPI read event  */
 #define BMV_IF_PREALLOC		0x4	/* rtn status BMV_OF_PREALLOC if req */
 #define BMV_IF_VALID	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC)
-#ifdef __KERNEL__
-#define BMV_IF_EXTENDED 0x40000000	/* getpmapx if set */
-#endif
 
 /*	bmv_oflags values - returned for for each non-header segment */
 #define BMV_OF_PREALLOC		0x1	/* segment = unwritten pre-allocation */
 
-/*	Convert getbmap <-> getbmapx - move fields from p1 to p2. */
-#define GETBMAP_CONVERT(p1,p2) {	\
-	p2.bmv_offset = p1.bmv_offset;	\
-	p2.bmv_block = p1.bmv_block;	\
-	p2.bmv_length = p1.bmv_length;	\
-	p2.bmv_count = p1.bmv_count;	\
-	p2.bmv_entries = p1.bmv_entries;  }
-
-
 /*
  * Structure for XFS_IOC_FSSETDM.
  * For use by backup and restore programs to set the XFS on-disk inode

Re: [PATCH 1/2, RFC] convert xfs_getbmap to take formatter functions

[Christoph Hellwig]

On Mon, Oct 20, 2008 at 05:24:03PM -0500, Eric Sandeen wrote:
> Preliminary work to hook up fiemap, this allows us to pass in an
> arbitrary formatter to copy extent data back to userspace.
> 
> The formatter takes info for 1 extent, a pointer to the user "thing*"
> and a pointer to a "filled" variable to indicate whether a userspace
> buffer did get filled in (for fiemap, hole "extents" are skipped).
> 
> I'm just using the getbmapx struct as a "common denominator" because
> as far as I can see, it holds all info that any formatters will care
> about.
> 
> ("*thing" because fiemap doesn't pass the user pointer around, but rather
> has a pointer to a fiemap info structure, and helpers associated with it)
> 
> I'm still working on the fiemap hookup but wanted to put this out for comment.

This looks good to me.

>  /*
> - * Fcntl interface to xfs_bmapi.
> + * ioctl interface to xfs_bmapi.
>   */

>  /*
> - * Fcntl interface to xfs_bmapi.
> + * ioctl interface to xfs_bmapi.
>   */

Well, it will be for fiemap, too.  And the comment doesn't make much
sense either.  Either remove it completely or replace it by something
that makes sense.

Re: [PATCH 1/2, RFC] convert xfs_getbmap to take formatter functions

[Eric Sandeen]

Christoph Hellwig wrote:
> On Mon, Oct 20, 2008 at 05:24:03PM -0500, Eric Sandeen wrote:
...

>>  /*
>> - * Fcntl interface to xfs_bmapi.
>> + * ioctl interface to xfs_bmapi.
>>   */
> 
>>  /*
>> - * Fcntl interface to xfs_bmapi.
>> + * ioctl interface to xfs_bmapi.
>>   */
> 
> Well, it will be for fiemap, too.  And the comment doesn't make much
> sense either.  Either remove it completely or replace it by something
> that makes sense.
> 

well aren't XFS_IOC_GETBMAP(X) and fiemap _both_ ioctl interfaces to
xfs_bmapi?

-Eric

Re: [PATCH 1/2, RFC] convert xfs_getbmap to take formatter functions

[Christoph Hellwig]

On Tue, Oct 21, 2008 at 08:22:50AM -0500, Eric Sandeen wrote:
> Christoph Hellwig wrote:
> > On Mon, Oct 20, 2008 at 05:24:03PM -0500, Eric Sandeen wrote:
> ...
> 
> >>  /*
> >> - * Fcntl interface to xfs_bmapi.
> >> + * ioctl interface to xfs_bmapi.
> >>   */
> > 
> >>  /*
> >> - * Fcntl interface to xfs_bmapi.
> >> + * ioctl interface to xfs_bmapi.
> >>   */
> > 
> > Well, it will be for fiemap, too.  And the comment doesn't make much
> > sense either.  Either remove it completely or replace it by something
> > that makes sense.
> > 
> 
> well aren't XFS_IOC_GETBMAP(X) and fiemap _both_ ioctl interfaces to
> xfs_bmapi?

Hmm, From the syscall point of view fiemap is indeed an ioctl, too.
But then again the point of this function isn't do anything
ioctl-related, but to allow to get a list of all extents for an inode
and format them arbitrarily.

Re: [PATCH 1/2, RFC] convert xfs_getbmap to take formatter functions

[Eric Sandeen]

Christoph Hellwig wrote:
> On Tue, Oct 21, 2008 at 08:22:50AM -0500, Eric Sandeen wrote:
>> Christoph Hellwig wrote:
>>> On Mon, Oct 20, 2008 at 05:24:03PM -0500, Eric Sandeen wrote:
>> ...
>>
>>>>  /*
>>>> - * Fcntl interface to xfs_bmapi.
>>>> + * ioctl interface to xfs_bmapi.
>>>>   */
>>>>  /*
>>>> - * Fcntl interface to xfs_bmapi.
>>>> + * ioctl interface to xfs_bmapi.
>>>>   */
>>> Well, it will be for fiemap, too.  And the comment doesn't make much
>>> sense either.  Either remove it completely or replace it by something
>>> that makes sense.
>>>
>> well aren't XFS_IOC_GETBMAP(X) and fiemap _both_ ioctl interfaces to
>> xfs_bmapi?
> 
> Hmm, From the syscall point of view fiemap is indeed an ioctl, too.
> But then again the point of this function isn't do anything
> ioctl-related, but to allow to get a list of all extents for an inode
> and format them arbitrarily.

arright... if the comment is the only concern, I'm happy with that  ;)

-Eric