Re: [PATCH] iomap: Set all uptodate bits for an Uptodate page

[Brian Foster <bfoster@redhat.com>]

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.
> 
> Fixes: 9dc55f1389f9 ("iomap: add support for sub-pagesize buffered I/O without buffer heads")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reported-by: Qian Cai <cai@redhat.com>
> Cc: Brian Foster <bfoster@redhat.com>
> ---
>  fs/iomap/buffered-io.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 8b6cca7e34e4..8180061b9e16 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -60,6 +60,8 @@ iomap_page_create(struct inode *inode, struct page *page)
>  	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
>  			GFP_NOFS | __GFP_NOFAIL);
>  	spin_lock_init(&iop->uptodate_lock);
> +	if (PageUptodate(page))
> +		bitmap_fill(iop->uptodate, nr_blocks);

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.

Brian

>  	attach_page_private(page, iop);
>  	return iop;
>  }
> -- 
> 2.28.0
>