xfs: xfs_bulkstat_single consolidation

From: Jie Liu<[email protected]>

In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might return different error if either call failed, we'd better return the proper error in this case. Moreover, the function argument done is useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.

Signed-off-by Jie Liu<[email protected]>

---

Quoted 7 lines · Jeff Liu

From: Jie Liu<[email protected]>

In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might return different error if either call failed, we'd better return the proper error in this case. Moreover, the function argument done is useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.

Quoted 2 lines

Signed-off-by Jie Liu<[email protected]>

---

Yes, I know dmapi is not loved here but SGI still uses it and it wants the done flag still..

Quoted 8 lines · Mark Tinguely

Quoted 7 lines · Jeff Liu

From: Jie Liu<[email protected]>

In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might return different error if either call failed, we'd better return the proper error in this case. Moreover, the function argument done is useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.

Quoted 5 lines

Quoted 2 lines

Signed-off-by Jie Liu<[email protected]>

---

Yes, I know dmapi is not loved here but SGI still uses it and it wants the done flag still..

My mistake. At that time, I noticed that there has comments about this in xfs_ioc_bulkstat(), i.e,

/* done = 1 if there are more stats to get and if bulkstat */ /* should be called again (unused here, but used in dmapi) */

However, I failed to find out why it would be called by going through the dmapi source code...

I'll keep this argument in next round of post.

Quoted 17 lines · Jeff Liu

Quoted 8 lines · Mark Tinguely

Quoted 7 lines · Jeff Liu

From: Jie Liu<[email protected]>

In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might return different error if either call failed, we'd better return the proper error in this case. Moreover, the function argument done is useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.

Quoted 5 lines

Quoted 2 lines

Signed-off-by Jie Liu<[email protected]>

---

Yes, I know dmapi is not loved here but SGI still uses it and it wants the done flag still..

My mistake. At that time, I noticed that there has comments about this in xfs_ioc_bulkstat(), i.e,

Quoted 7 lines

/* done = 1 if there are more stats to get and if bulkstat */ /* should be called again (unused here, but used in dmapi) */

However, I failed to find out why it would be called by going through the dmapi source code...

I'll keep this argument in next round of post.

Well, let's consider how DMAPI uses it first.

dmapi_ioctl()
use_rvp = 0;
case DM_GET_BULKALL:

    use_rvp = 1;
    dm_get_bulkattr_rvp(*rvp)
      fsys_vector->get_bulkattr_rvp(rvp)
        xfs_dm_get_bulkall_rvp(*rvalp)
          xfs_bulkstat(&done)
          *rvalp = !done ? 1 : 0;

  if (use_rvp && !error)
    return rvp;

Ok, so it returns the "done" status to userspace. How is "done" calculated?

        if (agno >= mp->m_sb.sb_agcount) {
                /*
                 * If we ran out of filesystem, mark lastino as off
                 * the end of the filesystem, so the next call
                 * will return immediately.
                 */
                *lastinop = (xfs_ino_t)XFS_AGINO_TO_INO(mp, agno, 0);
                *done = 1;
        } else
                *lastinop = (xfs_ino_t)lastino;

Oh, so it's nothing special - the lastinop is pointed outside the current filesystem bounds and done is set to 1. IOWs, the dmapi code could easily generate the "done" value based on the returned lastinop value. i.e. xfs_dm_get_bulkall_rvp() can do this after the xfs_bulkstat() call:

	if (XFS_INO_TO_AGNO(mp, lastinop) >= mp->m_sb.sb_agcount)
		*rvalp = 0;
	else
		*rvalp = 1;

And that means we can remove the done parameter from xfs_bulkstat() and no longer have to care about what DMAPI requires. Hence I think the patch as it stands does not impact on DMAPI functionality and so it just fine to clean up...

Quoted 31 lines · Dave Chinner

Quoted 17 lines · Jeff Liu

Quoted 8 lines · Mark Tinguely

Quoted 7 lines · Jeff Liu

From: Jie Liu<[email protected]>

