xfs
[Top] [All Lists]

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

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 1/2] commit_metadata export operation replacing nfsd_sync_dir
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 16 Feb 2010 17:06:45 -0500
Cc: bfields@xxxxxxxxxxxx, linux-nfs@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx
In-reply-to: <20100216210413.5694.88826.stgit@case>
References: <20100216210026.5694.14423.stgit@case> <20100216210413.5694.88826.stgit@case>
User-agent: Mutt/1.5.19 (2009-01-05)
This looks very good to me.  A couple of tiny nitpicks below:


> +/*
> + * Commit metadata changes to stable storage.  You pay pass NULL for dchild.
> + */

The dchild argument is gone in this version.

> +     struct writeback_control wbc = {
> +             .sync_mode = WB_SYNC_ALL,
> +             .nr_to_write = 0, /* metadata only */
> +     };
> +     int error = 0;
> +
> +     if (!EX_ISSYNC(fhp->fh_export))
> +             return 0;
> +
> +     if (export_ops->commit_metadata) {
> +             error = export_ops->commit_metadata(inode);
> +     } else {
> +             error = sync_inode(inode, &wbc);
> +     }

Maybe move the wbc declaration into the else branch here to keep
variables in the smallest possible scope.

> @@ -1199,7 +1204,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct 
> svc_fh *resfhp,
>       if (current_fsuid() != 0)
>               iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
>       if (iap->ia_valid)
> -             return nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0);
> +             return nfsd_setattr(rqstp, resfhp, iap, 0, 0);

While this is a worthwhile cleanup I'd not put it into a patch that is
now entirely unrelated.

> +     err = 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;

The if statement above seems rather minindented, possibly due to the
partial use of spaces instead of tabs.

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