Re: [PATCH 4/6] shmem: prepare shmem quota infrastructure

[Carlos Maiolino <cem@kernel.org>]

Hi Honza.

> > +	if (!dquot->dq_dqb.dqb_bhardlimit &&
> > +	    !dquot->dq_dqb.dqb_bsoftlimit &&
> > +	    !dquot->dq_dqb.dqb_ihardlimit &&
> > +	    !dquot->dq_dqb.dqb_isoftlimit)
> > +		set_bit(DQ_FAKE_B, &dquot->dq_flags);
> > +	spin_unlock(&dquot->dq_dqb_lock);
> > +
> > +	/* Make sure flags update is visible after dquot has been filled */
> > +	smp_mb__before_atomic();
> > +	set_bit(DQ_ACTIVE_B, &dquot->dq_flags);
> 
> I'm slightly wondering whether we shouldn't have a dquot_mark_active()
> helper for this to hide the barrier details...

This sounds good to me, would be ok for you if I simply add this to my todo
list, and do it once this series is merged? I'd rather avoid to add more patches
to the series now adding more review overhead.
I can send a new patch later creating a new helper and replacing all
set_bit(DQ_ACTIVE_B, ...) calls with the new helper.

> 
> > +out_unlock:
> > +	up_write(&dqopt->dqio_sem);
> > +	mutex_unlock(&dquot->dq_lock);
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Store limits from dquot in the tree unless it's fake. If it is fake
> > + * remove the id from the tree since there is no useful information in
> > + * there.
> > + */
> > +static int shmem_release_dquot(struct dquot *dquot)
> > +{
> > +	struct mem_dqinfo *info = sb_dqinfo(dquot->dq_sb, dquot->dq_id.type);
> > +	struct rb_node *node = ((struct rb_root *)info->dqi_priv)->rb_node;
> > +	qid_t id = from_kqid(&init_user_ns, dquot->dq_id);
> > +	struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
> > +	struct quota_id *entry = NULL;
> > +
> > +	mutex_lock(&dquot->dq_lock);
> > +	/* Check whether we are not racing with some other dqget() */
> > +	if (dquot_is_busy(dquot))
> > +		goto out_dqlock;
> > +
> > +	down_write(&dqopt->dqio_sem);
> > +	while (node) {
> > +		entry = rb_entry(node, struct quota_id, node);
> > +
> > +		if (id < entry->id)
> > +			node = node->rb_left;
> > +		else if (id > entry->id)
> > +			node = node->rb_right;
> > +		else
> > +			goto found;
> > +	}
> > +
> > +	up_write(&dqopt->dqio_sem);
> > +	mutex_unlock(&dquot->dq_lock);
> 
> We should report some kind of error here, shouldn't we? We do expect to
> have the quota_id allocated from shmem_acquire_dquot() and we will be
> possibly loosing set limits here.
> 

Sounds correct, I'll update it once we agree on how to proceed with your above
suggestion of dquot_mark_active().

> Otherwise the patch looks good to me.
> 
> 								Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

-- 
Carlos Maiolino