On Wed 10-06-20 08:11:41, Christoph Hellwig wrote:
> On Mon, Jun 01, 2020 at 11:18:57AM +0200, Jan Kara wrote:
> > The only use of I_DIRTY_TIME_EXPIRE is to detect in
> > __writeback_single_inode() that inode got there because flush worker
> > decided it's time to writeback the dirty inode time stamps (either
> > because we are syncing or because of age). However we can detect this
> > directly in __writeback_single_inode() and there's no need for the
> > strange propagation with I_DIRTY_TIME_EXPIRE flag.
>
> Looks good:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>
> One nit below:
>
> > if (inode->i_state & I_DIRTY_TIME) {
> > if ((dirty & I_DIRTY_INODE) ||
> > - wbc->sync_mode == WB_SYNC_ALL ||
> > - unlikely(inode->i_state & I_DIRTY_TIME_EXPIRED) ||
> > + wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
> > unlikely(time_after(jiffies,
> > (inode->dirtied_time_when +
> > dirtytime_expire_interval * HZ)))) {
> > - dirty |= I_DIRTY_TIME | I_DIRTY_TIME_EXPIRED;
> > + dirty |= I_DIRTY_TIME;
> > trace_writeback_lazytime(inode);
> > }
> > - } else
> > - inode->i_state &= ~I_DIRTY_TIME_EXPIRED;
> > + }
>
> We can also drop some indentation here. And remove the totally silly
> unlikely, something like:
>
> if ((inode->i_state & I_DIRTY_TIME) &&
> ((dirty & I_DIRTY_INODE) ||
> wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
> time_after(jiffies, inode->dirtied_time_when +
> dirtytime_expire_interval * HZ)))) {
> dirty |= I_DIRTY_TIME;
> trace_writeback_lazytime(inode);
> }
Sure, I've done this. Once fstests run passes, I'll send v2 (likely
tomorrow).
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
