Re: [PATCH 04/16] xfs: don't use vfs writeback for pure metadata modifications

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 04/16] xfs: don't use vfs writeback for pure metadata modifications
Date: Wed, 22 Sep 2010 13:24:01 -0400	[thread overview]
Message-ID: <20100922172401.GB5697@infradead.org> (raw)
In-Reply-To: <1285137869-10310-5-git-send-email-david@fromorbit.com>

> However, the timstamp changes are slightly more complex than this -
> there are a couple of places that do unlogged updates of the
> timestamps, and the VFS need to be informed of these. Hence add a
> new function xfs_trans_inode_chgtime() for transactional changes,
> and leave xfs_ichgtime() for the non-transactional changes.

The only user of xfs_ichgtime after this patch is a special case in
truncate for the case of a zero-sized file that's also truncated to size
zero.  I think we should just remove this special case and not require
xfs_ichgtime at all.  I'll prepare patches to clean up xfs_setattr
and remove this non-transaction update and once this patch is rebased
ontop of that it can be simplied again.

That leaves the timestamp updates from the data I/O path special as
they still get updated via direct writes to inode->i_*time and
mark_inode_dirty.  I guess we'll have to live with that for now.

> + * Transactional inode timestamp update. requires inod to be locked and joined
> + * to the transaction supplied. Relies on the transaction subsystem to track
> + * dirty state and update/writeback the inode accordingly.

s/inod/the inode/

Also I wonder if xfs_trans_ichgtime should be in xfs_trans_inode.c with
a prototype in xfs_trans.h, just like all the other xfs_trans*
functions.

>  	/*
> +	 * Hit the inode change time.
> +	 */

All these comments are utterly pointless.  I'd suggest removing them
when touching the surrounding areas.

> +++ b/fs/xfs/xfs_inode_item.c
> @@ -223,15 +223,6 @@ xfs_inode_item_format(
>  	nvecs	     = 1;
>  
>  	/*
> -	 * Make sure the linux inode is dirty. We do this before
> -	 * clearing i_update_core as the VFS will call back into
> -	 * XFS here and set i_update_core, so we need to dirty the
> -	 * inode first so that the ordering of i_update_core and
> -	 * unlogged modifications still works as described below.
> -	 */
> -	xfs_mark_inode_dirty_sync(ip);
> -

With this gone the comment above xfs_fs_dirty_inode will need an update.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs