Explanation for dropping write-back in mmap

Explanation for dropping write-back in mmap

[Ramkumar Ramachandra]

Hi Junio,

In the commit f48000fcbe100, you've forced the caller to use
MAP_PRIVATE in the alternative mmap implementation dropping write-back
support. Could you kindly explain the rationale for this? David's SVN
exporter uses mmap with MAP_SHARED, and we figured that the merge
immediately breaks this functionality.

Noticed-by: Jonathan Nieder <jrnieder@gmail.com>

-----------------------------------------------------------------------------------------------------------
commit f48000fcbe1009c18f1cc46e56cde2cb632071fa
Author: Junio C Hamano <junkio@cox.net>
Date:   Sat Oct 8 15:54:36 2005 -0700

    Yank writing-back support from gitfakemmap.

    We do not write through our use of mmap(), so make sure callers pass
    MAP_PRIVATE and remove support for writing changes back.

    Signed-off-by: Junio C Hamano <junkio@cox.net>
-----------------------------------------------------------------------------------------------------------

Thanks.

-- Ram

Re: Explanation for dropping write-back in mmap

[Shawn O. Pearce]

Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> In the commit f48000fcbe100, you've forced the caller to use
> MAP_PRIVATE in the alternative mmap implementation dropping write-back
> support. Could you kindly explain the rationale for this? David's SVN
> exporter uses mmap with MAP_SHARED, and we figured that the merge
> immediately breaks this functionality.
> 
> Noticed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> -----------------------------------------------------------------------------------------------------------
> commit f48000fcbe1009c18f1cc46e56cde2cb632071fa
> Author: Junio C Hamano <junkio@cox.net>
> Date:   Sat Oct 8 15:54:36 2005 -0700
> 
>     Yank writing-back support from gitfakemmap.
> 
>     We do not write through our use of mmap(), so make sure callers pass
>     MAP_PRIVATE and remove support for writing changes back.
> 
>     Signed-off-by: Junio C Hamano <junkio@cox.net>
> -----------------------------------------------------------------------------------------------------------


It got dropped because we never really used it.  In almost every
location we were only mapping a file for reading, but were writing
through normal IO write functions.  The Windows emulation code at
that time was using malloc()+read() to emulate mmap(), and thus
any modifications made to the buffer would not be flushed back.

The one place where we were doing both was fast-import, but it was
playing loose with the mmap consistency rules.  I think we have
fixed that code since then to ensure the data stays consistent on
all platforms.

I would strongly suggest finding another way to implement the SVN
exporter, without using MAP_SHARED.

-- 
Shawn.

Re: Explanation for dropping write-back in mmap

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> In the commit f48000fcbe100, you've forced the caller to use
> MAP_PRIVATE in the alternative mmap implementation dropping write-back
> support.
> ...
> commit f48000fcbe1009c18f1cc46e56cde2cb632071fa
> Author: Junio C Hamano <junkio@cox.net>
> Date:   Sat Oct 8 15:54:36 2005 -0700
>
>     Yank writing-back support from gitfakemmap.
>
>     We do not write through our use of mmap(), so make sure callers pass
>     MAP_PRIVATE and remove support for writing changes back.

That's a 

I don't think we _dropped_ a _working_ support that allowed shared
mapping.  IIRC the implementation emulated only private mapping well
enough to support the use of mmap() in our codebase (iow, instead of
allocating a buffer and reading into it and possibly mucking with it
without affecting outside world, map it to read and then possibly mucking
with it), but lacked input validation to make sure that no caller
mistakenly thinks the implementation could satisfy non private mapping.

Also I don't think I did this without telling other people---it would be a
lot more likely that somebody else noticed it and the issue was discussed
on the list and resulted in this commit.  I would check the commit date
and see the discussion around that time if I were you to learn the
backstory.

Re: Explanation for dropping write-back in mmap

[Ramkumar Ramachandra]

Hi Junio and Shawn,

Shawn O. Pearce wrote:
> I would strongly suggest finding another way to implement the SVN
> exporter, without using MAP_SHARED.

David has already started working on using realloc + persist, but I'd
like to know the reason for your recommendation- is it simply because
mmap with MAP_SHARED is difficult to port, or is there some other
reason as well?

Junio C Hamano wrote:
> I don't think we _dropped_ a _working_ support that allowed shared
> mapping.  IIRC the implementation emulated only private mapping well
> enough to support the use of mmap() in our codebase (iow, instead of
> allocating a buffer and reading into it and possibly mucking with it
> without affecting outside world, map it to read and then possibly mucking
> with it), but lacked input validation to make sure that no caller
> mistakenly thinks the implementation could satisfy non private mapping.

Ah, so you did it for safety/ sanity reasons back then.

> Also I don't think I did this without telling other people---it would be a
> lot more likely that somebody else noticed it and the issue was discussed
> on the list and resulted in this commit.  I would check the commit date
> and see the discussion around that time if I were you to learn the
> backstory.

Right. Thanks for this- I read the "First cut at git port to Cygwin"
thread and learnt the backstory.

-- Ram

Re: Explanation for dropping write-back in mmap

[David Michael Barr]

Hi Ram,

> Shawn O. Pearce wrote:
>> I would strongly suggest finding another way to implement the SVN
>> exporter, without using MAP_SHARED.
> 
> David has already started working on using realloc + persist, but I'd
> like to know the reason for your recommendation- is it simply because
> mmap with MAP_SHARED is difficult to port, or is there some other
> reason as well?

I had a think about it, this would be a good chance to enforce append-only writes.
The general strategy would be to write batches of objects for each commit.
All of the data structures in use support this pattern.
However, I'll need to refactor string_pool slightly to make it work cleanly.

> Junio C Hamano wrote:
>> I don't think we _dropped_ a _working_ support that allowed shared
>> mapping.  IIRC the implementation emulated only private mapping well
>> enough to support the use of mmap() in our codebase (iow, instead of
>> allocating a buffer and reading into it and possibly mucking with it
>> without affecting outside world, map it to read and then possibly mucking
>> with it), but lacked input validation to make sure that no caller
>> mistakenly thinks the implementation could satisfy non private mapping.
> 
> Ah, so you did it for safety/ sanity reasons back then.

Having not read the backstory, this was my guess as to the rationale.

--
David Barr.