On Wed, Oct 21, 2020 at 10:12 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > > D. If one does a partial page write of a page which is not uptodate, then > keep page locked and do not try to send multiple pages in that write. > If page is uptodate, then release page lock and continue to add more > pages to same request. > > IOW, if head page is partial (and it is not uptodate), we will just > send first WRITE with head page. Rest of the pages will go in second > WRITE and tail page could be locked if it was a partial write and > page was not uptodate. Please have a look at attached patch. Looks good. > I still some concerns though with error handling. Not sure what to > do about it. > > 1. What happens if WRITE fails. If we are writing a full page and we > already marked it as Uptodate (And not dirty), then we have page > cache in page where we wrote data but could not send it to disk > (and did not mark dirty as well). So if a user reads the page > back it might get cache copy or get old copy from disk (if page > cache copy was released). AFAICS this is what happens on write failure in current code IF the page was uptodate previously. Moving the SetPageUptodate() before the WRITE makes this happen in all cases. On write failure the page the uptodate flag should be cleared, which should partially solve the above issue. There still remains a window where a concurrent read or load would get the wrong data, but I don't think anybody cares (same happens with buffered write). > 2. What happens if it is a partial page write to an Uptodate page > in cache and that WRITE fails. Now we have same error scenario > as 1. In fact this is true for even current code and not > necessarily a new scenario. Same as above: need to clear uptodate flag. > 3. Current code marks a page Uptodate upon WRITE completion if > it was full page WRITE. What if page was uptodate to begin > with and write fails. So current code will not mark it > Uptodate but it is already uptodate and we have same problem as 1. Yes. > Apart from above, there are some other concerns as well. > > So with this patch, if a page is Uptodate we drop lock and send WRITE. > Otherwise we keep page lock and send WRITE. This should probably be > fine from read or fault read point of view. Given we are holding inode > lock, that means write path is not a problem as well. But > > What if page is redirtied through a write mapping > ------------------------------------------------- > If page is redirtied through writable mmap, then two writes for same > page can go in any order. But in synchronous write we are carrying > pointer to page cache page, so it probably does not matter. We will > just write same data twice. It's not that simple. Data dirtied through an mmap will be written back using a temporary page. So a WRITE request with such a page can be in flight while a write(2) triggers a synchronous WRITE request for the same data. The two WRITEs are not ordered in any way and in fact the page lock doesn't help, so this is not a new issue. OTOH this does not appear to be a problem in real life, since without msync() it is not guaranteed that the memory mapping is synchronized with the backing file (Linux has stronger guarantees, but test suites such as fsx assume the lesser guarantees by POSIX). This could be fixed to conform to the stronger coherency guarantee by calling fuse_wait_on_page_writeback() after having gotten a locked page in the synchronous writeback path. > > What about races with direct_IO read > ------------------------------------ > If a WRITE is in progress, it is probably not marked dirty so > generic_file_read_iter() will probably not block on > filemap_write_and_wait_range() and continue mapping->a_ops->direct_IO(). > And that means it can read previous disk data before this WRITE is > complete. Synchronous write vs. direct IO read shouldn't be a problem as long as the server provides a coherent view of the file to both. Thanks, Miklos
