Skip to contentSkip to conversation

convert xfs_getbmap to take formatter functions

RFC PATCH In review Eric Sandeen 2 participants 5 messages Oct 20, 2008 – Oct 21, 2008 XFS filesystem

lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/2 convert xfs_getbmap to take formatter functions

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(-)
fs/xfs/xfs_bmap.c +44 -37
Index: 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);
fs/xfs/xfs_bmap.h +9 -4
Index: 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
fs/xfs/linux-2.6/xfs_ioctl.c +14 -25
Index: 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;
fs/xfs/xfs_fs.h +0 -12
Index: 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
#1 lore raw
Quoted 7 lines · 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).

Quoted 8 lines

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.

Quoted 9 lines

/*
- * 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.

#2 lore raw
Quoted 1 line · Christoph Hellwig

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

...

Quoted 5 lines
Quoted 4 lines

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

Quoted 9 lines
Quoted 4 lines

/*
- * 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

#3 lore raw
Quoted 14 lines · Eric Sandeen
Quoted 1 line · Christoph Hellwig

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

...

Quoted 10 lines
Quoted 9 lines

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

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

Quoted 7 lines
Quoted 4 lines

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.

#4 lore raw
Quoted 17 lines · Christoph Hellwig
Quoted 16 lines · Eric Sandeen
Quoted 1 line · Christoph Hellwig

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

...

Quoted 12 lines
Quoted 8 lines

/*
- * 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.

Quoted 7 lines
Quoted 2 lines

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