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("a_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.
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.
+ dquot = list_first_entry(&rls_head, struct dquot, dq_free);
+ if (dquot_dirty(dquot)) {
+ spin_unlock(&dq_list_lock);
+ /* Commit dquot before releasing */
+ dquot_write_dquot(dquot);
+ goto restart;
+ }
+ /* Always clear DQ_ACTIVE_B, unless racing with dqget() */
+ if (dquot_active(dquot)) {
+ spin_unlock(&dq_list_lock);
+ dquot->dq_sb->dq_op->release_dquot(dquot);
I'd just go to restart here to make the logic simple. Forward progress is
guaranteed anyway and it isn't really much less efficient.
The rest looks good.
Honza
Thanks!
--
With Best Regards,
Baokun Li
.
