Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 16:33:04 +0200
From:   Jan Kara <jack@suse.cz>
To:     Baokun Li <libaokun1@huawei.com>
Cc:     Jan Kara <jack@suse.cz>, linux-fsdevel@vger.kernel.org,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, yangerkun@huawei.com, chengzhihao1@huawei.com,
        yukuai3@huawei.com
Subject: Re: [PATCH v2 5/7] quota: fix dqput() to follow the guarantees
 dquot_srcu should provide
Message-ID: <20230629143304.2t45zta3f57imowa@quack3>
References: <20230628132155.1560425-1-libaokun1@huawei.com>
 <20230628132155.1560425-6-libaokun1@huawei.com>
 <20230629105954.5cpqpch46ik4bg27@quack3>
 <9ac4fdcf-f236-8a05-bb96-b0b85a63b54e@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9ac4fdcf-f236-8a05-bb96-b0b85a63b54e@huawei.com>
Precedence: bulk

On Thu 29-06-23 19:47:08, Baokun Li wrote:
> On 2023/6/29 18:59, Jan Kara wrote:
> > On Wed 28-06-23 21:21:53, Baokun Li wrote:
> > > @@ -760,6 +771,8 @@ dqcache_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
> > >   	struct dquot *dquot;
> > >   	unsigned long freed = 0;
> > > +	flush_delayed_work(&quota_release_work);
> > > +
> > I would not flush the work here. Sure, it can make more dquots available
> > for reclaim but I think it is more important for the shrinker to not wait
> > on srcu period as shrinker can be called very frequently under memory
> > pressure.
> This is because I want to use remove_free_dquot() directly, and if I don't
> do
> flush here anymore, then DQST_FREE_DQUOTS will not be accurate.
> Since that's the case, I'll remove the flush here and add a determination
> to remove_free_dquot() whether to increase DQST_FREE_DQUOTS.

OK.

> > >   	spin_lock(&dq_list_lock);
> > >   	while (!list_empty(&free_dquots) && sc->nr_to_scan) {
> > >   		dquot = list_first_entry(&free_dquots, struct dquot, dq_free);
> > > @@ -787,6 +800,60 @@ static struct shrinker dqcache_shrinker = {
> > >   	.seeks = DEFAULT_SEEKS,
> > >   };
> > > +/*
> > > + * Safely release dquot and put reference to dquot.
> > > + */
> > > +static void quota_release_workfn(struct work_struct *work)
> > > +{
> > > +	struct dquot *dquot;
> > > +	struct list_head rls_head;
> > > +
> > > +	spin_lock(&dq_list_lock);
> > > +	/* Exchange the list head to avoid livelock. */
> > > +	list_replace_init(&releasing_dquots, &rls_head);
> > > +	spin_unlock(&dq_list_lock);
> > > +
> > > +restart:
> > > +	synchronize_srcu(&dquot_srcu);
> > > +	spin_lock(&dq_list_lock);
> > > +	while (!list_empty(&rls_head)) {
> > I think the logic below needs a bit more work. Firstly, I think that
> > dqget() should removing dquots from releasing_dquots list - basically just
> > replace the:
> > 	if (!atomic_read(&dquot->dq_count))
> > 		remove_free_dquot(dquot);
> > with
> > 	/* Dquot on releasing_dquots list? Drop ref kept by that list. */
> > 	if (atomic_read(&dquot->dq_count) == 1 && !list_empty(&dquot->dq_free))
> > 		atomic_dec(&dquot->dq_count);
> > 	remove_free_dquot(dquot);
> > 	atomic_inc(&dquot->dq_count);
> > 
> > That way we are sure that while we are holding dq_list_lock, all dquots on
> > rls_head list have dq_count == 1.
> I wrote it this way at first, but that would have been problematic, so I
> ended up dropping the dq_count == 1 constraint for dquots on
> releasing_dquots.  Like the following, we will get a bad dquot directly:
> 
> quota_release_workfn
> ?spin_lock(&dq_list_lock)
> ?dquot = list_first_entry(&rls_head, struct dquot, dq_free)
> ?spin_unlock(&dq_list_lock)
> ?dquot->dq_sb->dq_op->release_dquot(dquot)
> ?release_dquot
> ?????? dqget
> ??????? atomic_dec(&dquot->dq_count)
> ??????? remove_free_dquot(dquot)
> ??????? atomic_inc(&dquot->dq_count)
> ??????? spin_unlock(&dq_list_lock)
> ??????? wait_on_dquot(dquot)
> ??????? if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))
> ??????? // still active
> ?mutex_lock(&dquot->dq_lock)
> ?dquot_is_busy(dquot)
> ? atomic_read(&dquot->dq_count) > 1
> ?clear_bit(DQ_ACTIVE_B, &dquot->dq_flags)
> ?mutex_unlock(&dquot->dq_lock)
> 
> Removing dquot from releasing_dquots and its reduced reference count
> will cause dquot_is_busy() in dquot_release to fail. wait_on_dquot(dquot)
> in dqget would have no effect. This is also the reason why I did not restart
> at dquot_active. Adding dquot to releasing_dquots only in dqput() and
> removing dquot from releasing_dquots only in quota_release_workfn() is
> a simple and effective way to ensure consistency.

Indeed, that's a good point. Still cannot we simplify the loop like:

	while (!list_empty(&rls_head)) {
		dquot = list_first_entry(&rls_head, struct dquot, dq_free);
		/* Dquot got used again? */
		if (atomic_read(&dquot->dq_count) > 1) {
			atomic_dec(&dquot->dq_count);
			remove_free_dquot(dquot);
			continue;
		}
		if (dquot_dirty(dquot)) {
			keep what you had
		}
		if (dquot_active(dquot)) {
			spin_unlock(&dq_list_lock);
			dquot->dq_sb->dq_op->release_dquot(dquot);
			goto restart;
		}
		/* Dquot is inactive and clean, we can move it to free list */
		atomic_dec(&dquot->dq_count);
		remove_free_dquot(dquot);
		put_dquot_last(dquot);
	}

What do you think?
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR