Re: process killed: text file modification

On Thu, Mar 23, 2017 at 12:55:09AM +0000, Rick Macklem wrote:
> Wow, this is looking good to me. I had thought that the simple way to make
> ncl_putpages() go through the buffer cache was to replace ncl_writerpc() with
> VOP_WRITE(). My concern was all the memory<->memory copying that would
> go on between the pages being written and the buffers allocated by VOP_WRITE().
> If there is a way to avoid some (if not all) of this memory<->memory copying, then
> I think it would be a big improvement..
UIO_NOCOPY means that uio is only updated to indicate the operation
as performed, but no real copying occurs. This is exactly what the
_putpages() case needs, since the data is already in the pages. When
buffers are created for the corresponding file offsets, appropriate
pages are put into the buffer's page array and data appears in the
buffer with zero copying.

This is how generic putpages code works for local filesystems, e.g. UFS.

> 
> As far as the commit goes, you don't need to do anything if you are calling VOP_WRITE().
> (The code below VOP_WRITE() takes care of all of that.)
> --> You might want to implement a function like nfs_write(), but with extra arguments.
>       If you did that, you could indicate when you want the writes to happen synchronously
>       vs. async/delayed and that would decide when FILESYNC would be specified.
> 
Yes, this is what I want to improve in the patch.  As I noted, I added
translation of the VM_PAGER_PUT_* flags into IO_* flags, but IO_* flags
needs more code.  Most important is IO_ASYNC which probably should become
similar to the current !IO_SYNC ncl_write(), but without clustering.

You mentioned that NFSWRITE_FILESYNC/NFSWRITE_UNSTABLE should be specified,
and it seems that this is managed by B_NEEDCOMMIT buffer flag.  I see
that B_NEEEDCOMMIT is cleared in ncl_write().

> As far as I know, the unpatched nc_putpages() is badly broken for the
> UNSTABLE/commit case. For UNSTABLE writes, the client is supposed to
> know how to write the data again if the server crashes/reboots before
> a Commit RPC is successfully done for the data. (The ncl_clearcommit()
> function is the one called when the server indicates it has rebooted
> and needs this. It makes no sense whatsoever and breaks the client
> to call it in ncl_putpages() when mustcommit is set. All mustcommit
> being set indicates is that the write RPC was done UNSTABLE and the
> above applies to it. Some servers always do FILESYNC, so it isn't ever
> necessary to do a Commit PRC or redo the write RPCs.)
>
> Summary. If you are calling VOP_WRITE() or a similar call above the
> buffer cache, then you don't have to worry about any of this.
Ok, thanks.

> 
> > Things that needs to be done is to add missed handling of the IO flags to
> > ncl_write().
> 
> > +       if (error == 0 || !nfs_keep_dirty_on_error)
> >                 vnode_pager_undirty_pages(pages, rtvals, count - uio.uio_resid);
> If the data isn't copied, will this data still be available to the NFS buffer cache code,
> so that it can redo the writes for the UNSTABLE case, if the server reboots before a
> Commit RPC has succeeded?
As far as buffers are there (e.g. not marked clean), the data is there.
Of course, userspace can modify the data in pages if writeable mapping
exists, but it is expected.

Oh, I remembered one more question I wanted to ask in the previous mail.
With the patch, ncl_write() can be called from the delayed contexts
like pagedaemon, or after all writeable file descriptors referencing
the file are closed.  Wouldn't some calls to VOP_OPEN()/VOP_CLOSE() around
the VOP_WRITE() needed there ?

> 
> > -               if (must_commit)
> > -                       ncl_clearcommit(vp->v_mount);
> No matter what else we do, this should go away. As above, it breaks the NFS client
> and basically forces all dirty buffer cache blocks to be rewritten when it shouldn't
> be necessary.