linux-erofs.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] iomap: make inline data support more flexible
@ 2021-07-24  3:44 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)
  0 siblings, 2 replies; 8+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-07-24  3:44 UTC (permalink / raw)
  To: linux-fsdevel, linux-erofs, linux-xfs; +Cc: Matthew Wilcox (Oracle)

The first patch seems to be where Gao Xiang, Christoph and I have
settled for immediate support of EROFS tails.  The second is a prequel
to the folio work.  Because folios are variable size, if we have, eg,
an 8.1KiB file, we must support reading the first two pages of the folio
from blocks and then the last 100 bytes from the tailpack.  That means
effectively supporting block size < page size.  I thought it cleaner to
separate it out, and maybe that can go in this round.

The folio patches are rebased on top of all this; if you're hungry to
see them, they can be found at
https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/folio
as usual.  I haven't applied all the R-b yet (and I should probably
figure out which ones still apply since I did some substantial changes
to a couple of the patches).

Gao Xiang (1):
  iomap: Support file tail packing

Matthew Wilcox (Oracle) (1):
  iomap: Support inline data with block size < page size

 fs/iomap/buffered-io.c | 46 +++++++++++++++++++++++++-----------------
 fs/iomap/direct-io.c   | 10 +++++----
 include/linux/iomap.h  | 14 +++++++++++++
 3 files changed, 47 insertions(+), 23 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] iomap: Support file tail packing
  2021-07-24  3:44 [PATCH 0/2] iomap: make inline data support more flexible Matthew Wilcox (Oracle)
@ 2021-07-24  3:44 ` Matthew Wilcox (Oracle)
  2021-07-24  3:44 ` [PATCH 2/2] iomap: Support inline data with block size < page size Matthew Wilcox (Oracle)
  1 sibling, 0 replies; 8+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-07-24  3:44 UTC (permalink / raw)
  To: linux-fsdevel, linux-erofs, linux-xfs; +Cc: Matthew Wilcox

From: Gao Xiang <hsiangkao@linux.alibaba.com>

Add support for reading inline data content into the page cache from
nonzero page-aligned file offsets.  This enables the EROFS tailpacking
mode where the last few bytes of the file are stored right after the
inode.

The buffered write path remains untouched since EROFS cannot be used
for testing. It'd be better to be implemented if upcoming real users
care and provide a real pattern rather than leave untested dead code
around.

Tested-by: Huang Jianan <huangjianan@oppo.com> # erofs
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/iomap/buffered-io.c | 42 ++++++++++++++++++++++++++----------------
 fs/iomap/direct-io.c   | 10 ++++++----
 include/linux/iomap.h  | 14 ++++++++++++++
 3 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index a463b41c0a16..7bd8e5de996d 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -205,25 +205,29 @@ struct iomap_readpage_ctx {
 	struct readahead_control *rac;
 };
 
-static void
-iomap_read_inline_data(struct inode *inode, struct page *page,
-		struct iomap *iomap)
+static int iomap_read_inline_data(struct inode *inode, struct page *page,
+		struct iomap *iomap, loff_t pos)
 {
-	size_t size = i_size_read(inode);
+	size_t size = iomap->length + iomap->offset - pos;
 	void *addr;
 
 	if (PageUptodate(page))
-		return;
+		return PAGE_SIZE;
 
-	BUG_ON(page_has_private(page));
-	BUG_ON(page->index);
-	BUG_ON(size > PAGE_SIZE - offset_in_page(iomap->inline_data));
+	/* 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;
 
 	addr = kmap_atomic(page);
-	memcpy(addr, iomap->inline_data, size);
+	memcpy(addr, iomap_inline_buf(iomap, pos), size);
 	memset(addr + size, 0, PAGE_SIZE - size);
 	kunmap_atomic(addr);
 	SetPageUptodate(page);
+	return PAGE_SIZE;
 }
 
 static inline bool iomap_block_needs_zeroing(struct inode *inode,
@@ -245,11 +249,8 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 	unsigned poff, plen;
 	sector_t sector;
 
-	if (iomap->type == IOMAP_INLINE) {
-		WARN_ON_ONCE(pos);
-		iomap_read_inline_data(inode, page, iomap);
-		return PAGE_SIZE;
-	}
+	if (iomap->type == IOMAP_INLINE)
+		return iomap_read_inline_data(inode, page, iomap, pos);
 
 	/* zero post-eof blocks as the page may be mapped */
 	iop = iomap_page_create(inode, page);
@@ -581,6 +582,15 @@ __iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, int flags,
 	return 0;
 }
 
+static int iomap_write_begin_inline(struct inode *inode,
+		struct page *page, struct iomap *srcmap)
+{
+	/* needs more work for the tailpacking case, disable for now */
+	if (WARN_ON_ONCE(srcmap->offset != 0))
+		return -EIO;
+	return iomap_read_inline_data(inode, page, srcmap, 0);
+}
+
 static int
 iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 		struct page **pagep, struct iomap *iomap, struct iomap *srcmap)
@@ -610,14 +620,14 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 	}
 
 	if (srcmap->type == IOMAP_INLINE)
-		iomap_read_inline_data(inode, page, srcmap);
+		status = iomap_write_begin_inline(inode, page, srcmap);
 	else if (iomap->flags & IOMAP_F_BUFFER_HEAD)
 		status = __block_write_begin_int(page, pos, len, NULL, srcmap);
 	else
 		status = __iomap_write_begin(inode, pos, len, flags, page,
 				srcmap);
 
-	if (unlikely(status))
+	if (unlikely(status < 0))
 		goto out_unlock;
 
 	*pagep = page;
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 9398b8c31323..a6aaea2764a5 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -378,23 +378,25 @@ iomap_dio_inline_actor(struct inode *inode, loff_t pos, loff_t length,
 		struct iomap_dio *dio, struct iomap *iomap)
 {
 	struct iov_iter *iter = dio->submit.iter;
+	void *dst = iomap_inline_buf(iomap, pos);
 	size_t copied;
 
-	BUG_ON(pos + length > PAGE_SIZE - offset_in_page(iomap->inline_data));
+	if (WARN_ON_ONCE(!iomap_inline_data_size_valid(iomap)))
+		return -EIO;
 
 	if (dio->flags & IOMAP_DIO_WRITE) {
 		loff_t size = inode->i_size;
 
 		if (pos > size)
-			memset(iomap->inline_data + size, 0, pos - size);
-		copied = copy_from_iter(iomap->inline_data + pos, length, iter);
+			memset(iomap_inline_buf(iomap, size), 0, pos - size);
+		copied = copy_from_iter(dst, length, iter);
 		if (copied) {
 			if (pos + copied > size)
 				i_size_write(inode, pos + copied);
 			mark_inode_dirty(inode);
 		}
 	} else {
-		copied = copy_to_iter(iomap->inline_data + pos, length, iter);
+		copied = copy_to_iter(dst, length, iter);
 	}
 	dio->size += copied;
 	return copied;
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 479c1da3e221..3cfe787fbf50 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -97,6 +97,20 @@ iomap_sector(struct iomap *iomap, loff_t pos)
 	return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT;
 }
 
