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

Dave Chinner
2010-09-23T00:35:35Z

On Wed, Sep 22, 2010 at 01:24:01PM -0400, Christoph Hellwig wrote:
> > 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.

If we get rid of the special setattr case, then yes, it should be
moved to a transaction specific file.

>
> > /*
> > + * Hit the inode change time.
> > + */
>
> All these comments are utterly pointless. I'd suggest removing them
> when touching the surrounding areas.

Ok, will do.

>
> > +++ 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.

OK.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com