Re: [PATCH 2/2] iomap: Support inline data with block size < page size

[Gao Xiang <hsiangkao@linux.alibaba.com>]

Hi Matthew,

On Sat, Jul 24, 2021 at 04:44:35AM +0100, Matthew Wilcox (Oracle) wrote:
> Remove the restriction that inline data must start on a page boundary
> in a file.  This allows, for example, the first 2KiB to be stored out
> of line and the trailing 30 bytes to be stored inline.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/iomap/buffered-io.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 7bd8e5de996d..d7d6af29af7f 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -209,25 +209,23 @@ static int iomap_read_inline_data(struct inode *inode, struct page *page,
>  		struct iomap *iomap, loff_t pos)
>  {
>  	size_t size = iomap->length + iomap->offset - pos;
> +	size_t poff = offset_in_page(pos);
>  	void *addr;
>  
>  	if (PageUptodate(page))
> -		return PAGE_SIZE;
> +		return PAGE_SIZE - poff;
>  
> -	/* inline data must start page aligned in the file */
> -	if (WARN_ON_ONCE(offset_in_page(pos)))
> -		return -EIO;
>  	if (WARN_ON_ONCE(!iomap_inline_data_size_valid(iomap)))
>  		return -EIO;
> -	if (WARN_ON_ONCE(page_has_private(page)))
> -		return -EIO;
> +	if (poff > 0)
> +		iomap_page_create(inode, page);

Thanks for the patch!

Previously I'd like to skip the leading uptodate blocks and update the
extent it covers that is due to already exist iop. If we get rid of the
offset_in_page(pos) restriction like this, I'm not sure if we (or some
other fs users) could face something like below (due to somewhat buggy
fs users likewise):

 [0 - 4096)    plain block

 [4096 - 4608)  tail INLINE 1 (e.g. by mistake or just splitted
                                    .iomap_begin() report.)
 [4608 - 5120]  tail INLINE 2

with this code iomap_set_range_uptodate() wouldn't behave correctly.

In addition, currently EROFS cannot test such path (since EROFS is
page-sized block only for now) as Darrick said in the previous reply,
so I think it would be better together with the folio patchset and
targeted for the corresponding merge window, so I can test iomap
supported EROFS with the new folio support together (That also give
me some time to support sub-pagesized uncompressed blocks...)

>  
> -	addr = kmap_atomic(page);
> +	addr = kmap_atomic(page) + poff;
>  	memcpy(addr, iomap_inline_buf(iomap, pos), size);
> -	memset(addr + size, 0, PAGE_SIZE - size);
> +	memset(addr + size, 0, PAGE_SIZE - poff - size);
>  	kunmap_atomic(addr);

As my limited understanding, this may need to be fixed, since it
doesn't match kmap_atomic(page)...

Thanks,
Gao Xiang

> -	SetPageUptodate(page);
> -	return PAGE_SIZE;
> +	iomap_set_range_uptodate(page, poff, PAGE_SIZE - poff);
> +	return PAGE_SIZE - poff;
>  }
>  
>  static inline bool iomap_block_needs_zeroing(struct inode *inode,
> -- 
> 2.30.2
>