+static inline void *iomap_inline_buf(const struct iomap *iomap, loff_t pos)
+{
+	return iomap->inline_data - iomap->offset + pos;
+}
+
+/*
+ * iomap->inline_data is a potentially kmapped page, ensure it never crosses a
+ * page boundary.
+ */
+static inline bool iomap_inline_data_size_valid(const struct iomap *iomap)
+{
+	return iomap->length <= PAGE_SIZE - offset_in_page(iomap->inline_data);
+}
+
 /*
  * When a filesystem sets page_ops in an iomap mapping it returns, page_prepare
  * and page_done will be called for each page written to.  This only applies to
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] iomap: Support inline data with block size < page size
  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 ` Matthew Wilcox (Oracle)
  2021-07-24  4:46   ` Gao Xiang
  1 sibling, 1 reply; 8+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-07-24  3:44 UTC (permalink / raw)
  To: linux-fsdevel, linux-erofs, linux-xfs; +Cc: Matthew Wilcox (Oracle)

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);
 
-	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);
-	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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] iomap: Support inline data with block size < page size
  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
  2021-07-24  5:11     ` Gao Xiang
  2021-07-24 14:03     ` Matthew Wilcox
  0 siblings, 2 replies; 8+ messages in thread
From: Gao Xiang @ 2021-07-24  4:46 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle); +Cc: linux-fsdevel, linux-xfs, linux-erofs

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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] iomap: Support inline data with block size < page size
  2021-07-24  4:46   ` Gao Xiang
@ 2021-07-24  5:11     ` Gao Xiang
  2021-07-24  7:52       ` Gao Xiang
  2021-07-24 14:03     ` Matthew Wilcox
  1 sibling, 1 reply; 8+ messages in thread
From: Gao Xiang @ 2021-07-24  5:11 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle); +Cc: linux-fsdevel, linux-xfs, linux-erofs

On Sat, Jul 24, 2021 at 12:46:45PM +0800, Gao Xiang wrote:
> 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
> 

(cont.)

So I think without the !offset_in_page(pos) restriction, at least we
may need to add something like:

if (WARN_ON_ONCE(size != i_size_read(inode) - iomap->offset))
	return -EIO;

to the approach to detect such cases at least but that is no need with
page-sized and !offset_in_page(pos) restriction.

> 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
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] iomap: Support inline data with block size < page size
  2021-07-24  5:11     ` Gao Xiang
@ 2021-07-24  7:52       ` Gao Xiang
  0 siblings, 0 replies; 8+ messages in thread
From: Gao Xiang @ 2021-07-24  7:52 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle); +Cc: linux-fsdevel, linux-xfs, linux-erofs

On Sat, Jul 24, 2021 at 01:11:09PM +0800, Gao Xiang wrote:
> On Sat, Jul 24, 2021 at 12:46:45PM +0800, Gao Xiang wrote:

...

> > 
> > 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
> > 
> 
> (cont.)
> 
> So I think without the !offset_in_page(pos) restriction, at least we
> may need to add something like:
> 
> if (WARN_ON_ONCE(size != i_size_read(inode) - iomap->offset))
> 	return -EIO;
> 
> to the approach to detect such cases at least but that is no need with
> page-sized and !offset_in_page(pos) restriction.
> 

Never mind, after rethinking with clear head, I think I was overthinking
this part and it shouldn't behave like this. Sorry about the noise above.

> > 
> > >  
> > > -	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
> > > 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] iomap: Support inline data with block size < page size
  2021-07-24  4:46   ` Gao Xiang
  2021-07-24  5:11     ` Gao Xiang
@ 2021-07-24 14:03     ` Matthew Wilcox
  2021-07-24 15:14       ` Gao Xiang
  1 sibling, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2021-07-24 14:03 UTC (permalink / raw)
  To: linux-fsdevel, linux-erofs, linux-xfs

On Sat, Jul 24, 2021 at 12:46:45PM +0800, Gao Xiang wrote:
> 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

My assumption is that an INLINE extent is <= block_size.  Otherwise
the first part of the extent would be not-inline.  So this would be
a bug in the filesystem; [4096-4608) should not be an inline extent.

> 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...)

Do you want to test erofs with multi-page folios?  That might be
even more interesting than block size < page size.

> > -	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)...

void kunmap_local_indexed(void *vaddr)
{
        unsigned long addr = (unsigned long) vaddr & PAGE_MASK;

so it's fine to unmap any address in the page.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] iomap: Support inline data with block size < page size
  2021-07-24 14:03     ` Matthew Wilcox
@ 2021-07-24 15:14       ` Gao Xiang
  0 siblings, 0 replies; 8+ messages in thread
From: Gao Xiang @ 2021-07-24 15:14 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: linux-fsdevel, linux-xfs, linux-erofs

On Sat, Jul 24, 2021 at 03:03:56PM +0100, Matthew Wilcox wrote:
> On Sat, Jul 24, 2021 at 12:46:45PM +0800, Gao Xiang wrote:
> > 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
> 
> My assumption is that an INLINE extent is <= block_size.  Otherwise
> the first part of the extent would be not-inline.  So this would be
> a bug in the filesystem; [4096-4608) should not be an inline extent.

Yes, never mind. Sorry about again.

> 
> > 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...)
> 
> Do you want to test erofs with multi-page folios?  That might be
> even more interesting than block size < page size.

Hmm.. I'm busy in developing for some new scenario. Will look into
that after the current busy period.

> 
> > > -	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)...
> 
> void kunmap_local_indexed(void *vaddr)
> {
>         unsigned long addr = (unsigned long) vaddr & PAGE_MASK;
> 
> so it's fine to unmap any address in the page.

I already checked this (in practice it has no problem due to the
current implementation), yet I'm not quite sure if it matches the
API usage, and not quite sure how many in-kernel users use like this.

Thanks,
Gao Xiang

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-07-24 15:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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).