Re: [PATCH 1/2] commit_metadata export operation replacing nfsd_sync_dir

[Alex Elder <aelder@xxxxxxx>]

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.

. . .