On Wed, Nov 23, 2022 at 05:37:45PM +0100, Jan Kara wrote:
> On Mon 21-11-22 15:28:53, Lukas Czerner wrote:
> > Implement user and group quota support for tmpfs using system quota file
> > in vfsv0 quota format. Because everything in tmpfs is temporary and as a
> > result is lost on umount, the quota files are initialized on every
> > mount. This also goes for quota limits, that needs to be set up after
> > every mount.
> >
> > The quota support in tmpfs is well separated from the rest of the
> > filesystem and is only enabled using mount option -o quota (and
> > usrquota and grpquota for compatibility reasons). Only quota accounting
> > is enabled this way, enforcement needs to be enable by regular quota
> > tools (using Q_QUOTAON ioctl).
> >
> > Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
>
> ...
>
> > diff --git a/Documentation/filesystems/tmpfs.rst b/Documentation/filesystems/tmpfs.rst
> > index 0408c245785e..9c4f228ef4f3 100644
> > --- a/Documentation/filesystems/tmpfs.rst
> > +++ b/Documentation/filesystems/tmpfs.rst
> > @@ -86,6 +86,18 @@ use up all the memory on the machine; but enhances the scalability of
> > that instance in a system with many CPUs making intensive use of it.
> >
> >
> > +tmpfs also supports quota with the following mount options
> > +
> > +======== =============================================================
> > +quota Quota accounting is enabled on the mount. Tmpfs is using
> > + hidden system quota files that are initialized on mount.
> > + Quota limits can quota enforcement can be enabled using
> ^^^ and?
>
> > + standard quota tools.
> > +usrquota Same as quota option. Exists for compatibility reasons.
> > +grpquota Same as quota option. Exists for compatibility reasons.
>
> As we discussed with V1, I'd prefer if user & group quotas could be enabled
> / disabled independently. Mostly to not differ from other filesystems
> unnecessarily.
Ok, but other file systems (at least xfs and ext) differs. Mounting ext4
file system with quota feature with default quota option settings will
always enable accounting for both user and group. Mount options quota,
usrquota and grpquota enables enforcement; selectively with the last
two.
On xfs with no mount options quota is disabled. With quota, usrquota and
grpquota enforcement is enabled, again selectively with the last two.
And yes, with this implementation tmpfs is again different. The idea was
to allow enabling accounting and enforcement (with default limits)
selectively.
So how would you like the tmpfs to do it? I think having accounting only
can be useful and I'd like to keep it. Maybe adding qnoenforce,
uqnoenforce and qgnoenforce mount options, but that seems cumbersome to
me and enabling accounting by default seems a bit much. What do you think?
>
> > +======== =============================================================
> > +
> > +
> > tmpfs has a mount option to set the NUMA memory allocation policy for
> > all files in that instance (if CONFIG_NUMA is enabled) - which can be
> > adjusted on the fly via 'mount -o remount ...'
> > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> > index f1a7a03632a2..007604e7eb09 100644
> > --- a/fs/quota/dquot.c
> > +++ b/fs/quota/dquot.c
> > @@ -716,11 +716,11 @@ int dquot_quota_sync(struct super_block *sb, int type)
> > for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> > if (type != -1 && cnt != type)
> > continue;
> > - if (!sb_has_quota_active(sb, cnt))
> > - continue;
> > - inode_lock(dqopt->files[cnt]);
> > - truncate_inode_pages(&dqopt->files[cnt]->i_data, 0);
> > - inode_unlock(dqopt->files[cnt]);
> > + if (sb_has_quota_active(sb, cnt) && dqopt->files[cnt]) {
> > + inode_lock(dqopt->files[cnt]);
> > + truncate_inode_pages(&dqopt->files[cnt]->i_data, 0);
> > + inode_unlock(dqopt->files[cnt]);
> > + }
> > }
>
> No need to mess with this when you have DQUOT_QUOTA_SYS_FILE set.
Ah right, there is this condition earlier in the function. I'll drop it.
if (dqopt->flags & DQUOT_QUOTA_SYS_FILE)
return 0;
>
> > +/*
> > + * We don't have any quota files to read, or write to/from, but quota code
> > + * requires .quota_read and .quota_write to exist.
> > + */
> > +static ssize_t shmem_quota_write(struct super_block *sb, int type,
> > + const char *data, size_t len, loff_t off)
> > +{
> > + return len;
> > +}
> > +
> > +static ssize_t shmem_quota_read(struct super_block *sb, int type, char *data,
> > + size_t len, loff_t off)
> > +{
> > + return len;
> > +}
>
> Instead of these functions I'd go for attached patch.
Ok, I'll take a look.
>
> > @@ -363,7 +438,7 @@ bool shmem_charge(struct inode *inode, long pages)
> > struct shmem_inode_info *info = SHMEM_I(inode);
> > unsigned long flags;
> >
> > - if (!shmem_inode_acct_block(inode, pages))
> > + if (shmem_inode_acct_block(inode, pages))
> > return false;
>
> As Brian asked, I'd prefer to have the calling convention change as a
> separate patch.
Alright I'll split it up.
>
> > +static struct inode *shmem_get_inode(struct super_block *sb, struct inode *dir,
> > + umode_t mode, dev_t dev, unsigned long flags)
> > +{
> > + int err;
> > + struct inode *inode;
> > +
> > + inode = shmem_get_inode_noquota(sb, dir, mode, dev, flags);
> > + if (inode) {
> > + err = dquot_initialize(inode);
> > + if (err)
> > + goto errout;
> > +
> > + err = dquot_alloc_inode(inode);
> > + if (err) {
> > + dquot_drop(inode);
> > + goto errout;
> > + }
> > + }
> > + return inode;
>
> I'd rather make shmem_get_inode() always return ERR_PTR or inode pointer.
> It's more natural convention. Also this calling convention change should
> go into a separate patch.
ok.
Thanks!
-Lukas
>
> > +
> > +errout:
> > + inode->i_flags |= S_NOQUOTA;
> > + iput(inode);
> > + shmem_free_inode(sb);
> > + if (err)
> > + return ERR_PTR(err);
> > + return NULL;
> > +}
> > +
> ...
>
> > @@ -4196,8 +4348,10 @@ static struct file *__shmem_file_setup(struct vfsmount *mnt, const char *name, l
> >
> > inode = shmem_get_inode(mnt->mnt_sb, NULL, S_IFREG | S_IRWXUGO, 0,
> > flags);
> > - if (unlikely(!inode)) {
> > + if (IS_ERR_OR_NULL(inode)) {
> > shmem_unacct_size(flags, size);
> > + if (IS_ERR(inode))
> > + return (struct file *)inode;
> ^^ Uhuh. ERR_CAST()?
>
> > return ERR_PTR(-ENOSPC);
> > }
> > inode->i_flags |= i_flags;
>
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
