Re: [PATCH 10/14] fs: factor out a sync_lazytime helper

On Fri 14-11-25 07:26:13, Christoph Hellwig wrote:
> Centralize how we synchronize a lazytime update into the actual on-disk
> timestamp into a single helper.
> 
> Signed-off-by: Christoph Hellwig <[email protected]>

...

>  	/*
> -	 * If the inode has dirty timestamps and we need to write them, call
> -	 * mark_inode_dirty_sync() to notify the filesystem about it and to
> -	 * change I_DIRTY_TIME into I_DIRTY_SYNC.
> +	 * For data integrity writeback, or when the dirty interval expired,
> +	 * ask the file system to propagata lazy timestamp updates into real
> +	 * dirty state.
>  	 */
> -	if ((inode->i_state & I_DIRTY_TIME) &&
> -	    (wbc->sync_mode == WB_SYNC_ALL ||
> -	     time_after(jiffies, inode->dirtied_time_when +
> -			dirtytime_expire_interval * HZ))) {
> -		trace_writeback_lazytime(inode);
> -		mark_inode_dirty_sync(inode);
> -	}
> +	if (wbc->sync_mode == WB_SYNC_ALL ||
> +	    time_after(jiffies, inode->dirtied_time_when +
> +			dirtytime_expire_interval * HZ))
> +		sync_lazytime(inode);

The checking of inode->dirtied_time_when for inode potentially without
I_DIRTY_TIME set (and thus with unclear value of dirtied_time_when) is kind
of odd. It is harmless but IMO still not a good practice. Can't we keep
this condition as is and just call sync_lazytime()?

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR