Re: [PATCH RFC v2 5/6] inode: port __I_LRU_ISOLATING to var event

[Christian Brauner <brauner@kernel.org>]

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.