linux-erofs.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Gao Xiang <hsiangkao@linux.alibaba.com>
To: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-erofs@lists.ozlabs.org
Subject: Re: [PATCH 2/2] iomap: Support inline data with block size < page size
Date: Sat, 24 Jul 2021 12:46:45 +0800	[thread overview]
Message-ID: <YPubNbDS0KgUALtt@B-P7TQMD6M-0146.local> (raw)
In-Reply-To: <20210724034435.2854295-3-willy@infradead.org>

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
> 

  reply	other threads:[~2021-07-24  4:47 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-24  3:44 [PATCH 0/2] iomap: make inline data support more flexible Matthew Wilcox (Oracle)
2021-07-24  3:44 ` [PATCH 1/2] iomap: Support file tail packing Matthew Wilcox (Oracle)
2021-07-24  3:44 ` [PATCH 2/2] iomap: Support inline data with block size < page size Matthew Wilcox (Oracle)
2021-07-24  4:46   ` Gao Xiang [this message]
2021-07-24  5:11     ` Gao Xiang
2021-07-24  7:52       ` Gao Xiang
2021-07-24 14:03     ` Matthew Wilcox
2021-07-24 15:14       ` Gao Xiang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YPubNbDS0KgUALtt@B-P7TQMD6M-0146.local \
    --to=hsiangkao@linux.alibaba.com \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH 2/2] iomap: Support inline data with block size < page size' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).