Generally this looks like a good change, but I do have
some questions embedded below.
-Alex
On Thu, 2010-02-11 at 16:05 -0600, Ben Myers wrote:
> - Add commit_metadata export_operation to allow the underlying filesystem to
> decide how to sync parent and child inodes most efficiently.
>
> - Usage of nfsd_sync_dir and write_inode_now has been replaced with the
> commit_metadata function that takes a svc_fh and optional dentry for the child.
In the cases you pass the child dentry, you will now be syncing
things to disk in a slightly different way/order from before.
I think this way is actually much better, but I thought I'd ask
anyway: are you sure that it's OK to sync them at the same time,
rather than directory first, then child (or the other way around,
depending on the case)?
> - 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 synched here.
>
> - nfsd4_sync_rec_dir now uses vfs_fsync so that commit_metadata can be static
>
> - Add a 'delay_commit' arg to nfsd_setattr so that callers of
> nfsd_create_setattr can commit parent and child together avoiding an extra
> sync.
I think this is a distinct enough change to warrant a separate patch
from the commit_metadata feature.
>
> Signed-off-by: Ben Myers <bpm@xxxxxxx>
. . .
> 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);
Why do you no longer need to acquire the mutex here?
I see it gets acquired during the ->fsync() call
inside vfs_fsync_range(), but is there a reason
that it needed to be held during the filemap_write_and_wait()
(as is done in the nfsd_sync_dir() code) also?
. . .
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index ed024d3..97474fb 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
. . .
> @@ -415,9 +449,8 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> }
> if (size_change)
> put_write_access(inode);
> - if (!err)
> - if (EX_ISSYNC(fhp->fh_export))
> - write_inode_now(inode, 1);
> + if (!err && !delay_commit)
> + err = nfserrno(commit_metadata(fhp, NULL));
Is this sufficient even if the size has changed?
> out:
> return err;
>
. . .
> @@ -1766,10 +1763,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, NULL);
> if (!host_err)
> - host_err = nfsd_sync_dir(fdentry);
> + host_err = commit_metadata(ffhp, NULL);
It violates the spirit of the "parent" and "child"
nature of its arguments, but it might be nice to
commit both directories' metadata with the same
call here.
. . .
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
