On Fri, Apr 21, 2023 at 12:47:43PM +0200, Jan Kara wrote:
> On Fri 21-04-23 12:20:42, Carlos Maiolino wrote:
> > On Thu, Apr 20, 2023 at 04:39:54PM +0200, Jan Kara wrote:
> > > On Thu 20-04-23 10:03:59, cem@xxxxxxxxxx wrote:
> > > > @@ -3714,6 +3723,50 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> > > > ctx->seen |= SHMEM_SEEN_QUOTA;
> > > > ctx->quota_types |= QTYPE_MASK_GRP;
> > > > break;
> > > > + case Opt_usrquota_block_hardlimit:
> > > > + size = memparse(param->string, &rest);
> > > > + if (*rest || !size)
> > > > + goto bad_value;
> > > > + if (size > SHMEM_QUOTA_MAX_SPC_LIMIT)
> > > > + return invalfc(fc,
> > > > + "User quota block hardlimit too large.");
> > > > + ctx->qlimits.usrquota_bhardlimit = size;
> > > > + ctx->seen |= SHMEM_SEEN_QUOTA;
> > > > + ctx->quota_types |= QTYPE_MASK_USR;
> > >
> > > So if I get it right, the intention here is that if
> > > usrquota_block_hardlimit=value option is used, it automatically enables
> > > user quota accounting and enforcement. I guess it is logical but it is not
> > > documented and I'd prefer to require explicit usrquota mount option to
> > > enable accounting & enforcement - it is then e.g. easier to parse mount
> > > options (in userspace) for finding out whether enforcement is enabled or
> > > not.
> >
> > Hmmm, I think I see what you mean. I can make usrquota_block_hardlimit options
> > to not actually set the quota flag on quota_types, so this should be explicitly
> > set by usrquota/grpquota options. Does that work for you?
>
> Yes, works for me!
Great!
>
> > > Also I can imagine we would allow changing the default limits on
> > > remount but it isn't easy to enable quota accounting on remount etc.
> > >
> >
> > hmm, yes, maybe enabling default limits to be changed on remount isn't a big
> > deal, once the quota is already enabled, so everything is already in place.
>
> Exactly. I don't say you have to do it now as I don't think that is super
> useful. But if there's a demand we can easily do it.
Sounds good :) If you don't mind, I'd postpone it, as I am planning to add
prjquotas to it later, I can add it to the same series, just to avoid adding
more review overhead to this one.
>
> > > > diff --git a/mm/shmem_quota.c b/mm/shmem_quota.c
> > > > index c0b531e2ef688..3cc53f2c35e2c 100644
> > > > --- a/mm/shmem_quota.c
> > > > +++ b/mm/shmem_quota.c
> > > > @@ -166,6 +166,7 @@ static int shmem_acquire_dquot(struct dquot *dquot)
> > > > {
> > > > struct mem_dqinfo *info = sb_dqinfo(dquot->dq_sb, dquot->dq_id.type);
> > > > struct rb_node **n = &((struct rb_root *)info->dqi_priv)->rb_node;
> > > > + struct shmem_sb_info *sbinfo = dquot->dq_sb->s_fs_info;
> > > > struct rb_node *parent = NULL, *new_node = NULL;
> > > > struct quota_id *new_entry, *entry;
> > > > qid_t id = from_kqid(&init_user_ns, dquot->dq_id);
> > > > @@ -195,6 +196,14 @@ static int shmem_acquire_dquot(struct dquot *dquot)
> > > > }
> > > >
> > > > new_entry->id = id;
> > > > + if (dquot->dq_id.type == USRQUOTA) {
> > > > + new_entry->bhardlimit = sbinfo->qlimits.usrquota_bhardlimit;
> > > > + new_entry->ihardlimit = sbinfo->qlimits.usrquota_ihardlimit;
> > > > + } else if (dquot->dq_id.type == GRPQUOTA) {
> > > > + new_entry->bhardlimit = sbinfo->qlimits.grpquota_bhardlimit;
> > > > + new_entry->ihardlimit = sbinfo->qlimits.grpquota_ihardlimit;
> > > > + }
> > > > +
> > > > new_node = &new_entry->node;
> > > > rb_link_node(new_node, parent, n);
> > > > rb_insert_color(new_node, (struct rb_root *)info->dqi_priv);
> > >
> > > Maybe in shmem_dquot_release() when usage is 0 and limits are at default
> > > limits, we can free the structure?
> >
> > hmmm, which struct are you talking about? quota_id? As we do for DQ_FAKE?
>
> Yes.
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
--
Carlos Maiolino
