On 3.02.25 9:49, Pavel Tikhomirov wrote:
On 2/3/25 15:42, Alexander Atanasov wrote:
On 3.02.25 9:27, Pavel Tikhomirov wrote:
On 2/3/25 14:45, Alexander Atanasov wrote:
On 3.02.25 8:01, Pavel Tikhomirov wrote:
@@ -166,7 +171,6 @@ static bool ploop_try_delay_enospc(struct
ploop_rq *prq, struct pio *pio)
bool delayed = true;
unsigned long flags;
- spin_lock_irqsave(&ploop->deferred_lock, flags);
if (unlikely(ploop->wants_suspend)) {
delayed = false;
goto unlock;
@@ -176,10 +180,11 @@ static bool ploop_try_delay_enospc(struct
ploop_rq *prq, struct pio *pio)
pr_err_once(PL_FMT("underlying disk is almost full"),
ploop_device_name(ploop));
+ spin_lock_irqsave(&ploop->deferred_lock, flags);
ploop->event_enospc = true;
- list_add_tail(&pio->list, &ploop->enospc_pios);
-unlock:
spin_unlock_irqrestore(&ploop->deferred_lock, flags);
+ llist_add((struct llist_node *)(&pio->list), &ploop-
>enospc_pios);
+unlock:
if (delayed)
mod_timer(&ploop->enospc_timer, jiffies +
PLOOP_ENOSPC_TIMEOUT);
Can you please explain why we need to take defered_lock around
ploop- >event_enospc setting after your patch? (It looks that this
lock does not do anything now.)
see static int ploop_get_event(...), without lock event can be missed
That is not an explanation. How exactly can it be missed?
Other cpu can set "ploop->event_enospc = true" here before lock (i.e. it
was set to true twice), that would lead to emiting only one event for
two sets.
spin_lock_irq(&ploop->deferred_lock);
if (ploop->event_enospc) { <- while emit
other cpu can set
ret = (DMEMIT("event_ENOSPC\n")) ? 1 : 0;
if (ret)
ploop->event_enospc = false; <- next cleaars
here -> second event lost
Not lost, you just emit once for two sets, which as you can see from
above comment is possible even with locks everywhere. So your
explanation does not prove that we need this lock AFAICS.
It is okay to emit once for two events set befere we EMIT.
It is not okay to miss an event set after we emit.
1. Event(s) one or more
2. Start to EMIT
3. Userspace gets first event does what it does to handle it.
4. A new event comes - userspace is not notified
5. End of EMIT
at 4. we have a lost event - an event userspace will not receive.
Cost is minimal as we do not expect to see much of this so the
correctness is more important here.
And this can happen for sure - we have a lots of writes , they get
queued. Userspace frees some space , writes are retried but again space
is over because they need more space than it is freed. Without the
second event we get stuck here, userspace will not free more space.
And pios will hang an retry.
}
spin_unlock_irq(&ploop->deferred_lock);
--
Regards,
Alexander Atanasov
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