In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might return different error if either call failed, we'd better return the proper error in this case. Moreover, the function argument done is useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.

Quoted 5 lines

Quoted 2 lines

Signed-off-by Jie Liu<[email protected]>

---

Yes, I know dmapi is not loved here but SGI still uses it and it wants the done flag still..

My mistake. At that time, I noticed that there has comments about this in xfs_ioc_bulkstat(), i.e,

Quoted 7 lines

/* done = 1 if there are more stats to get and if bulkstat */ /* should be called again (unused here, but used in dmapi) */

However, I failed to find out why it would be called by going through the dmapi source code...

I'll keep this argument in next round of post.

Well, let's consider how DMAPI uses it first.

dmapi_ioctl()
use_rvp = 0;
case DM_GET_BULKALL:

Quoted 6 lines

    use_rvp = 1;
    dm_get_bulkattr_rvp(*rvp)
      fsys_vector->get_bulkattr_rvp(rvp)
        xfs_dm_get_bulkall_rvp(*rvalp)
          xfs_bulkstat(&done)
          *rvalp = !done ? 1 : 0;

Thanks for the clarification. Now I can understand the use scenarios via DMAPI.

Quoted 30 lines

  if (use_rvp && !error)
    return rvp;

Ok, so it returns the "done" status to userspace. How is "done" calculated?

        if (agno >= mp->m_sb.sb_agcount) {
                /*
                 * If we ran out of filesystem, mark lastino as off
                 * the end of the filesystem, so the next call
                 * will return immediately.
                 */
                *lastinop = (xfs_ino_t)XFS_AGINO_TO_INO(mp, agno, 0);
                *done = 1;
        } else
                *lastinop = (xfs_ino_t)lastino;

Oh, so it's nothing special - the lastinop is pointed outside the current filesystem bounds and done is set to 1. IOWs, the dmapi code could easily generate the "done" value based on the returned lastinop value. i.e. xfs_dm_get_bulkall_rvp() can do this after the xfs_bulkstat() call:

	if (XFS_INO_TO_AGNO(mp, lastinop) >= mp->m_sb.sb_agcount)
		*rvalp = 0;
	else
		*rvalp = 1;

Quoted 4 lines

And that means we can remove the done parameter from xfs_bulkstat() and no longer have to care about what DMAPI requires. Hence I think the patch as it stands does not impact on DMAPI functionality and so it just fine to clean up...

Yep. Except DMAPI, the only user of the done parameter is quota check, i.e, xfs_qm_quotacheck():

        do {
                /*
                 * Iterate thru all the inodes in the file system,
                 * adjusting the corresponding dquot counters in core.
                 */
                error = xfs_bulkstat(mp, &lastino, &count,
                                     xfs_qm_dqusage_adjust,
                                     structsz, NULL, &done);
                if (error)
                        break;

        } while (!done);

But if we finally could perform quota check in parallel, the done parameter can totally be removed as flush_workqueue() can ensure that is completed. Actually, I just did it as the last patch in parallel quota check which is not yet posted.

From: Jie Liu <[email protected]>

In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might return different error if either call failed, we'd better return the proper error in this case. Moreover, the function argument done is useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.

Signed-off-by Jie Liu <[email protected]>

---
 fs/xfs/xfs_ioctl.c  |  3 +--
 fs/xfs/xfs_itable.c | 59 ++++++++++++++++++++++++++---------------------------
 fs/xfs/xfs_itable.h |  3 +--
 3 files changed, 31 insertions(+), 34 deletions(-)

fs/xfs/xfs_ioctl.c +1 -2

index 33ad9a7..2fdd750 100644
@@ -828,8 +828,7 @@ xfs_ioc_bulkstat(
 		error = xfs_inumbers(mp, &inlast, &count,
 					bulkreq.ubuffer, xfs_inumbers_fmt);
 	else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE)
