On Thu, Sep 24, 2020 at 02:59:00PM +0100, Matthew Wilcox wrote: > On Thu, Sep 24, 2020 at 09:12:35AM -0400, Brian Foster wrote: > > On Thu, Sep 24, 2020 at 01:56:08PM +0100, Matthew Wilcox (Oracle) wrote: > > > For filesystems with block size < page size, we need to set all the > > > per-block uptodate bits if the page was already uptodate at the time > > > we create the per-block metadata. This can happen if the page is > > > invalidated (eg by a write to drop_caches) but ultimately not removed > > > from the page cache. > > > > > > This is a data corruption issue as page writeback skips blocks which > > > are marked !uptodate. > > > > Thanks. Based on my testing of clearing PageUptodate here I suspect this > > will similarly prevent the problem, but I'll give this a test > > nonetheless. > > > > I am a little curious why we'd prefer to fill the iop here rather than > > just clear the page state if the iop data has been released. If the page > > is partially uptodate, then we end up having to re-read the page > > anyways, right? OTOH, I guess this behavior is more consistent with page > > size == block size filesystems where iop wouldn't exist and we just go > > by page state, so perhaps that makes more sense. > > Well, it's _true_ ... the PageUptodate bit means that every byte in this > page is at least as new as every byte on storage. There's no need to > re-read it, which is what we'll do if we ClearPageUptodate. > Agreed, Pagetodate(page) means the whole page content is available now, see create_page_buffers() -> create_empty_buffers() and try_to_free_buffers() (much like .releasepage()) in buffer head approach. Reviewed-by: Gao Xiang <hsiangkao@xxxxxxxxxx> Thanks, Gao Xiang
