On Thu, Aug 22, 2024 at 01:10:43PM +0200, Christian Brauner wrote:
> 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.
>
I mean the upfront check, before any other work:
static void inode_wait_for_lru_isolating(struct inode *inode)
{
lockdep_assert_held(&inode->i_lock);
if (!(inode->i_state & I_LRU_ISOLATING))
return;
/* for loop goes here, losing an indentation level but otherwise
* identical. same remark for the writeback thing */
}
> >
> > 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.
This definitely should be sorted out that callers don't need to loop if
the condition is a one off in the worst case, but I concede getting
there is not *needed* at the moment, just a fixme.
