xfs
[Top] [All Lists]

Re: [PATCH 5/5] xfs: log timestamp updates

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/5] xfs: log timestamp updates
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 16 Feb 2012 18:30:33 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120207181155.671953164@xxxxxxxxxxxxxxxxxxxxxx>
References: <20120207181037.745771452@xxxxxxxxxxxxxxxxxxxxxx> <20120207181155.671953164@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Feb 07, 2012 at 01:10:42PM -0500, Christoph Hellwig wrote:
> Timestamps on regular files are the last metadata that XFS does not update
> transactionally.  Now that we use the delaylog mode exclusively and made
> the log scode scale extremly well there is no need to bypass that code for
> timestamp updates.  Logging all updates allows to drop a lot of code, and
> will allow for further performance improvements later on.
> 
> Note that this patch drops optimized handling of fdatasync - it will be
> added back in a separate commit.

Only thing I see here is that we'll now block reads on a frozen
filesystem. That will bring us in line with ext3/4 behaviour, but
that's probably no bad thing from a consistency/user expectation POV.

> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

The code looks ok, but could you elaborate on this:

> --- xfs.orig/fs/xfs/xfs_super.c       2012-02-07 10:01:26.804584821 -0800
> +++ xfs/fs/xfs/xfs_super.c    2012-02-07 10:01:29.214584865 -0800
> @@ -868,91 +868,58 @@ xfs_fs_inode_init_once(
>  }
>  
>  /*
> - * Dirty the XFS inode when mark_inode_dirty_sync() is called so that
> - * we catch unlogged VFS level updates to the inode.
> + * This is called by the VFS when dirtying inode metadata.  This can happen
> + * for a few reasons, but we only care about timestamp updates, given that
> + * we handled the rest ourselves.  In theory no other calls should happen,
> + * but for example generic_write_end() keeps dirtying the inode after
> + * updating i_size.  Thus we check that the flags are exactly I_DIRTY_SYNC,
> + * and skip this call otherwise.
>   *
> - * We need the barrier() to maintain correct ordering between unlogged
> - * updates and the transaction commit code that clears the i_update_core
> - * field. This requires all updates to be completed before marking the
> - * inode dirty.
> + * We'll hopefull get a different method just for updating timestamps soon,
> + * at which point this hack can go away, and maybe we'll also get real
> + * error handling here.
>   */

Are you looking at adding specific superblock operation for updating
timestamps on an inode? Or something else?

As to fdatasync optimisations, we could just add a new (flags) field
to the inode log item (i.e. separate to the format flags) that is
set here if and only if the inode has not already had it's core
logged and is in the CIL. If subsequent transactions that log the
core clear that flag, then we have a flag that would only be set if
timestamps have been logged by themselves.  That would give
xfs_file_fsync some method of determining whether fdatasync
should force the log or not....

> @@ -1471,7 +1438,6 @@ static const struct super_operations xfs
>       .alloc_inode            = xfs_fs_alloc_inode,
>       .destroy_inode          = xfs_fs_destroy_inode,
>       .dirty_inode            = xfs_fs_dirty_inode,
> -     .write_inode            = xfs_fs_write_inode,
>       .evict_inode            = xfs_fs_evict_inode,
>       .put_super              = xfs_fs_put_super,
>       .sync_fs                = xfs_fs_sync_fs,

Oh, how I like that. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>