[PATCH] honor group quota if user quota also exists

[Björn JACKE]

On 2019-02-02 at 00:15 +0200 Uri Simchoni sent off:
> It's not quite clear to me what the algorithm should be:
> - Do we present free disk based on which one (user/group) has less free
> disk?
> - Do we present free disk based on which one (user/group) has less quota?
> 
> The argument for the first way is that it reflects the free disk, The
> argument for the second way is that the "disk size" is stable.
> 
> It makes me think that maybe the code should remain as-is, and recommend
> to set either user quota *or* group quota, but not both.

I know it's difficult to decide what is the best to do here. Leaving it as it
is now is the worst through. We should definetely show the min value of the
left free space, no matter if it's user or group quota. If one of them reaches
the limit, then the left free quota space reported to the user should also
reach the zero line.

Recommending not to set both is a good idea but we need to hande the case when
both are set as graceful as possible :-)


> Having said that, I assume we're using the second method, because that's
> what the code seems to be trying to do, and because I don't like the
> disk size to be "jumping around" if files are created/destroyed,
> although it may still produce free-disk that's not accurate.
> 
> I also think it's quite easy to add a test for this - we have
> test_dfree_quota.sh which can probably be extended to handle this case.

I'm not sure what you mean here exactly


> > +	bool ret_user_quota = False;
> 
> We use true/false now - this routine already has a mix of old and new
> style, so better (IMHO) to use new style and maybe follow up with a
> patch to convert the rest.
> 
> >  
> >  	/*
> >  	 * First of all, check whether user quota is
> > @@ -428,7 +429,7 @@ bool disk_quotas(connection_struct *conn, struct smb_filename *fname,
> >  		*dsize = D.softlimit;
> >  	}
> >  
> > -	return True;
> > +	ret_user_quota = True;
> >  	
> >  try_group_quota:
> >  	/*
> > @@ -441,10 +442,10 @@ try_group_quota:
> >  	r = SMB_VFS_GET_QUOTA(conn, fname, SMB_GROUP_FS_QUOTA_TYPE,
> >  			      id, &D);
> >  	if (r == -1 && errno != ENOSYS) {
> > -		return false;
> > +		return ret_user_quota;
> >  	}
> >  	if (r == 0 && (D.qflags & QUOTAS_DENY_DISK) == 0) {
> > -		return false;
> > +		return ret_user_quota;
> >  	}
> >  
> >  	id.gid = getegid();
> > @@ -454,7 +455,7 @@ try_group_quota:
> >  			      &D);
> >  
> >  	if (r == -1) {
> > -		return False;
> > +		return ret_user_quota;
> >  	}
> >  
> >  	*bsize = D.bsize;
> > @@ -468,13 +469,16 @@ try_group_quota:
> >  		*dfree = 0;
> >  		*dsize = D.curblocks;
> >  	} else if (D.softlimit==0 && D.hardlimit==0) {
> > -		return False;
> > +		return ret_user_quota;
> >  	} else {
> >  		if (D.softlimit == 0) {
> >  			D.softlimit = D.hardlimit;
> >  		}
> > -		*dfree = D.softlimit - D.curblocks;
> > -		*dsize = D.softlimit;
> > +		/* let's see if user quota or group quota is more limiting: */
> > +		if (*dfree > D.softlimit) {
> 
> That should be *dsize right?
> 
> Additionally, if the user quota wasn't successful, *dsize/*dfree are not
> initialized (at least by this function). Perhaps a better approach would be:
> 1. get the group quota
> 2. decide which of the two to use
> 3. set all three output parameters based on the chosen quota

yes, metze proposed something like this also today...

Thanks
Björn