On Thu, Aug 22, 2024 at 11:48:45AM GMT, Mateusz Guzik wrote:
> On Thu, Aug 22, 2024 at 10:53:50AM +0200, Christian Brauner wrote:
> > On Wed, Aug 21, 2024 at 03:41:45PM GMT, Jeff Layton wrote:
> > > > if (inode->i_state & I_LRU_ISOLATING) {
> > > > - DEFINE_WAIT_BIT(wq, &inode->i_state, __I_LRU_ISOLATING);
> > > > - wait_queue_head_t *wqh;
> > > > -
> > > > - wqh = bit_waitqueue(&inode->i_state, __I_LRU_ISOLATING);
> > > > - spin_unlock(&inode->i_lock);
> > > > - __wait_on_bit(wqh, &wq, bit_wait, TASK_UNINTERRUPTIBLE);
> > > > - spin_lock(&inode->i_lock);
> > > > + struct wait_bit_queue_entry wqe;
> > > > + struct wait_queue_head *wq_head;
> > > > +
> > > > + wq_head = inode_bit_waitqueue(&wqe, inode, __I_LRU_ISOLATING);
> > > > + for (;;) {
> > > > + prepare_to_wait_event(wq_head, &wqe.wq_entry,
> > > > + TASK_UNINTERRUPTIBLE);
> > > > + if (inode->i_state & I_LRU_ISOLATING) {
> > > > + spin_unlock(&inode->i_lock);
> > > > + schedule();
> > > > + spin_lock(&inode->i_lock);
> > > > + continue;
> > > > + }
> > > > + break;
> > > > + }
> > >
> > > nit: personally, I'd prefer this, since you wouldn't need the brackets
> > > or the continue:
> > >
> > > if (!(inode->i_state & LRU_ISOLATING))
> > > break;
> > > spin_unlock();
> > > schedule();
> >
> > Yeah, that's nicer. I'll use that.
>
> In that case may I suggest also short-circuiting the entire func?
>
> if (!(inode->i_state & I_LRU_ISOLATING))
> return;
Afaict, if prepare_to_wait_event() has been called finish_wait() must be
called.
>
> then the entire thing loses one indentation level
>
> Same thing is applicable to the I_SYNC flag.
>
> A non-cosmetic is as follows: is it legal for a wake up to happen
> spuriously?
So, I simply mimicked ___wait_var_event() and __wait_on_bit() - both
loop. I reasoned that ___wait_var_event() via @state and __wait_on_bit()
via @mode take the task state into account to wait in. And my conclusion
was that they don't loop on TASK_UNINTERRUPTIBLE but would loop on e.g.,
TASK_INTERRUPTIBLE. Iow, I assume that spurious wakeups shouldn't happen
with TASK_UNINTERRUPTIBLE.
But it's not something I'm able to assert with absolute confidence so I
erred on the side of caution.
