Re: [PATCH 1/2] quota: Add mountpath based quota support

[Jan Kara]

On Thu 04-02-21 07:34:14, Christoph Hellwig wrote:
> On Tue, Feb 02, 2021 at 07:02:41PM +0100, Jan Kara wrote:
> > Hum, let me think out loud. The path we pass to Q_QUOTAON is a path to
> > quota file - unless the filesystem stores quota in hidden files in which
> > case this argument is just ignored. You're right we could require that
> > specifically for Q_QUOTAON, the mountpoint path would actually need to
> > point to the quota file if it is relevant, otherwise anywhere in the
> > appropriate filesystem. We don't allow quota file to reside on a different
> > filesystem (for a past decade or so) so it should work fine.
> > 
> > So the only problem I have is whether requiring the mountpoint argument to
> > point quota file for Q_QUOTAON isn't going to be somewhat confusing to
> > users. At the very least it would require some careful explanation in the
> > manpage to explain the difference between quotactl_path() and quotactl()
> > in this regard. But is saving the second path for Q_QUOTAON really worth the
> > bother?
> 
> I find the doubled path argument a really horrible API, so I'd pretty
> strongly prefer to avoid that.

Honestly, I don't understand why is it so horrible. The paths point to
different things... The first path identifies the filesystem to operate on,
the second path identifies the file which contains quota accounting data.
In the ancient times, the file with quota accounting data could even be
stored on a different filesystem (these were still times when filesystem
metadata journalling was a new thing - like late 90's).  But later I just
disallowed that because it was not very useful (and luckily even used) and
just complicated matters.
 
Anyway, back to 2021 :). What I find somewhat confusing about a single path
for Q_QUOTAON is that for any other quotactl, any path on the filesystem is
fine. Similarly if quota data is stored in the hidden file, any path on the
filesystem is fine. It is only for Q_QUOTAON on a filesystem where quota
data is stored in a normal file, where we suddently require that the path
has to point to it.

Now quota data stored in a normal file is a setup we try to deprecate
anyway so another option is to just leave quotactl_path() only for those
setups where quota metadata is managed by the filesystem so we don't need
to pass quota files to Q_QUOTAON?

> > > Given how cheap quotactl_cmd_onoff and quotactl_cmd_write are we
> > > could probably simplify this down do:
> > > 
> > > 	if (quotactl_cmd_write(cmd)) {
> > 
> > This needs to be (quotactl_cmd_write(cmd) || quotactl_cmd_onoff(cmd)).
> > Otherwise I agree what you suggest is somewhat more readable given how
> > small the function is.
> 
> The way I read quotactl_cmd_write, it only special cases a few commands
> and returns 0 there, so we should not need the extra quotactl_cmd_onoff
> call, as all those commands are not explicitly listed.

Right, sorry, I was mistaken.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR