Re: [PATCH 3/3] writeback: Drop I_DIRTY_TIME_EXPIRE

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

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);
	}