Hi Jens,
Thank you for your feedback. Let me clarify the rationale behind this change.
On 2024-06-12 10:54, Jens Axboe wrote:
> It's modified under the lock, and any reader should do so as well. If
> not, there's a race regardless of your change or not.
The existing implementation contains multiple unsynchronized reads of d->flags.
For example, in aoecmd_sleepwork() itself:
void aoecmd_sleepwork(struct work_struct *work)
{
struct aoedev *d = container_of(work, struct aoedev, work);
/* These flag checks are performed WITHOUT LOCKING: */
if (d->flags & DEVFL_GDALLOC) <--- Unsynchronized read
aoeblk_gdalloc(d);
if (d->flags & DEVFL_NEWSIZE) { <--- Unsynchronized read
set_capacity_and_notify(d->gd, d->ssize);
spin_lock_irq(&d->lock);
d->flags |= DEVFL_UP; <--- Two separate writes
d->flags &= ~DEVFL_NEWSIZE; <--- to flags under lock
spin_unlock_irq(&d->lock);
}
}
The problematic sequence would be:
1. Thread A sets DEVFL_UP under lock
2. Thread B (or same thread) reads d->flags without lock, sees DEVFL_UP set but
DEVFL_NEWSIZE still present (before the second write clears it)
3. This could lead to unexpected behavior since DEVFL_NEWSIZE indicates a state
transition that should be atomic with DEVFL_UP setting
By consolidating the flag updates into a single atomic operation under lock:
d->flags = (d->flags | DEVFL_UP) & ~DEVFL_NEWSIZE;
We ensure any unsynchronized reader (like the DEVFL_NEWSIZE check in this same
function) will either see both changes or neither, avoiding intermediate states.
Would you agree this provides stronger consistency guarantees for readers that
(per current code) don't take the lock when checking flags?
Best regards,
Gui-Dong Han
