Re: [RFC PATCH 1/2] commit_metadata export operation and nfsd_sync2

bpm
2010-02-10T19:53:05Z

Hey Christoph,

Thanks for the suggestions.

On Wed, Feb 10, 2010 at 03:56:14AM -0500, Christoph Hellwig wrote:
> A better calling convention would be for the first paramter to
> always be non-zero (and we could take the file handle for that one),

Yeah that works out much nicer.

> Currently the only caller passing a NULL first argument is
> nfsd_setattr. If we use ->write_inode as fallback we could just
> pass it as first, if using ->fsync we'd need to take i_mutex before,
> but we might just stick to using ->write_inode to keep things
> simple (and get rid of the NULL file pointer in ->fsync).
>
> > + if (export_ops->commit_metadata) {
> > + if (parent)
> > + error = filemap_write_and_wait(p_inode->i_mapping);
> > + if (child)
> > + error2 = filemap_write_and_wait(c_inode->i_mapping);
>
> I think Trond explained that we do not want force data to disk here.

Thought so. I was making an effort to punch all the same buttons as
before so that behavior wouldn't change for filesystems other than xfs
until they're ready.

> > + if (parent) {
> > + error = filemap_write_and_wait(p_inode->i_mapping);
> > + if (!error && p_inode->i_fop->fsync)
> > + error = p_inode->i_fop->fsync(NULL, parent, 0);
> > + }
> > + if (child)
> > + write_inode_now(c_inode, 1);
> > + }
>
> Btw, as we don't need to write data to disk this should be a
> sync_inode calls with the following second argument:
>
> struct writeback_control wbc = {
> .sync_mode = WB_SYNC_ALL,
> .nr_to_write = 0, /* metadata-only */
> };

So, uh, are you suggesting that I use file_operations.fsync,
write_inode_now, vfs_fsync, write_inode, sync_inode, or
super_operations.write_inode? Would it be good to stay away from the
inode_lock?

-Ben