Re: [PATCH 2/2] xfs_export_operations.commit_metadata

Alex Elder
2010-02-12T14:46:51Z

On Thu, 2010-02-11 at 16:05 -0600, Ben Myers wrote:
> This is the commit_metadata export operation for XFS, including the changes
> suggested by hch and dgc:
>
> - Takes two two inodes instead of dentries and can assume the parent is
> always set.

I alluded to this in my review of the first patch.
This could be changed considered in a more generic
sense, "sync one or two inodes' metadata" rather
than presupposing the two inodes have a parent/child
relationship.

> - Uses xfs_lock_two_inodes for the ilock.
>
> - Forces the log up to the larger lsn of parent and child.
>
> - Uses XFS_LSN_CMP for lsn comparison.
>
> - Doesn't force the log if nobody had a pincount.

So if the log doesn't get forced, what causes the
desired metadata sync expected as a result of this
call? (Maybe this is a dumb question.)

> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
> fs/xfs/linux-2.6/xfs_export.c | 43 +++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/fs/xfs/linux-2.6/xfs_export.c b/fs/xfs/linux-2.6/xfs_export.c
> index 87b8cbd..d37ced1 100644
> --- a/fs/xfs/linux-2.6/xfs_export.c
> +++ b/fs/xfs/linux-2.6/xfs_export.c
> @@ -29,6 +29,7 @@
> #include "xfs_vnodeops.h"
> #include "xfs_bmap_btree.h"
> #include "xfs_inode.h"
> +#include "xfs_inode_item.h"
>
> /*
> * Note that we only accept fileids which are long enough rather than allow
> @@ -215,9 +216,51 @@ xfs_fs_get_parent(
> return d_obtain_alias(VFS_I(cip));
> }
>
> +STATIC int
> +xfs_fs_nfs_commit_metadata(
> + struct inode *parent,
> + struct inode *child)
> +{
> + struct xfs_inode *dp = XFS_I(parent);
> + struct xfs_inode *ip = NULL;
> + struct xfs_mount *mp = dp->i_mount;
> + xfs_lsn_t force_lsn = NULLCOMMITLSN;
> + int error = 0;
> +
> + if (child) {
> + ip = XFS_I(child);
> + xfs_lock_two_inodes(dp, ip, XFS_ILOCK_SHARED);
> + } else {
> + xfs_ilock(dp, XFS_ILOCK_SHARED);
> + }
> +
> + if (xfs_ipincount(dp)) {
> + force_lsn = dp->i_itemp->ili_last_lsn;
> + if (ip && xfs_ipincount(ip) &&
> + XFS_LSN_CMP(force_lsn, ip->i_itemp->ili_last_lsn) < 0) {
> + force_lsn = ip->i_itemp->ili_last_lsn;
> + }
> + } else if (ip && xfs_ipincount(ip)) {
> + force_lsn = ip->i_itemp->ili_last_lsn;
> + }
> +
> + if (force_lsn != NULLCOMMITLSN) {
> + error = _xfs_log_force(mp, force_lsn,
> + XFS_LOG_FORCE | XFS_LOG_SYNC, NULL);

You want this here:
error = xfs_log_force_lsn(mp, force_lsn, XFS_LOG_FORCE | XFS_LOG_SYNC);

> + }
> +
> + if (child)
> + xfs_iunlock(ip, XFS_ILOCK_SHARED);
> + if (parent)
> + xfs_iunlock(dp, XFS_ILOCK_SHARED);
> +
> + return error;
> +}
> +
> const struct export_operations xfs_export_operations = {
> .encode_fh = xfs_fs_encode_fh,
> .fh_to_dentry = xfs_fs_fh_to_dentry,
> .fh_to_parent = xfs_fs_fh_to_parent,
> .get_parent = xfs_fs_get_parent,
> + .commit_metadata = xfs_fs_nfs_commit_metadata,
> };
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs