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

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

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