[PATCH 0/2] commit_metadata export operation v6

[PATCH 0/2] commit_metadata export operation v6

[Ben Myers]

Hey Bruce,

Here is the latest version of the knfsd sync changes.  I have addressed the
suggestions provided by Christoph, Trond, Dave, and Alex.

In this latest version I have fixed up the few nits in the nfsd patch that
Christoph pointed out yesterday.  I also have simplified the xfs patch as
suggested by Dave.

Thanks,
	Ben
---

Ben Myers (2):
      commit_metadata export operation replacing nfsd_sync_dir
      xfs_export_operations.commit_metadata


 fs/nfsd/nfs4recover.c         |    4 --
 fs/nfsd/vfs.c                 |  106 ++++++++++++++++++++---------------------
 fs/xfs/linux-2.6/xfs_export.c |   20 ++++++++
 include/linux/exportfs.h      |    5 ++
 4 files changed, 78 insertions(+), 57 deletions(-)

-- 
Signature

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

[PATCH 1/2] commit_metadata export operation replacing nfsd_sync_dir

[Ben Myers]

- Add commit_metadata export_operation to allow the underlying filesystem to
decide how to commit an inode most efficiently.

- Usage of nfsd_sync_dir and write_inode_now has been replaced with the
commit_metadata function that takes a svc_fh.

- The commit_metadata function calls the commit_metadata export_op if it's
there, or else falls back to sync_inode instead of fsync and write_inode_now
because only metadata need be synced here.

- nfsd4_sync_rec_dir now uses vfs_fsync so that commit_metadata can be static

Signed-off-by: Ben Myers <bpm@sgi.com>
---
 fs/nfsd/nfs4recover.c    |    4 --
 fs/nfsd/vfs.c            |  106 +++++++++++++++++++++++-----------------------
 include/linux/exportfs.h |    5 ++
 3 files changed, 58 insertions(+), 57 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 5a754f7..98fb98e 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -119,9 +119,7 @@ out_no_tfm:
 static void
 nfsd4_sync_rec_dir(void)
 {
-	mutex_lock(&rec_dir.dentry->d_inode->i_mutex);
-	nfsd_sync_dir(rec_dir.dentry);
-	mutex_unlock(&rec_dir.dentry->d_inode->i_mutex);
+	vfs_fsync(NULL, rec_dir.dentry, 0);
 }
 
 int
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index ed024d3..8afdba5 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -27,6 +27,8 @@
 #include <linux/jhash.h>
 #include <linux/ima.h>
 #include <asm/uaccess.h>
+#include <linux/exportfs.h>
+#include <linux/writeback.h>
 
 #ifdef CONFIG_NFSD_V3
 #include "xdr3.h"
@@ -271,6 +273,32 @@ out:
 	return err;
 }
 
+/*
+ * Commit metadata changes to stable storage.
+ */
+static int
+commit_metadata(struct svc_fh *fhp)
+{
+	struct inode *inode = fhp->fh_dentry->d_inode;
+	const struct export_operations *export_ops = inode->i_sb->s_export_op;
+	int error = 0;
+
+	if (!EX_ISSYNC(fhp->fh_export))
+		return 0;
+
+	if (export_ops->commit_metadata) {
+		error = export_ops->commit_metadata(inode);
+	} else {
+		struct writeback_control wbc = {
+			.sync_mode = WB_SYNC_ALL,
+			.nr_to_write = 0, /* metadata only */
+		};
+
+		error = sync_inode(inode, &wbc);
+	}
+
+	return error;
+}
 
 /*
  * Set various file attributes.
@@ -769,28 +797,6 @@ nfsd_close(struct file *filp)
 }
 
 /*
- * Sync a directory to disk.
- *
- * We can't just call vfs_fsync because our requirements are slightly odd:
- *
- *  a) we do not have a file struct available
- *  b) we expect to have i_mutex already held by the caller
- */
-int
-nfsd_sync_dir(struct dentry *dentry)
-{
-	struct inode *inode = dentry->d_inode;
-	int error;
-
-	WARN_ON(!mutex_is_locked(&inode->i_mutex));
-
-	error = filemap_write_and_wait(inode->i_mapping);
-	if (!error && inode->i_fop->fsync)
-		error = inode->i_fop->fsync(NULL, dentry, 0);
-	return error;
-}
-
-/*
  * Obtain the readahead parameters for the file
  * specified by (dev, ino).
  */
@@ -1331,12 +1337,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out_nfserr;
 	}
 
