On 19 Oct 2020, at 21:43, Matthew Wilcox wrote:

This is a weird one ... which is good because it means the obvious
ones have been fixed and now I'm just tripping over the weird cases.
And fortunately, xfstests exercises the weird cases.

1. The file is 0x3d000 bytes long.
2. A readahead allocates an order-2 THP for 0x3c000-0x3ffff
3. We simulate a read error for 0x3c000-0x3cfff
4. Userspace writes to 0x3d697 to 0x3dfaa
5. iomap_write_begin() gets the 0x3c page, sees it's THP and !Uptodate
   so it calls iomap_split_page() (passing page 0x3d)
6. iomap_split_page() calls split_huge_page()
7. split_huge_page() sees that page 0x3d is beyond EOF, so it removes 
it
   from i_pages
8. iomap_write_actor() copies the data into page 0x3d

I’m guessing that iomap_write_begin() is still in charge of locking 
the pages, and that iomap_split_page()->split_huge_page() is just 
reusing that lock?

It sounds like you’re missing a flag to iomap_split_page() that says: 
I care about range A->B, even if its beyond EOF.  IOW, 
iomap_write_begin()’s path should be in charge of doing the right 
thing for the write, without relying on the rest of the kernel to avoid 
upsetting it.

9. The write is lost.

Trying to persuade XFS to update i_size before calling
iomap_file_buffered_write() seems like a bad idea.

Changing split_huge_page() to disregard i_size() is something I kind
of want to be able to do long-term in order to make hole-punch more
efficient, but that seems like a lot of work right now.

The problem with trusting i_size is that it changes at surprising times. 
  For this code inside split_huge_page(), end == i_size_read()

         for (i = HPAGE_PMD_NR - 1; i >= 1; i--) {
                 __split_huge_page_tail(head, i, lruvec, list);
                 /* Some pages can be beyond i_size: drop them from page 
cache */
                 if (head[i].index >= end) {
                         ClearPageDirty(head + i);

But, we actually change i_size after dropping all the page locks.  In 
xfs this is xfs_setattr_size()->truncate_setsize(), all of which means 
that dropping PageDirty seems unwise if this code is running 
concurrently with an expanding truncate.  If i_size jumps past the page 
where you’re clearing dirty, it probably won’t be good.  Ignore me 
if this is already handled differently, it just seems error prone in 
current Linus.

-chris