[patchset] quota fixes, cleanups and sysquotas for AIX

[Björn JACKE]

On 2019-02-13 at 23:24 +0200 Uri Simchoni via samba-technical sent off:
> 6/13 - A consequence of this change is that setting the
> storage-quota/inode-limit ratio depends on compile-time platform, which
> is probably not desirable (see vfs_set_ntquota() and how inode limit is
> calculated). Perhaps the ratio should have its own constant (I'm not
> sure what's the rationale of allowing 2K per inode in the original code).

I think the we should remove this artifical inode ratio limit entirely. Windows
has no inode quota limit settings here and we should not introduce such limits
out of nothing.


> Also:
> > +#elif defined(HAVE_UFS_UFS_QUOTA_H)           /* BSDs */
> > +#define QUOTABLOCK_SIZE 512
> 
> In the 4B code before change, DEV_BSIZE is used. Maybe 512 is just as
> good...

DEV_BSIZE is just wrong for the quota block size. The device bsize is the same
as the quota block size on the BSDs co-incendently. DEV_BSIZE existst on a lot
of platforms like Linux and AIX also and is different there from the quota
block size. So it's better to just set the number here as there is no define
there.

> 7/13 - RB+ me
> 8/13:
> > +#ifdef HAVE_STRUCT_DQBLK_DQB_CURBYTES
> > +/* we handle the byte vs. block count dynamically via QUOTABLOCK_SIZE 1 */
> > +#define dqb_curbytes dqb_curblocks
> > +#endif
> > +
> 
> Shouldn't it be the other way around? Our code is using dqb_curblocks,
> and Darwin recognizes dqb_curbytes...

good catch! fixed it.


> > +#ifdef HAVE_STRUCT_DQBLK_DQB_CURBYTES
> > +	/* we want Darwin 0 byte files not to take 0 space, set a minimum of 1 here */
> > +	if (dp->curblocks == 0) {
> > +		dp->curblocks = 1;
> > +	}
> 
> I don't get this - what's wrong with taking zero space?

well, this was coming from the original patch from apple, this is what the + 1
was previously in the code. I don't know if there was a failure case or it was
just a gut feeling of precaution which made them put the +1 there. On the other
hand we have byte-count on Linux here also and we do not handle 0byte files
differently there. I suggest I just comment this hunk out to have it documented
somehow, do you agree?


> 9/13 - JFS - I suppose it works so RB+ me
> 
> 10/13 JFS2: - looks OK. I wish we were more consistent in handling of
> the QUOTAS_ENABLED / QUOTAS_DENY_DISK flags - some drivers return an
> error to signal they are not supported and some return success with
> flags cleared. That'll wait for another day I guess...
> 
> 11/13 - RB+ me
> 
> 12/13 RB+ me
> 
> 13/13:
> > +		/* let's see if user quota or group quota is more limiting: */
> > +		if (*dsize > D.softlimit) {
> > +			*dfree = D.softlimit - D.curblocks;
> > +			*dsize = D.softlimit;
> > +		
> 
> My concern here is that we count on the caller to set *dsize to 0 before
> calling us, which is not expected for an output parameter. If the user
> quota check fails, and goto try_group_quota is executed, then we don't
> set *dsize, and then compare whatever's there. It so happens that the
> caller initializes this to zero because of Samba coding conventions, but
> I don't think this convention is universal enough. To honor the UNIX
> tradition that out parameters are not modified on error we need a local
> variable set to 0, and then set to user disk size if that's successful.

yes, will work that over ...


> It would also be nice to add tests for this case where both group and
> user quota are defined - see test_dfree_quota.sh - I think it amounts to
> adding a couple of lines, no need for additional infrastructure. Best
> thing is added test + knownfail first, then fix and removal from knownfail.

I'll look into that.

Thanks!
Björn