-	if (EX_ISSYNC(fhp->fh_export)) {
-		err = nfserrno(nfsd_sync_dir(dentry));
-		write_inode_now(dchild->d_inode, 1);
-	}
+	err = nfsd_create_setattr(rqstp, resfhp, iap);
 
-	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
+	/*
+	 * nfsd_setattr already committed the child.  Transactional filesystems
+	 * had a chance to commit changes for both parent and child
+	 * simultaneously making the following commit_metadata a noop.
+	 */
+	err2 = nfserrno(commit_metadata(fhp));
 	if (err2)
 		err = err2;
 	mnt_drop_write(fhp->fh_export->ex_path.mnt);
@@ -1368,7 +1376,6 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	struct dentry	*dentry, *dchild = NULL;
 	struct inode	*dirp;
 	__be32		err;
-	__be32		err2;
 	int		host_err;
 	__u32		v_mtime=0, v_atime=0;
 
@@ -1463,11 +1470,6 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (created)
 		*created = 1;
 
-	if (EX_ISSYNC(fhp->fh_export)) {
-		err = nfserrno(nfsd_sync_dir(dentry));
-		/* setattr will sync the child (or not) */
-	}
-
 	nfsd_check_ignore_resizing(iap);
 
 	if (createmode == NFS3_CREATE_EXCLUSIVE) {
@@ -1482,9 +1484,13 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	}
 
  set_attr:
