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

[Dave Chinner <david@fromorbit.com>]

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