Re: [PATCH 05/11] fs: refactor ->update_time handling

[Jan Kara]

On Tue 06-01-26 08:49:59, Christoph Hellwig wrote:
> Pass the type of update (atime vs c/mtime plus version) as an enum
> instead of a set of flags that caused all kinds of confusion.
> Because inode_update_timestamps now can't return a modified version
> of those flags, return the I_DIRTY_* flags needed to persist the
> update, which is what the main caller in generic_update_time wants
> anyway, and which is suitable for the other callers that only want
> to know if an update happened.
> 
> The whole update_time path keeps the flags argument, which will be used
> to support non-blocking updates soon even if it is unused, and (the
> slightly renamed) inode_update_time also gains the possibility to return
> a negative errno to support this.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

...

> +static int inode_update_cmtime(struct inode *inode)
> +{
> +	struct timespec64 now = inode_set_ctime_current(inode);

This needs to be below sampling of ctime. Otherwise inode dirtying will be
broken...

> +	struct timespec64 ctime = inode_get_ctime(inode);
> +	struct timespec64 mtime = inode_get_mtime(inode);
> +	unsigned int dirty = 0;
> +	bool mtime_changed;
> +
> +	mtime_changed = !timespec64_equal(&now, &mtime);
> +	if (mtime_changed || !timespec64_equal(&now, &ctime))
> +		dirty = inode_time_dirty_flag(inode);
> +	if (mtime_changed)
> +		inode_set_mtime_to_ts(inode, now);
> +
> +	if (IS_I_VERSION(inode) && inode_maybe_inc_iversion(inode, !!dirty))
> +		dirty |= I_DIRTY_SYNC;
> +
> +	return dirty;
> +}

Otherwise the patch looks good to me.

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR