Re: [PATCH v2 5/7] quota: fix dqput() to follow the guarantees dquot_srcu should provide

[Baokun Li]

On 2023/6/29 22:33, Jan Kara wrote:
> 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
This looks great, and the code looks much cleaner, and I'll send out the
next version later containing your suggested changes!

Thank you so much for your patient review!
--
With Best Regards,
Baokun Li
.