-		error = xfs_bulkstat_single(mp, &inlast,
-						bulkreq.ubuffer, &done);
+		error = xfs_bulkstat_single(mp, &inlast, bulkreq.ubuffer);
 	else	/* XFS_IOC_FSBULKSTAT */
 		error = xfs_bulkstat(mp, &inlast, &count, xfs_bulkstat_one,
 				     sizeof(xfs_bstat_t), bulkreq.ubuffer,

fs/xfs/xfs_itable.c +22 -28

index b5bb7b6..692671c 100644
@@ -509,51 +509,50 @@ xfs_bulkstat(
 }
 
 /*
- * Return stat information in bulk (by-inode) for the filesystem.
- * Special case for non-sequential one inode bulkstat.
+ * Return stat information in bulk (by-inode) for the filesystem.  Special case
+ * for non-sequential one inode bulkstat.
  */
-int					/* error status */
+int
 xfs_bulkstat_single(
-	xfs_mount_t		*mp,	/* mount point for filesystem */
+	struct xfs_mount	*mp,	/* mount point for filesystem */
 	xfs_ino_t		*lastinop, /* inode to return */
-	char			__user *buffer, /* buffer with inode stats */
-	int			*done)	/* 1 if there are more stats to get */
+	char			__user *buffer) /* buffer with inode stats */
 {
-	int			count;	/* count value for bulkstat call */
-	int			error;	/* return value */
-	xfs_ino_t		ino;	/* filesystem inode number */
+	xfs_ino_t		ino = *lastinop;
 	int			res;	/* result from bs1 */
+	int			error;
 
 	/*
-	 * note that requesting valid inode numbers which are not allocated
+	 * Note that requesting valid inode numbers which are not allocated
 	 * to inodes will most likely cause xfs_imap_to_bp to generate warning
-	 * messages about bad magic numbers. This is ok. The fact that
-	 * the inode isn't actually an inode is handled by the
-	 * error check below. Done this way to make the usual case faster
-	 * at the expense of the error case.
+	 * messages about bad magic numbers. This is ok. The fact that the
+	 * inode isn't actually an inode is handled by the error check below.
+	 * Done this way to make the usual case faster at the expense of the
+	 * error case.
 	 */
-
-	ino = *lastinop;
-	error = xfs_bulkstat_one(mp, ino, buffer, sizeof(xfs_bstat_t),
+	error = xfs_bulkstat_one(mp, ino, buffer, sizeof(struct xfs_bstat),
 				 NULL, &res);
 	if (error) {
+		int		count = 1;
+		int		done, error2;
+
 		/*
-		 * Special case way failed, do it the "long" way
-		 * to see if that works.
+		 * Special case way failed, do it the "long" way to see if
+		 * that works.
 		 */
 		(*lastinop)--;
-		count = 1;
-		if (xfs_bulkstat(mp, lastinop, &count, xfs_bulkstat_one,
-				sizeof(xfs_bstat_t), buffer, done))
-			return error;
-		if (count == 0 || (xfs_ino_t)*lastinop != ino)
-			return error == EFSCORRUPTED ?
-				XFS_ERROR(EINVAL) : error;
-		else
-			return 0;
+		error2 = xfs_bulkstat(mp, lastinop, &count, xfs_bulkstat_one,
+				      sizeof(struct xfs_bstat), buffer, &done);
+		if (error2)
+			return error2;

fs/xfs/xfs_itable.c +0 -0

 }
 
 int

fs/xfs/xfs_itable.h +0 -0

index 97295d9..60ce988 100644

Quoted 105 lines · Jeff Liu

From: Jie Liu <[email protected]>

In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might return different error if either call failed, we'd better return the proper error in this case. Moreover, the function argument done is useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.

Signed-off-by Jie Liu <[email protected]>

---
 fs/xfs/xfs_ioctl.c  |  3 +--
 fs/xfs/xfs_itable.c | 59 ++++++++++++++++++++++++++---------------------------
 fs/xfs/xfs_itable.h |  3 +--
 3 files changed, 31 insertions(+), 34 deletions(-)

fs/xfs/xfs_ioctl.c +1 -2

index 33ad9a7..2fdd750 100644
@@ -828,8 +828,7 @@ xfs_ioc_bulkstat(
 		error = xfs_inumbers(mp, &inlast, &count,
 					bulkreq.ubuffer, xfs_inumbers_fmt);
 	else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE)
-		error = xfs_bulkstat_single(mp, &inlast,
-						bulkreq.ubuffer, &done);
+		error = xfs_bulkstat_single(mp, &inlast, bulkreq.ubuffer);
 	else	/* XFS_IOC_FSBULKSTAT */
 		error = xfs_bulkstat(mp, &inlast, &count, xfs_bulkstat_one,
 				     sizeof(xfs_bstat_t), bulkreq.ubuffer,

fs/xfs/xfs_itable.c +27 -28

index b5bb7b6..692671c 100644
@@ -509,51 +509,50 @@ xfs_bulkstat(
 }
 
 /*
- * Return stat information in bulk (by-inode) for the filesystem.
- * Special case for non-sequential one inode bulkstat.
+ * Return stat information in bulk (by-inode) for the filesystem.  Special case
+ * for non-sequential one inode bulkstat.
  */
-int					/* error status */
+int
 xfs_bulkstat_single(
-	xfs_mount_t		*mp,	/* mount point for filesystem */
+	struct xfs_mount	*mp,	/* mount point for filesystem */
 	xfs_ino_t		*lastinop, /* inode to return */
-	char			__user *buffer, /* buffer with inode stats */
-	int			*done)	/* 1 if there are more stats to get */
+	char			__user *buffer) /* buffer with inode stats */
 {
-	int			count;	/* count value for bulkstat call */
-	int			error;	/* return value */
-	xfs_ino_t		ino;	/* filesystem inode number */
+	xfs_ino_t		ino = *lastinop;
 	int			res;	/* result from bs1 */
+	int			error;
 
 	/*
-	 * note that requesting valid inode numbers which are not allocated
+	 * Note that requesting valid inode numbers which are not allocated
 	 * to inodes will most likely cause xfs_imap_to_bp to generate warning
-	 * messages about bad magic numbers. This is ok. The fact that
-	 * the inode isn't actually an inode is handled by the
-	 * error check below. Done this way to make the usual case faster
-	 * at the expense of the error case.
+	 * messages about bad magic numbers. This is ok. The fact that the
+	 * inode isn't actually an inode is handled by the error check below.
+	 * Done this way to make the usual case faster at the expense of the
+	 * error case.
 	 */
-
-	ino = *lastinop;
-	error = xfs_bulkstat_one(mp, ino, buffer, sizeof(xfs_bstat_t),
+	error = xfs_bulkstat_one(mp, ino, buffer, sizeof(struct xfs_bstat),
 				 NULL, &res);
 	if (error) {
+		int		count = 1;
+		int		done, error2;
+
 		/*
-		 * Special case way failed, do it the "long" way
-		 * to see if that works.
+		 * Special case way failed, do it the "long" way to see if
+		 * that works.
 		 */
 		(*lastinop)--;
-		count = 1;
-		if (xfs_bulkstat(mp, lastinop, &count, xfs_bulkstat_one,
-				sizeof(xfs_bstat_t), buffer, done))
-			return error;
-		if (count == 0 || (xfs_ino_t)*lastinop != ino)
-			return error == EFSCORRUPTED ?
-				XFS_ERROR(EINVAL) : error;
-		else
-			return 0;
+		error2 = xfs_bulkstat(mp, lastinop, &count, xfs_bulkstat_one,
+				      sizeof(struct xfs_bstat), buffer, &done);
+		if (error2)
+			return error2;
+
+		if (count == 0 || *lastinop != ino) {
+			if (error == EFSCORRUPTED)
+				error = XFS_ERROR(EINVAL);
+		}

fs/xfs/xfs_itable.c xfs_bulkstat( · line 555

Quoted 5 lines

fs/xfs/xfs_itable.c +2 -2

 	}
-	*done = 0;
-	return 0;
+
+	return error;

Here it looks like you return an error value if the initial xfs_bulkstat_one() fails and the fallback xfs_bulkstat() succeeds.