Re: [PATCH 1/2] iomap: support tail packing inline read

On Sat, Jul 17, 2021 at 11:15:58PM +0800, Gao Xiang wrote:

Hi Matthew,

On Sat, Jul 17, 2021 at 04:01:38PM +0100, Matthew Wilcox wrote:

quoted

On Sat, Jul 17, 2021 at 09:38:18PM +0800, Gao Xiang wrote:

quoted

Sorry about some late. I've revised a version based on Christoph's
version and Matthew's thought above. I've preliminary checked with
EROFS, if it does make sense, please kindly help check on the gfs2
side as well..

I don't understand how this bit works:

This part inherited from the Christoph version without change.
The following thoughts are just my own understanding...

quoted

quoted

 	struct page *page = ctx->cur_page;
-	struct iomap_page *iop;
+	struct iomap_page *iop = NULL;
 	bool same_page = false, is_contig = false;
 	loff_t orig_pos = pos;
 	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 && !pos)
+		WARN_ON_ONCE(to_iomap_page(page) != NULL);
+	else
+		iop = iomap_page_create(inode, page);

Imagine you have a file with bytes 0-2047 in an extent which is !INLINE
and bytes 2048-2051 in the INLINE extent.  When you read the page, first
you create an iop for the !INLINE extent.  Then this function is called

Yes, it first created an iop for the !INLINE extent.

quoted

again for the INLINE extent and you'll hit the WARN_ON_ONCE.  No?

If it is called again with another INLINE extent, pos will be non-0?
so (!pos) == false. Am I missing something?

Well, either sense of a WARN_ON is wrong.

For a file which is PAGE_SIZE + 3 bytes in size, to_iomap_page() will
be NULL.  For a file which is PAGE_SIZE/2 + 3 bytes in size,
to_iomap_page() will not be NULL.  (assuming the block size is <=
PAGE_SIZE / 2).

I think we need a prep patch that looks something like this:

+++ b/fs/iomap/buffered-io.c

@@ -252,8 +252,12 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
                return PAGE_SIZE;
        }

+       if (offset_in_page(pos) || length < PAGE_SIZE)
+               iop = iomap_page_create(inode, page);
+       else
+               iop = NULL;
+
        /* zero post-eof blocks as the page may be mapped */
-       iop = iomap_page_create(inode, page);
        iomap_adjust_read_range(inode, iop, &pos, length, &poff, &plen);
        if (plen == 0)
                goto done;

ie first get the conditions right under which we should create an iop.