-	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
-	if (err2)
-		err = err2;
+	err = nfsd_create_setattr(rqstp, resfhp, iap);
+
+	/*
+	 * nfsd_setattr already committed the child (and possibly also the parent).
+	 */
+	if (!err)
+		err = nfserrno(commit_metadata(fhp));
 
 	mnt_drop_write(fhp->fh_export->ex_path.mnt);
 	/*
@@ -1599,12 +1605,9 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		}
 	} else
 		host_err = vfs_symlink(dentry->d_inode, dnew, path);
-
-	if (!host_err) {
-		if (EX_ISSYNC(fhp->fh_export))
-			host_err = nfsd_sync_dir(dentry);
-	}
 	err = nfserrno(host_err);
+	if (!err)
+		err = nfserrno(commit_metadata(fhp));
 	fh_unlock(fhp);
 
 	mnt_drop_write(fhp->fh_export->ex_path.mnt);
@@ -1666,11 +1669,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 	}
 	host_err = vfs_link(dold, dirp, dnew);
 	if (!host_err) {
-		if (EX_ISSYNC(ffhp->fh_export)) {
-			err = nfserrno(nfsd_sync_dir(ddir));
-			write_inode_now(dest, 1);
-		}
-		err = 0;
+		err = nfserrno(commit_metadata(ffhp));
+		if (!err)
+			err = nfserrno(commit_metadata(tfhp));
 	} else {
 		if (host_err == -EXDEV && rqstp->rq_vers == 2)
 			err = nfserr_acces;
@@ -1766,10 +1767,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 		goto out_dput_new;
 
 	host_err = vfs_rename(fdir, odentry, tdir, ndentry);
-	if (!host_err && EX_ISSYNC(tfhp->fh_export)) {
-		host_err = nfsd_sync_dir(tdentry);
+	if (!host_err) {
+		host_err = commit_metadata(tfhp);
 		if (!host_err)
-			host_err = nfsd_sync_dir(fdentry);
+			host_err = commit_metadata(ffhp);
 	}
 
 	mnt_drop_write(ffhp->fh_export->ex_path.mnt);
@@ -1850,12 +1851,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 
 	dput(rdentry);
 
-	if (host_err)
-		goto out_drop;
-	if (EX_ISSYNC(fhp->fh_export))
-		host_err = nfsd_sync_dir(dentry);
+	if (!host_err)
+		host_err = commit_metadata(fhp);
 
-out_drop:
 	mnt_drop_write(fhp->fh_export->ex_path.mnt);
 out_nfserr:
 	err = nfserrno(host_err);
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index dc12f41..a9cd507 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -96,6 +96,7 @@ struct fid {
  * @fh_to_parent:   find the implied object's parent and get a dentry for it
  * @get_name:       find the name for a given inode in a given directory
  * @get_parent:     find the parent of a given directory
+ * @commit_metadata: commit metadata changes to stable storage
  *
  * See Documentation/filesystems/nfs/Exporting for details on how to use
  * this interface correctly.
@@ -137,6 +138,9 @@ struct fid {
  *    is also a directory.  In the event that it cannot be found, or storage
  *    space cannot be allocated, a %ERR_PTR should be returned.
  *
+ * commit_metadata:
+ *    @commit_metadata should commit metadata changes to stable storage.
+ *
  * Locking rules:
  *    get_parent is called with child->d_inode->i_mutex down
  *    get_name is not (which is possibly inconsistent)
@@ -152,6 +156,7 @@ struct export_operations {
 	int (*get_name)(struct dentry *parent, char *name,
 			struct dentry *child);
 	struct dentry * (*get_parent)(struct dentry *child);
+	int (*commit_metadata)(struct inode *inode);
 };
 
 extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,

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

[PATCH 2/2] xfs_export_operations.commit_metadata

[Ben Myers]

This is the commit_metadata export operation for XFS.

- Takes one inode to be committed.

- Forces the log up to the lsn of the inode.

- Doesn't force the log if the inode doesn't have a pincount.

Signed-off-by: Ben Myers <bpm@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/linux-2.6/xfs_export.c |   20 ++++++++++++++++++++
 1 files changed, 20 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..8615d99 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,28 @@ xfs_fs_get_parent(
 	return d_obtain_alias(VFS_I(cip));
 }
 
+STATIC int
+xfs_fs_nfs_commit_metadata(
+	struct inode		*inode)
+{
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	int			error = 0;
+
+	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	if (xfs_ipincount(ip)) {
+		error = _xfs_log_force(mp, ip->i_itemp->ili_last_lsn,
+				XFS_LOG_FORCE | XFS_LOG_SYNC, NULL);
+	}
+	xfs_iunlock(ip, 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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[Dave Chinner]

On Wed, Feb 17, 2010 at 02:05:16PM -0600, Ben Myers wrote:
> This is the commit_metadata export operation for XFS.
> 
> - Takes one inode to be committed.
> 
> - Forces the log up to the lsn of the inode.
> 
> - Doesn't force the log if the inode doesn't have a pincount.
> 
> Signed-off-by: Ben Myers <bpm@sgi.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks good, Ben.

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 0/2] commit_metadata export operation v6

[Christoph Hellwig]

On Wed, Feb 17, 2010 at 02:05:05PM -0600, Ben Myers wrote:
> Hey Bruce,
> 
> Here is the latest version of the knfsd sync changes.  I have addressed the
> suggestions provided by Christoph, Trond, Dave, and Alex.
> 
> In this latest version I have fixed up the few nits in the nfsd patch that
> Christoph pointed out yesterday.  I also have simplified the xfs patch as
> suggested by Dave.

Both patches looks good to me,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH 0/2] commit_metadata export operation v6

[J. Bruce Fields]

On Wed, Feb 17, 2010 at 02:05:05PM -0600, Ben Myers wrote:
> Hey Bruce,
> 
> Here is the latest version of the knfsd sync changes.  I have addressed the
> suggestions provided by Christoph, Trond, Dave, and Alex.
> 
> In this latest version I have fixed up the few nits in the nfsd patch that
> Christoph pointed out yesterday.  I also have simplified the xfs patch as
> suggested by Dave.

Thanks, applying.

(Do you the second (xfs) patch to go in through the nfsd tree as well,
or should that go in through xfs maintainers after the nfsd merge?)

--b.

> 
> Thanks,
> 	Ben
> ---
> 
> Ben Myers (2):
>       commit_metadata export operation replacing nfsd_sync_dir
>       xfs_export_operations.commit_metadata
> 
> 
>  fs/nfsd/nfs4recover.c         |    4 --
>  fs/nfsd/vfs.c                 |  106 ++++++++++++++++++++---------------------
>  fs/xfs/linux-2.6/xfs_export.c |   20 ++++++++
>  include/linux/exportfs.h      |    5 ++
>  4 files changed, 78 insertions(+), 57 deletions(-)
> 
> -- 
> Signature

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

Re: [PATCH 0/2] commit_metadata export operation v6

[bpm]

Hey Bruce,

On Sat, Feb 20, 2010 at 06:38:04PM -0500, J. Bruce Fields wrote:
> On Wed, Feb 17, 2010 at 02:05:05PM -0600, Ben Myers wrote:
> > Hey Bruce,
> > 
> > Here is the latest version of the knfsd sync changes.  I have addressed the
> > suggestions provided by Christoph, Trond, Dave, and Alex.
> > 
> > In this latest version I have fixed up the few nits in the nfsd patch that
> > Christoph pointed out yesterday.  I also have simplified the xfs patch as
> > suggested by Dave.
> 
> Thanks, applying.
> 
> (Do you the second (xfs) patch to go in through the nfsd tree as well,
> or should that go in through xfs maintainers after the nfsd merge?)

I chatted with Alex.  It sounds like the best thing is for both patches
to go in through the nfsd tree since they need to go in order.

Thanks,
	Ben

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

Re: [PATCH 0/2] commit_metadata export operation v6

[J. Bruce Fields]

On Mon, Feb 22, 2010 at 01:53:53PM -0600, bpm@sgi.com wrote:
> Hey Bruce,
> 
> On Sat, Feb 20, 2010 at 06:38:04PM -0500, J. Bruce Fields wrote:
> > On Wed, Feb 17, 2010 at 02:05:05PM -0600, Ben Myers wrote:
> > > Hey Bruce,
> > > 
> > > Here is the latest version of the knfsd sync changes.  I have addressed the
> > > suggestions provided by Christoph, Trond, Dave, and Alex.
> > > 
> > > In this latest version I have fixed up the few nits in the nfsd patch that
> > > Christoph pointed out yesterday.  I also have simplified the xfs patch as
> > > suggested by Dave.
> > 
> > Thanks, applying.
> > 
> > (Do you the second (xfs) patch to go in through the nfsd tree as well,
> > or should that go in through xfs maintainers after the nfsd merge?)
> 
> I chatted with Alex.  It sounds like the best thing is for both patches
> to go in through the nfsd tree since they need to go in order.

OK, thanks, committed.--b.

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

[PATCH 0/2] commit_metadata export operation v5

[Ben Myers]

Hey Bruce,

Here is the latest version of the knfsd sync changes that I've been spamming
you with.  I have addressed the latest suggestions provided by Christoph,
Trond, Dave, and Alex.  This version of the commit_metadata export operation
takes only one inode as suggested by Christoph and it turns out to be much
cleaner this way.  Now we've gone back to committing all of the time in
nfsd_create_setattr and don't bother with the added argument.  A couple extra
comments to explain the commit ordering.

Thanks,
Ben

---

Ben Myers (2):
      commit_metadata export operation replacing nfsd_sync_dir
      xfs_export_operations.commit_metadata


 fs/nfsd/nfs4recover.c         |    4 --
 fs/nfsd/vfs.c                 |  109 ++++++++++++++++++++---------------------
 fs/xfs/linux-2.6/xfs_export.c |   24 +++++++++
 include/linux/exportfs.h      |    5 ++
 4 files changed, 83 insertions(+), 59 deletions(-)

-- 

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

[PATCH 2/2] xfs_export_operations.commit_metadata

[Ben Myers]

This is the commit_metadata export operation for XFS.

- Takes one inode to be committed.

- Forces the log up to the lsn of the inode.

- Doesn't force the log if the inode doesn't have a pincount.

Signed-off-by: Ben Myers <bpm@sgi.com>
---
 fs/xfs/linux-2.6/xfs_export.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 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..47a8d1f 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,32 @@ xfs_fs_get_parent(
 	return d_obtain_alias(VFS_I(cip));
 }
 
+STATIC int
+xfs_fs_nfs_commit_metadata(
+	struct inode		*inode)
+{
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	xfs_lsn_t		force_lsn = NULLCOMMITLSN;
+	int			error = 0;
+
+	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	if (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);
+	}
+	xfs_iunlock(ip, 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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[Dave Chinner]

On Tue, Feb 16, 2010 at 03:04:18PM -0600, Ben Myers wrote:
> This is the commit_metadata export operation for XFS.
> 
> - Takes one inode to be committed.
> 
> - Forces the log up to the lsn of the inode.
> 
> - Doesn't force the log if the inode doesn't have a pincount.
> 
> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
>  fs/xfs/linux-2.6/xfs_export.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 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..47a8d1f 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,32 @@ xfs_fs_get_parent(
>  	return d_obtain_alias(VFS_I(cip));
>  }
>  
> +STATIC int
> +xfs_fs_nfs_commit_metadata(
> +	struct inode		*inode)
> +{
> +	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_lsn_t		force_lsn = NULLCOMMITLSN;
> +	int			error = 0;
> +
> +	xfs_ilock(ip, XFS_ILOCK_SHARED);
> +	if (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);
> +	}

That could be simplified to:

	if (xfs_ipincount(ip))
		_xfs_log_force(mp, ip->i_itemp->ili_last_lsn
				XFS_LOG_FORCE | XFS_LOG_SYNC, NULL);

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 0/2] commit_metadata export operation v4

[Ben Myers]

Hey Bruce,

Here is the latest version of the knfsd sync changes.  I have addressed the
suggestions provided by Christoph, Trond, and Dave.  This time rebased atop
for-2.6.34 (I hope).  Sorry for the extra noise.

Thanks,
	Ben

---

Ben Myers (2):
      commit_metadata export operation replacing nfsd_sync_dir
      xfs_export_operations.commit_metadata


 fs/nfsd/nfs3proc.c            |    2 -
 fs/nfsd/nfs4proc.c            |    2 -
 fs/nfsd/nfs4recover.c         |    4 -
 fs/nfsd/nfs4state.c           |    2 -
 fs/nfsd/nfsproc.c             |    4 +
 fs/nfsd/vfs.c                 |  116 +++++++++++++++++++----------------------
 fs/nfsd/vfs.h                 |    3 -
 fs/xfs/linux-2.6/xfs_export.c |   43 +++++++++++++++
 include/linux/exportfs.h      |    6 ++
 9 files changed, 111 insertions(+), 71 deletions(-)

-- 

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

[PATCH 2/2] xfs_export_operations.commit_metadata

[Ben Myers]

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.

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

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);
+	}
+
+	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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[Alex Elder]

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



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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[Christoph Hellwig]

On Fri, Feb 12, 2010 at 08:46:46AM -0600, Alex Elder wrote:
> 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.

Or just implement my later suggestion to only pass one inode to the
method and cal in on both inodes.   For the non-create case where
we have only one transaction to deal with fhr first call will take
care of it and unpin the second inode by forcing the log buffer out.
For the create case we need to make sure to call it on the child first
so that we force out the setattr transaction which also forced out the
earlier one.   This keeps the calling convention quite a bit simpler,
and also means we don't have to bother with locking two inodes or lsn
comparisms.

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

Inodes get pinned on transaction commit, and unpinned when the log I/O
for that transaction completes.  If the inode is not pinned this implies
it has already been written to disk, e.g. because we're filling the log
so fast that we need to write out more log buffers in that tiny window
between the metada operation and the commit_metadata call.

> > +		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);

In the XFS tree we do want either

	xfs_log_force_lsn(mp, force_lsn, XFS_LOG_SYNC);

or
	error = _xfs_log_force_lsn(mp, force_lsn, XFS_LOG_SYNC, NULL);

if we care enough about the returned error.  But Ben is working against
the NFS tree which doesn't have that change yet.

We can deal with that by either commiting the old variant to the nfs
tree and then leaving sending Stephen a patch to fix it up in -next,
or just not apply the xfs commit_metadata implementation yet, and wait
for it until both the xfs and nfs trees have hit mainline.

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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[bpm]

On Fri, Feb 12, 2010 at 12:47:07PM -0500, Christoph Hellwig wrote:
> On Fri, Feb 12, 2010 at 08:46:46AM -0600, Alex Elder wrote:
> > 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.
> 
> Or just implement my later suggestion to only pass one inode to the
> method and cal in on both inodes.   For the non-create case where
> we have only one transaction to deal with fhr first call will take
> care of it and unpin the second inode by forcing the log buffer out.
> For the create case we need to make sure to call it on the child first
> so that we force out the setattr transaction which also forced out the
> earlier one.

I chose not implement that suggestion because I prefer not to rely upon
the coincidence that the child be modified last and synced first in
knfsd.  It is better that the intent of the patch be clear, and that
filesystems other than xfs also have enough information to sort out how
to sync more efficiently.  knfsd shouldn't be forced to sync in a
specific order just because that's what works best for xfs.  Or, mebbe
it should and I'm being thick.  ;)

Alex's suggestion that ->commit_metadata be more generic about the
relationship of the two inodes seems reasonable, not sure what that
means for vfs.c:commit_metadata.  This will help with the rename case.

> This keeps the calling convention quite a bit simpler,
> and also means we don't have to bother with locking two inodes or lsn
> comparisms.

Don't need the ilock to check pincount?

> > > +		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);
> 
> In the XFS tree we do want either
> 
> 	xfs_log_force_lsn(mp, force_lsn, XFS_LOG_SYNC);
> 
> or
> 	error = _xfs_log_force_lsn(mp, force_lsn, XFS_LOG_SYNC, NULL);
> 
> if we care enough about the returned error.  But Ben is working against
> the NFS tree which doesn't have that change yet.
> 
> We can deal with that by either commiting the old variant to the nfs
> tree and then leaving sending Stephen a patch to fix it up in -next,
> or just not apply the xfs commit_metadata implementation yet, and wait
> for it until both the xfs and nfs trees have hit mainline.

Yeah.  I don't know who Stephen is.

Thanks,
	Ben

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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[J. Bruce Fields]

On Fri, Feb 12, 2010 at 01:56:47PM -0600, bpm@sgi.com wrote:
> On Fri, Feb 12, 2010 at 12:47:07PM -0500, Christoph Hellwig wrote:
> > if we care enough about the returned error.  But Ben is working against
> > the NFS tree which doesn't have that change yet.
> > 
> > We can deal with that by either commiting the old variant to the nfs
> > tree and then leaving sending Stephen a patch to fix it up in -next,
> > or just not apply the xfs commit_metadata implementation yet, and wait
> > for it until both the xfs and nfs trees have hit mainline.

Assuming the xfs tree's something stable, I could also just go ahead and
pull it into the nfsd tree.

(Or we could do it the other way around--as long as when the merge
window opens we try to send the pull requests in the same order.)

--b.

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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[Christoph Hellwig]

On Fri, Feb 12, 2010 at 03:03:59PM -0500, J. Bruce Fields wrote:
> Assuming the xfs tree's something stable, I could also just go ahead and
> pull it into the nfsd tree.
> 
> (Or we could do it the other way around--as long as when the merge
> window opens we try to send the pull requests in the same order.)

The xfs tree is supposed to be stable and we only had very few incidents
where a patch had to be pulled again.  But it's also a quite large set
of changes. 

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

Re: [PATCH 2/2] xfs_export_operations.commit_metadata

[Christoph Hellwig]

On Fri, Feb 12, 2010 at 01:56:47PM -0600, bpm@sgi.com wrote:
> I chose not implement that suggestion because I prefer not to rely upon
> the coincidence that the child be modified last and synced first in
> knfsd.

It does ot rely on that coincidence for correctness, just for a small
performance optimization.

> It is better that the intent of the patch be clear, and that
> filesystems other than xfs also have enough information to sort out how
> to sync more efficiently.  knfsd shouldn't be forced to sync in a
> specific order just because that's what works best for xfs.  Or, mebbe
> it should and I'm being thick.  ;)

The order of parent and child only happens in the create case where
NFS does a separate ->setattr call.  For all the operations handled
by the filesystem parent and child are in the same transaction for
every transactional filesystem.

> > This keeps the calling convention quite a bit simpler,
> > and also means we don't have to bother with locking two inodes or lsn
> > comparisms.
> 
> Don't need the ilock to check pincount?

Ah sorry, that should have read not bothering with locking two inodes
at the same time, which always is a bit troublesome.  We do need to lock
the inode for looking at the pincount.

> > We can deal with that by either commiting the old variant to the nfs
> > tree and then leaving sending Stephen a patch to fix it up in -next,
> > or just not apply the xfs commit_metadata implementation yet, and wait
> > for it until both the xfs and nfs trees have hit mainline.
> 
> Yeah.  I don't know who Stephen is.

Stephen Rothwell is the maintainer of the linux-next tree.

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

[PATCH 0/2] commit_metadata export operation v3

[Ben Myers]

Here is the latest version of the knfsd sync changes.  I have addressed the
suggestions provided by Christoph, Trond, and Dave.

Thanks,
	Ben

---

Ben Myers (2):
      commit_metadata export operation replacing nfsd_sync_dir
      xfs_export_operations.commit_metadata


 fs/nfsd/nfs3proc.c            |    2 -
 fs/nfsd/nfs4proc.c            |    2 -
 fs/nfsd/nfs4recover.c         |    4 -
 fs/nfsd/nfs4state.c           |    2 -
 fs/nfsd/nfsproc.c             |    4 +
 fs/nfsd/vfs.c                 |  116 +++++++++++++++++++----------------------
 fs/nfsd/vfs.h                 |    3 -
 fs/xfs/linux-2.6/xfs_export.c |   43 +++++++++++++++
 include/linux/exportfs.h      |    6 ++
 9 files changed, 111 insertions(+), 71 deletions(-)

-- 

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

[PATCH 2/2] xfs_export_operations.commit_metadata

[Ben Myers]

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.

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

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);
+	}
+
+	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