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

[Christoph Hellwig <hch@infradead.org>]

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.

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