Re: [PATCH 1/8] quota: Allow to pass mount path to quotactl

Re: [PATCH 1/8] quota: Allow to pass mount path to quotactl

[Jan Kara]

On Tue 26-01-21 11:45:57, Sascha Hauer wrote:
> On Mon, Jan 25, 2021 at 04:45:07PM +0100, Jan Kara wrote:
> > On Mon 25-01-21 09:38:54, Sascha Hauer wrote:
> > > On Fri, Jan 22, 2021 at 05:16:58PM +0000, Christoph Hellwig wrote:
> > > > On Fri, Jan 22, 2021 at 04:15:29PM +0100, Sascha Hauer wrote:
> > > > > This patch introduces the Q_PATH flag to the quotactl cmd argument.
> > > > > When given, the path given in the special argument to quotactl will
> > > > > be the mount path where the filesystem is mounted, instead of a path
> > > > > to the block device.
> > > > > This is necessary for filesystems which do not have a block device as
> > > > > backing store. Particularly this is done for upcoming UBIFS support.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > 
> > > > I hate overloading quotactl even more.  Why not add a new quotactl_path
> > > > syscall instead?
> > > 
> > > We can probably do that. Honza, what do you think?
> > 
> > Hum, yes, probably it would be cleaner to add a new syscall for this so
> > that we don't overload quotactl(2). I just didn't think of this.
> 
> How should the semantics of that new syscall look like?
> 
> The easiest and most obvious way would be to do it like the quotactl(2)
> and just replace the special argument with a path:
> 
> int quotactl_path(int cmd, const char *path, int id, caddr_t addr);

Yes, that's what I meant.

> If we try adding a new syscall then we could completely redefine the API
> and avoid the shortcomings of the original quotactl(2) if there are any.
> Can you foresee the discussions we end up in? I am afraid I am opening a
> can of worms here.
> OTOH there might be value in keeping the new syscall compatible to the
> existing one, but I don't know how much this argument counts.

That's a good question but also a can of worms as you write :). One obvious
problem with quotactl() is that's it's ioctl-like interface. So we have
several different operations mixed into a single syscall. Currently there
are these operations:

#define Q_SYNC     0x800001     /* sync disk copy of a filesystems quotas */
#define Q_QUOTAON  0x800002     /* turn quotas on */
#define Q_QUOTAOFF 0x800003     /* turn quotas off */
#define Q_GETFMT   0x800004     /* get quota format used on given filesystem */
#define Q_GETINFO  0x800005     /* get information about quota files */
#define Q_SETINFO  0x800006     /* set information about quota files */
#define Q_GETQUOTA 0x800007     /* get user quota structure */
#define Q_SETQUOTA 0x800008     /* set user quota structure */
#define Q_GETNEXTQUOTA 0x800009 /* get disk limits and usage >= ID */
<plus their XFS variants>

In a puristic world they'd be 9 different syscalls ... or somewhat less
because Q_GETNEXTQUOTA is a superset of Q_GETQUOTA, we could drop Q_SYNC
and Q_GETFMT because they have dubious value these days so we'd be left
with 6. I don't have a strong opinion whether 6 syscalls are worth the
cleanliness or whether we should go with just one new quotactl_path()
syscall. I've CCed linux-api in case other people have opinion.

Anyway, even if we go with single quotactl_path() syscall we should remove
the duplication between VFS and XFS quotactls when we are creating a new
syscall. Thoughts?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/8] quota: Allow to pass mount path to quotactl

[Christoph Hellwig]

On Wed, Jan 27, 2021 at 03:46:46PM +0100, Jan Kara wrote:
> In a puristic world they'd be 9 different syscalls ... or somewhat less
> because Q_GETNEXTQUOTA is a superset of Q_GETQUOTA, we could drop Q_SYNC
> and Q_GETFMT because they have dubious value these days so we'd be left
> with 6. I don't have a strong opinion whether 6 syscalls are worth the
> cleanliness or whether we should go with just one new quotactl_path()
> syscall. I've CCed linux-api in case other people have opinion.
> 
> Anyway, even if we go with single quotactl_path() syscall we should remove
> the duplication between VFS and XFS quotactls when we are creating a new
> syscall. Thoughts?

I thunk the multiplexer is just fine.  We don't really need Q_SYNC
for a new syscall.  For XFS vs classic VFS quota we probably don't
need to duplicate the two data structure, but we need to make sure we
catch the superset of the information if we want to disable the old
ones.

So I suspect just supporting evrything excpt for the global Q_SYNC
and reusing do_quotactl as-is is the most maintainable and easiest
to understand way forward.