On Tue, Jan 11, 2022 at 03:38:24PM -0800, John Hubbard wrote: > On 1/11/22 13:59, Minchan Kim wrote: > ... > > > > > Marking pages dirty after pinning them is a pre-existing area of > > > > > problems. See the long-running LWN articles about get_user_pages() [1]. > > > > > > > > Oh, Do you mean marking page dirty in DIO path is already problems? > > > > > > ^ marking page dirty too late in DIO path > > > > > > Typo fix. > > > > I looked though the articles but couldn't find dots to connetct > > issues with this MADV_FREE issue. However, man page shows a clue > > The area covered in those articles is about the fact that file system > and block are not safely interacting with pinned memory. Even today. > So I'm trying to make sure you're aware of that before you go too far > in that direction. > > > why it's fine. > > > > ``` > > O_DIRECT I/Os should never be run concurrently with the fork(2) system call, if the memory buffer is a private map‐ > > ping (i.e., any mapping created with the mmap(2) MAP_PRIVATE flag; this includes memory allocated on the heap and > > statically allocated buffers). Any such I/Os, whether submitted via an asynchronous I/O interface or from another > > thread in the process, should be completed before fork(2) is called. Failure to do so can result in data corruption > > and undefined behavior in parent and child processes. > > > > ``` > > > > I think it would make the copy_present_pte's page_dup_rmap safe. > > I'd have to see this in patch form, because I'm not quite able to visualize it yet. It would be great if you read though the original patch description. Since v2 had a little change to consider mutiple maps among parent and child, it would introduce a little mistmatch with the description but it's still quite good to explain current problem. https://lore.kernel.org/all/20220105233440.63361-1-mfo@xxxxxxxxxxxxx/T/#u Problem is MADV_FREEed anonymous memory is supposed to work based on dirtiness came from the user process's page table bit or PageDirty. Since VM can't see the dirty, it just discards the anonymous memory instead of swappoing out. Thus, the dirtiness is key to work correctly. However, DIO didn't make the page Dirty yet until IO is completed and at the same time, the store operation didn't go though via user process's page table regardless of DMA or other way. It makes VM could decide just drop the page since it didn't see any dirtiness from the page. So it turns out enduser would be surprised because the read syscall with DIO was completed but the data was zero rather than latest uptodate data. To prevent the problem, the patch suggested to compare page_mapcount with page_count since it expects any additional reference of the page means someone is doing accessing the memory so in this case, not discarding the page. However, Yu pointed out page_count and page_mapcount could be reordered in copy_page_range, for example. So I am looking for the solution(one would be adding memory barrier right before page_dup_rmap but I'd like to avoid it if we have other idea). And then man page says forking under going DIO would be already prohibited so the concern raised would be void, IIUC. Hope this helps your understanding. Thanks! work
