932398 - Add portable version of msync/FlushViewOfFile

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Luke Wagner [:luke]]

Attachment: add-pr-flush-mem-to-file

NSPR currently has the ability to portably mmap/munmap.  Another bit of related, but missing functionality is msync.  I'd like to use this in bug 929236 because it's simpler and faster than the alternatives.

Note: on Windows, the patch uses FlushViewOfFile followed by FlushFileBuffers because the MSDN page for FlushViewOfFile says that is what you need to do for a synchronous write to disk.

Comment 1

[Luke Wagner [:luke]]

Attachment: add-pr-flush-mem-to-file

Oops, missed the decl in _win95.h which is, apparently, quite necessary.

Comment 2

[Luke Wagner [:luke]]

r? ping.  Getting close to needing this.

Comment 3

[Luke Wagner [:luke]]

r? ping.  This will soon be the only thing blocking bug 929236 which is a major priority for the games project.

Comment 4

[(not currently active) Ted Mielczarek]

Sorry, been very backed up on reviews lately. Will review today.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 824517 [details] [diff] [review]
add-pr-flush-mem-to-file

Review of attachment 824517 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with one change.

::: nsprpub/pr/src/md/windows/ntmisc.c
@@ +1003,5 @@
> +    PROsfd osfd;
> +
> +    osfd = ( fd == (PRFileDesc*)-1 )?  -1 : fd->secret->md.osfd;
> +
> +    if (FlushViewOfFile(addr, len) && FlushFileBuffers((HANDLE) osfd)) {

These APIs only exist as of Windows XP. I don't know if that's an issue for NSPR (I seriously hope not.)

::: nsprpub/pr/src/nspr.def
@@ +229,4 @@
>  		PR_Malloc;
>  		PR_MemMap;
>  		PR_MemUnmap;
> +		PR_FlushMemToFile;

I assume you changed the name of the function at some point and forgot to update this?

Comment 6

[Luke Wagner [:luke]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
Thanks Ted!  Where should I land this patch?

> I assume you changed the name of the function at some point and forgot to
> update this?

You're right.  (I guess this .def file isn't used in any try configuration.)

Comment 7

[Luke Wagner [:luke]]

(My main question is whether I can just land my patch in the tree.)

Comment 8

[(not currently active) Ted Mielczarek]

If you can put up a finalized patch I'll land it for you in the NSPR repo, and then we can cut a beta release and sync it to m-c.

Comment 9

[Luke Wagner [:luke]]

Attachment: add-pr-flush-mem-to-file (finalized patch)

Comment 10

[Luke Wagner [:luke]]

Great, thanks!  Here's the patch with the .def fix.

Comment 11

[(not currently active) Ted Mielczarek]

http://hg.mozilla.org/projects/nspr/rev/8638930ae241

We'll need to cut a new NSPR beta (not hard) and sync to m-c to pick this up.

Comment 12

[Luke Wagner [:luke]]

Awesome.  Will that happen in this bug or will we need a second bug?

Comment 13

[Wan-Teh Chang]

Comment on attachment 8336163 [details] [diff] [review]
add-pr-flush-mem-to-file (finalized patch)

Review of attachment 8336163 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this patch looks good. I suggest a change to the PR_SyncMemMap
function. Since the patch has already been checked in, I marked this
patch review+, but please write a new patch to address my review comments.

::: nsprpub/pr/include/prio.h
@@ +1860,5 @@
> + */
> +NSPR_API(PRStatus) PR_SyncMemMap(
> +    PRFileDesc *fd,
> +    void *addr,
> +    PRUint32 len);

IMPORTANT: based on your Windows implementation of
this function in ntmisc.c, I think this function should
take only two arguments:
    void *addr,
    PRUint32 len

The FlushFileBuffers part in your Windows implementation
(in ntmisc.c) is already provided by the existing PR_Sync
function.

::: nsprpub/pr/src/md/unix/unix.c
@@ +3644,5 @@
> +    PRUint32 len)
> +{
> +    if (msync(addr, len, MS_SYNC) == 0) {
> +        return PR_SUCCESS;
> +    } else {

Omit "else" after a return statement.

@@ +3649,5 @@
> +        if (errno == EINVAL) {
> +            PR_SetError(PR_INVALID_ARGUMENT_ERROR, errno);
> +        } else {
> +            PR_SetError(PR_UNKNOWN_ERROR, errno);
> +        }

I know you copied this error-mapping code from _MD_MemUnmap.
Please do this instead:

  _PR_MD_MAP_DEFAULT_ERROR(errno);

::: nsprpub/pr/src/nspr.def
@@ +228,5 @@
>  		PR_MakeDir;
>  		PR_Malloc;
>  		PR_MemMap;
>  		PR_MemUnmap;
> +		PR_SyncMemMap;

This symbol needs to be added to a new NSPR 4.10.3 section at the end
of this file, like this:

;+NSPR_4.10.3 {
;+      global:
                PR_SyncMemMap;
;+} NSPR_4.9.2;

Comment 14

[Luke Wagner [:luke]]

(In reply to Wan-Teh Chang from comment #13)
> The FlushFileBuffers part in your Windows implementation
> (in ntmisc.c) is already provided by the existing PR_Sync
> function.

The semantics of PR_SyncMemMap is that, on return, the memory in the specified range has been flushed to disk.  As the MSDN documentation explains
  http://msdn.microsoft.com/en-us/library/windows/desktop/aa366563%28v=vs.85%29.aspx
FlushViewOfFile is not sufficient to achieve this; FlushFileBuffers is required.  If I removed the FlushFileBuffers, then the burden would be on the caller to make an extra call which would end up penalizing the unix path where msync is sufficient.

Comment 15

[Luke Wagner [:luke]]

Ted: since you're the one checking this in: would you like me to post a patch to address the other nits or would you like to just do them yourself directly?

Comment 16

[(not currently active) Ted Mielczarek]

Luke: if you could provide a new patch with the requested changes that would be much easier for me.

Comment 17

[Wan-Teh Chang]

Attachment: Change PR_SyncMemMap to not sync the file, other cleanups

Luke, Ted: this patch implements all of my suggested changes.

Since you know more about this than I do, I'll let you make
the final call. I am offering this patch for your convenience.

It seems that at least, you need a comment in unix.c to
point out that fsync is not necessary. This will address
the potential confusion when someone compares the
PR_SyncMemMap implementations in ntmisc.c and unix.c.

Also, if the |fd| argument is necessary, it may be better
to replace it with PRFileMap *fmap, which would fit better
would the NSPR file-mapping API. Note that PR_MemMap takes
a PRFileMap *fmap instead of a PRFileDesc *fd argument.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8336966 [details] [diff] [review]
Change PR_SyncMemMap to not sync the file, other cleanups

Thank you for the patch!  As previously explained, FlushFileBuffers really is necessary.  A comment on msync would be fine.

Comment 19

[Luke Wagner [:luke]]

Also I do not think it really makes sense to pass the PRFileMap to PR_SyncMemMap: the underlying OS calls on Windows don't require it; on the contrary: the docs specificially indicate that you can close the file-mapping before unmaping the view which means that the client might not even have a PRFileMap (they may have already PR_CloseFileMap'd it).

Comment 20

[Wan-Teh Chang]

Attachment: Add and clarify comments, miscellaneous cleanups

The "Sync" in "PR_SyncMemMap" can be interpreted to mean "synchronize" (flush)
or "synchronously". Although you already said "Synchronously flush", it still
confused me, so I added a sentence to reinforce that point. I also explained
the purpose of the "PRFileDesc *fd" argument.

In ntmisc.c, I deleted the potential value of -1 for |osfd|. I found that
(HANDLE)-1 is INVALID_HANDLE_VALUE, which is a valid argument for
CreateFileMapping, but it is not a valid argument for FlushFileBuffers.

Comment 21

[Wan-Teh Chang]

Attachment: [Correct patch] Add and clarify comments, miscellaneous cleanups

I attached the wrong patch earlier.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 8338964 [details] [diff] [review]
[Correct patch] Add and clarify comments, miscellaneous cleanups

Review of attachment 8338964 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 23

[Wan-Teh Chang]

Comment on attachment 8338964 [details] [diff] [review]
[Correct patch] Add and clarify comments, miscellaneous cleanups

Review of attachment 8338964 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/projects/nspr/rev/4a6c30714ec1