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;
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?
For legal waiters on the flag I_FREEING is set which prevents
I_LRU_ISOLATING from showing up afterwards. Thus it should be sufficient
to wait for the flag to clear once. This is moot if spurious wakeups do
happen.
If looping is needed, then this warn:
WARN_ON(inode->i_state & I_LRU_ISOLATING);
fails to test for anything since the loop is on that very condition (iow
it needs to be whacked)
The writeback code needs to loop because there are callers outside of
evict().
