Re: [PATCH v8 2/5] xfs: Add pquota fields where gquota is used.

[Chandra Seetharaman <sekharan@us.ibm.com>]

On Tue, 2013-06-11 at 09:17 +1000, Dave Chinner wrote:
> On Fri, May 24, 2013 at 04:57:05PM -0500, Chandra Seetharaman wrote:
> > On Fri, 2013-05-17 at 14:23 +1000, Dave Chinner wrote: 
> > > On Fri, May 10, 2013 at 04:21:26PM -0500, Chandra Seetharaman wrote:
> > > > Add project quota changes to all the places where group quota field
> > > > is used:
> > > >    * add separate project quota members into various structures
> > > >    * split project quota and group quotas so that instead of overriding
> > > >      the group quota members incore, the new project quota members are
> > > >      used instead
> > > >    * get rid of usage of the OQUOTA flag incore, in favor of separate
> > > >    * group
> > > >      and project quota flags.
> > > >    * add a project dquot argument to various functions.
> > > > 
> > > > No externally visible interfaces changed.
> > > 
> > > Looks pretty good. Some relatively minor changes below.
> .....
> > > >  		if (code)
> > > >  			return code;
> > > >  	}
> > > > @@ -994,8 +994,8 @@ xfs_ioctl_setattr(
> > > >  		    XFS_IS_PQUOTA_ON(mp) &&
> > > >  		    xfs_get_projid(ip) != fa->fsx_projid) {
> > > >  			ASSERT(tp);
> > > > -			code = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
> > > > -						capable(CAP_FOWNER) ?
> > > > +			code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL,
> > > > +						pdqp, capable(CAP_FOWNER) ?
> > > >  						XFS_QMOPT_FORCE_RES : 0);
> > > > @@ -1113,7 +1113,7 @@ xfs_ioctl_setattr(
> > > >  		if (xfs_get_projid(ip) != fa->fsx_projid) {
> > > >  			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
> > > >  				olddquot = xfs_qm_vop_chown(tp, ip,
> > > > -							&ip->i_gdquot, gdqp);
> > > > +							&ip->i_pdquot, pdqp);
> > > 
> > > xfs_qm_vop_chown() is only called on one dquot at a time, so it can
> > > only exchange one dquot at a time. and udqp is not used for hints,
> > > so it looks to me like udqp and gdqp are wholly unnecessary in this
> > > function....
> > 
> > Did not fully understand this comment. Can you please elaborate ?
> 
> What I was saying here is that throughout the function udqp and gdqp
> are passed to the various quota functions, but they are not used at
> all by the functions being called. i.e. you could pass NULL instead,
> and the code would still function identically. IOWs, we don't
> actually need to define anything other than the project quota
> related variables...

I see...
You want me to get rid of uid/gid usages from this context ? and pid
information from xfs_setattr_nonsize() function ?

> 
> > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > > index d82efaa..7c54ea4 100644
> > > > --- a/fs/xfs/xfs_iops.c
> > > > +++ b/fs/xfs/xfs_iops.c
> > > > @@ -517,7 +517,7 @@ xfs_setattr_nonsize(
> > > >  		ASSERT(udqp == NULL);
> > > >  		ASSERT(gdqp == NULL);
> > > >  		error = xfs_qm_vop_dqalloc(ip, uid, gid, xfs_get_projid(ip),
> > > > -					 qflags, &udqp, &gdqp);
> > > > +					 qflags, &udqp, &gdqp, NULL);
> > > 
> > > Why is there no project quota support here, even though we pass in a
> > > project ID? I know the answer, but please add a comment so that a
> > > couple of years down the track I don't need to go remind myself of
> > > why....
> > 
> > Not clear what you are expecting here.
> 
> I'm expecting you to write a comment explaining why we pass
> uid/gid/projid to the xfs_qm_vop_dqalloc() function, but then only
> pass udqp and gdqp but NULL instead of a project quota dquot
> pointer. Indeed, I already don't remember why this is a valid, so it
> clearly needs a comment explaining it....

>From what I see, we don't even have to send projid information in this
context. Do you want me to get rid of that (as mentioned above) ?

Does this comment sound correct ?
/*
 * Since no project related information is being changed, we do not
 * have to handle project quota information here.
 */

> 
> > > 
> > > 
> > > 			qino = xfs_sb_version_has_pquota(&mp->m_sb) ?
> > > 						mp->m_sb.sb_pquotino :
> > > 						mp->m_sb.sb_gquotino;
> > > 
> > > And the correct on-disk inode is read....
> > > 
> > The object of this patch is to replace all gquota with pquota stuff. I
> > am not using the pquotino or xfs_sb_version_has_pquota() until patch 3.
> > 
> > If I get the change forward, I have to merge patch 3 and part of this
> > patch into patch 1, which adds up lots of work.
> > 
> > The way the patches are split now, IMO, is easy to follow and works
> > independently.
> 
> That's fine. I review the patches one by one, so I make comments
> that are relevant to the current patch context. It may be that you
> changed it in a later patch and that often is fine - comments like
> this indicate that perhaps the commit message hasn't quite described
> the full context of the patch within the series correctly...
> 
> > > > +	if (pdqp) {
> > > > +		ASSERT(ip->i_pdquot == NULL);
> > > > +		ASSERT(XFS_IS_PQUOTA_ON(mp));
> > > > +		ASSERT(xfs_get_projid(ip) == be32_to_cpu(pdqp->q_core.d_id));
> > > > +
> > > > +		ip->i_pdquot = xfs_qm_dqhold(pdqp);
> > > > +		xfs_trans_mod_dquot(tp, pdqp, XFS_TRANS_DQ_ICOUNT, 1);
> > > > +	}
> > > >  }
> > > 
> > > Something this just triggered. All transaction reservations that
> > > reserve space for dquots need to be upped from 2 to 3 dquots.
> > 
> > Can you elaborate please.
> 
> A transaction ithat modifies space can now modify 3 dquots (u+g+p)
> instead of only 2 (u+(g or p)), and so the log space for any such 
> transaction goes up. It may be that you've already handled this, but
> if you're asking for an explanation then maybe not :/

IIUC, I am handling it by increasing the number of array elements in the
data structure xfs_dquot_acct. Let me know if it is incorrect.

> 
> Cheers,
> 
> Dave.


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