{"thread":{"id":"24032","subject":"Explanation for dropping write-back in mmap","startedAt":"2010-06-07T15:16:18Z","lastAt":"2010-06-07T23:13:14Z","messageCount":5,"participants":["Ramkumar Ramachandra","Shawn O. Pearce","Junio C Hamano","David Michael Barr"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"143165","messageId":"AANLkTilrinkNo9awJ9Wso4tXzQKghWEd2bfDRhPHPOga@mail.gmail.com","threadId":"24032","inReplyTo":null,"subject":"Explanation for dropping write-back in mmap","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-06-07T15:16:18Z","receivedAt":"2010-06-07T15:16:18Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nIn the commit f48000fcbe100, you've forced the caller to use\nMAP_PRIVATE in the alternative mmap implementation dropping write-back\nsupport. Could you kindly explain the rationale for this? David's SVN\nexporter uses mmap with MAP_SHARED, and we figured that the merge\nimmediately breaks this functionality.\n\nNoticed-by: Jonathan Nieder \n\n-----------------------------------------------------------------------------------------------------------\ncommit f48000fcbe1009c18f1cc46e56cde2cb632071fa\nAuthor: Junio C Hamano \nDate: Sat Oct 8 15:54:36 2005 -0700\n\n Yank writing-back support from gitfakemmap.\n\n We do not write through our use of mmap(), so make sure callers pass\n MAP_PRIVATE and remove support for writing changes back.\n\n Signed-off-by: Junio C Hamano \n-----------------------------------------------------------------------------------------------------------\n\nThanks.\n\n-- Ram\n"},{"id":"143172","messageId":"20100607155039.GG14847@spearce.org","threadId":"24032","inReplyTo":"AANLkTilrinkNo9awJ9Wso4tXzQKghWEd2bfDRhPHPOga@mail.gmail.com","subject":"Re: Explanation for dropping write-back in mmap","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-06-07T15:50:39Z","receivedAt":"2010-06-07T15:50:39Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Ramkumar Ramachandra wrote:\n> In the commit f48000fcbe100, you've forced the caller to use\n> MAP_PRIVATE in the alternative mmap implementation dropping write-back\n> support. Could you kindly explain the rationale for this? David's SVN\n> exporter uses mmap with MAP_SHARED, and we figured that the merge\n> immediately breaks this functionality.\n> \n> Noticed-by: Jonathan Nieder \n> \n> -----------------------------------------------------------------------------------------------------------\n> commit f48000fcbe1009c18f1cc46e56cde2cb632071fa\n> Author: Junio C Hamano \n> Date: Sat Oct 8 15:54:36 2005 -0700\n> \n> Yank writing-back support from gitfakemmap.\n> \n> We do not write through our use of mmap(), so make sure callers pass\n> MAP_PRIVATE and remove support for writing changes back.\n> \n> Signed-off-by: Junio C Hamano \n> -----------------------------------------------------------------------------------------------------------\n\n\nIt got dropped because we never really used it. In almost every\nlocation we were only mapping a file for reading, but were writing\nthrough normal IO write functions. The Windows emulation code at\nthat time was using malloc()+read() to emulate mmap(), and thus\nany modifications made to the buffer would not be flushed back.\n\nThe one place where we were doing both was fast-import, but it was\nplaying loose with the mmap consistency rules. I think we have\nfixed that code since then to ensure the data stays consistent on\nall platforms.\n\nI would strongly suggest finding another way to implement the SVN\nexporter, without using MAP_SHARED.\n\n-- \nShawn.\n"},{"id":"143173","messageId":"7vbpbmx2er.fsf@alter.siamese.dyndns.org","threadId":"24032","inReplyTo":"AANLkTilrinkNo9awJ9Wso4tXzQKghWEd2bfDRhPHPOga@mail.gmail.com","subject":"Re: Explanation for dropping write-back in mmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-07T15:55:56Z","receivedAt":"2010-06-07T15:55:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> In the commit f48000fcbe100, you've forced the caller to use\n> MAP_PRIVATE in the alternative mmap implementation dropping write-back\n> support.\n> ...\n> commit f48000fcbe1009c18f1cc46e56cde2cb632071fa\n> Author: Junio C Hamano \n> Date: Sat Oct 8 15:54:36 2005 -0700\n>\n> Yank writing-back support from gitfakemmap.\n>\n> We do not write through our use of mmap(), so make sure callers pass\n> MAP_PRIVATE and remove support for writing changes back.\n\nThat's a \n\nI don't think we _dropped_ a _working_ support that allowed shared\nmapping. IIRC the implementation emulated only private mapping well\nenough to support the use of mmap() in our codebase (iow, instead of\nallocating a buffer and reading into it and possibly mucking with it\nwithout affecting outside world, map it to read and then possibly mucking\nwith it), but lacked input validation to make sure that no caller\nmistakenly thinks the implementation could satisfy non private mapping.\n\nAlso I don't think I did this without telling other people---it would be a\nlot more likely that somebody else noticed it and the issue was discussed\non the list and resulted in this commit. I would check the commit date\nand see the discussion around that time if I were you to learn the\nbackstory.\n"},{"id":"143179","messageId":"AANLkTinJE5blkIRPp5NDKEWVm2uv-Yelt_niYko6XZa_@mail.gmail.com","threadId":"24032","inReplyTo":"7vbpbmx2er.fsf@alter.siamese.dyndns.org","subject":"Re: Explanation for dropping write-back in mmap","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-06-07T19:01:50Z","receivedAt":"2010-06-07T19:01:50Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio and Shawn,\n\nShawn O. Pearce wrote:\n> I would strongly suggest finding another way to implement the SVN\n> exporter, without using MAP_SHARED.\n\nDavid has already started working on using realloc + persist, but I'd\nlike to know the reason for your recommendation- is it simply because\nmmap with MAP_SHARED is difficult to port, or is there some other\nreason as well?\n\nJunio C Hamano wrote:\n> I don't think we _dropped_ a _working_ support that allowed shared\n> mapping.  IIRC the implementation emulated only private mapping well\n> enough to support the use of mmap() in our codebase (iow, instead of\n> allocating a buffer and reading into it and possibly mucking with it\n> without affecting outside world, map it to read and then possibly mucking\n> with it), but lacked input validation to make sure that no caller\n> mistakenly thinks the implementation could satisfy non private mapping.\n\nAh, so you did it for safety/ sanity reasons back then.\n\n> Also I don't think I did this without telling other people---it would be a\n> lot more likely that somebody else noticed it and the issue was discussed\n> on the list and resulted in this commit.  I would check the commit date\n> and see the discussion around that time if I were you to learn the\n> backstory.\n\nRight. Thanks for this- I read the \"First cut at git port to Cygwin\"\nthread and learnt the backstory.\n\n-- Ram\n"},{"id":"143194","messageId":"AD1AE06F-0986-46C4-BCEF-A496EB5CEB98@cordelta.com","threadId":"24032","inReplyTo":"AANLkTinJE5blkIRPp5NDKEWVm2uv-Yelt_niYko6XZa_@mail.gmail.com","subject":"Re: Explanation for dropping write-back in mmap","fromName":"David Michael Barr","fromEmail":"david.barr@cordelta.com","sentAt":"2010-06-07T23:13:14Z","receivedAt":"2010-06-07T23:13:14Z","isPatch":false,"sender":{"key":"david.barr@cordelta.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"Hi Ram,\n\n> Shawn O. Pearce wrote:\n>> I would strongly suggest finding another way to implement the SVN\n>> exporter, without using MAP_SHARED.\n> \n> David has already started working on using realloc + persist, but I'd\n> like to know the reason for your recommendation- is it simply because\n> mmap with MAP_SHARED is difficult to port, or is there some other\n> reason as well?\n\nI had a think about it, this would be a good chance to enforce append-only writes.\nThe general strategy would be to write batches of objects for each commit.\nAll of the data structures in use support this pattern.\nHowever, I'll need to refactor string_pool slightly to make it work cleanly.\n\n> Junio C Hamano wrote:\n>> I don't think we _dropped_ a _working_ support that allowed shared\n>> mapping. IIRC the implementation emulated only private mapping well\n>> enough to support the use of mmap() in our codebase (iow, instead of\n>> allocating a buffer and reading into it and possibly mucking with it\n>> without affecting outside world, map it to read and then possibly mucking\n>> with it), but lacked input validation to make sure that no caller\n>> mistakenly thinks the implementation could satisfy non private mapping.\n> \n> Ah, so you did it for safety/ sanity reasons back then.\n\nHaving not read the backstory, this was my guess as to the rationale.\n\n--\nDavid Barr.\n"}]}