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

[Jeff Liu <jeff.liu@oracle.com>]

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