On Fri, Mar 17, 2017 at 09:23:00PM +0000, Rick Macklem wrote: > Dimitry Andric wrote: > >On 17 Mar 2017, at 15:19, Konstantin Belousov <kostik...@gmail.com> wrote: > >> > >> On Fri, Mar 17, 2017 at 01:53:46PM +0000, Rick Macklem wrote: > >>> Well, I don't mind adding ncl_flush(), but it shouldn't be > >>> necessary. I actually had it in the first > >>> rendition of the patch, but took it out because it should happen > >>> on the VOP_CLOSE() if any writing to the buffer cache happened > >>> and that code hasn't changed in many years. > >> Dirty pages are flushed by writes, so if we have a set of dirty pages and > >> async vm_object_page_clean() is called on the vnode' vm_object, we get > >> a bunch of delayed-write AKA dirty buffers. This is possible even after > >> VOP_CLOSE() was done, e.g. by syncer performing regular run involving > >> vfs_msync(). > When I was talking about ncl_flush() above, I was referring to buffer cache > buffers written by a write(2) syscall, not the case of mmap'd pages. But dirty buffers can appear on the vnode queue due to dirty pages msyncing by syncer, for instance.
> > >> > >> I agree that the patch would not create new dirty buffers, but it is > >> possible > >> to get them by other means. > To write to a buffer cache block, the file would be opened by another > thread and that is what this sanity check was meant to catch. As > for dirtying pages that are mmap'd, as far as I understand it, the > NFS client has no way of knowing if this will happen more until > VOP_INACTIVE() is called on the vnode. > Syncer does not open the vnode inside the vfs_msync() operations. > To be honest, this check could easily be deleted. After all, NFS could care > less if a file > is being executed (all it sees are reads and writes). Without the check, the > executable > might do "interesting" things;-) > [stuff snipped] > > FWIW, Rick's patch seems to do the trick, both for my test case and lld > > itself. And even with vfs.timestamp_precision=3 on both server and > > client. > Hopefully the original reporter of the problem (Gergely ??) can test the > patch as well. > I think the patch is pretty harmless, although it could be argued that setting > np->m_mtime = np->n_nattr.na_mtime (or close to that) > shouldn't happen for the case where there isn't any dirty pages found to > flush. > However, once a file mmap'd we don't know when it does get modified anyhow > (as discussed above), so setting it here doesn't seem harmful to me. We do track writeability to the file, and do not allow execution if there is an active writer, be it a file descriptor opened for write, or a writeable mapping. And in reverse, if the file is executed (VV_TEXT is set), then we disallow opening the file for write. Of course, this cannot work when we execute file on one NFS client, and another client modifies the file, but then exactly this check and kill should provide some sanity. > > Thanks for testing the patch. Now, if others can test it...rick > _______________________________________________ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
