[PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

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 <jeff.liu@oracle.com>
---
 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(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 33ad9a7..2fdd750 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -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,
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index b5bb7b6..692671c 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -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);
+		}
 	}
-	*done = 0;
-	return 0;
+
+	return error;
 }
 
 int
diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
index 97295d9..60ce988 100644
--- a/fs/xfs/xfs_itable.h
+++ b/fs/xfs/xfs_itable.h
@@ -54,8 +54,7 @@ int
 xfs_bulkstat_single(
 	xfs_mount_t		*mp,
 	xfs_ino_t		*lastinop,
-	char			__user *buffer,
-	int			*done);
+	char			__user *buffer);
 
 typedef int (*bulkstat_one_fmt_pf)(  /* used size in bytes or negative error */
 	void			__user *ubuffer, /* buffer to write to */
-- 
1.8.3.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Mark Tinguely]

On 12/28/13 05:20, Jeff Liu wrote:
> From: Jie Liu<jeff.liu@oracle.com>
>
> 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<jeff.liu@oracle.com>
> ---

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

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Jeff Liu]

On 2013年12月31日 02:35, Mark Tinguely wrote:
> On 12/28/13 05:20, Jeff Liu wrote:
>> From: Jie Liu<jeff.liu@oracle.com>
>>
>> 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<jeff.liu@oracle.com>
>> ---
> 
> 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.

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Dave Chinner]

On Tue, Dec 31, 2013 at 05:51:33PM +0800, Jeff Liu wrote:
> On 2013年12月31日 02:35, Mark Tinguely wrote:
> > On 12/28/13 05:20, Jeff Liu wrote:
> >> From: Jie Liu<jeff.liu@oracle.com>
> >>
> >> 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<jeff.liu@oracle.com>
> >> ---
> > 
> > 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.

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Jeff Liu]

On 01/02 2014 09:12 AM, Dave Chinner wrote:
> On Tue, Dec 31, 2013 at 05:51:33PM +0800, Jeff Liu wrote:
>> On 2013年12月31日 02:35, Mark Tinguely wrote:
>>> On 12/28/13 05:20, Jeff Liu wrote:
>>>> From: Jie Liu<jeff.liu@oracle.com>
>>>>
>>>> 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<jeff.liu@oracle.com>
>>>> ---
>>>
>>> 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.
> 
> 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;
Thanks for the clarification.  Now I can understand the use scenarios
via DMAPI.
> 
>   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...
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.

Thanks,
-Jeff 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Dave Chinner]

On Thu, Jan 02, 2014 at 03:23:46PM +0800, Jeff Liu wrote:
> On 01/02 2014 09:12 AM, Dave Chinner wrote:
> > On Tue, Dec 31, 2013 at 05:51:33PM +0800, Jeff Liu wrote:
> >> On 2013年12月31日 02:35, Mark Tinguely wrote:
> >>> On 12/28/13 05:20, Jeff Liu wrote:
> >>>> From: Jie Liu<jeff.liu@oracle.com>
> >>>>
> >>>> 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<jeff.liu@oracle.com>
> >>>> ---
> >>>
> >>> 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.
> > 
> > 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;
> Thanks for the clarification.  Now I can understand the use scenarios
> via DMAPI.
> > 
> >   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...
> 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);

	} while (XFS_INO_TO_AGNO(mp, lastino) < mp->m_sb.sb_agcount);

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

Right - when you parallelise the quotacheck, then bulkstat
termination conditions are determined by the caller not
xfs_bulkstat(). Hence removing the "done" flag and letting the
callers determine temination conditions via the lastino being
returned seems like the right thing to do as a preparation step.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Brian Foster]

On 12/28/2013 06:20 AM, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> 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 <jeff.liu@oracle.com>
> ---
>  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(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 33ad9a7..2fdd750 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -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,
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index b5bb7b6..692671c 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -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);
> +		}
>  	}
> -	*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.

Brian

>  }
>  
>  int
> diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
> index 97295d9..60ce988 100644
> --- a/fs/xfs/xfs_itable.h
> +++ b/fs/xfs/xfs_itable.h
> @@ -54,8 +54,7 @@ int
>  xfs_bulkstat_single(
>  	xfs_mount_t		*mp,
>  	xfs_ino_t		*lastinop,
> -	char			__user *buffer,
> -	int			*done);
> +	char			__user *buffer);
>  
>  typedef int (*bulkstat_one_fmt_pf)(  /* used size in bytes or negative error */
>  	void			__user *ubuffer, /* buffer to write to */
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

[Jeff Liu]

On 01/04 2014 04:52 AM, Brian Foster wrote:
> On 12/28/2013 06:20 AM, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> 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 <jeff.liu@oracle.com>
>> ---
>>  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(-)
>>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 33ad9a7..2fdd750 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -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,
>> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
>> index b5bb7b6..692671c 100644
>> --- a/fs/xfs/xfs_itable.c
>> +++ b/fs/xfs/xfs_itable.c
>> @@ -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);
>> +		}
>>  	}
>> -	*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.
Good catch, thanks for for your review.

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs