iomap: fix readahead folio access after folio_end_read()

If the folio does not have an iomap_folio_state struct attached to it and the folio gets read in by the filesystem's IO helper, folio_end_read() may have already been called on the folio.

Fix this by invalidating ctx->cur_folio when a folio without iomap_folio_state metadata attached to it has been handed to the filesystem's IO helper.

fs/iomap/buffered-io.c +6 -0

 			*bytes_submitted += plen;
+			/*
+			 * If the folio does not have ifs metadata attached,
+			 * then after ->read_folio_range(), the folio might have
+			 * gotten freed (eg iomap_finish_folio_read() ->
+			 * folio_end_read() followed by page cache eviction,
+			 * which for readahead folios drops the last refcount).

Quoted 3 lines · Joanne Koong

If the folio does not have an iomap_folio_state struct attached to it and the folio gets read in by the filesystem's IO helper, folio_end_read() may have already been called on the folio.

Not just can, but it has to, as there is no other way to track when folio_end_read would need to be called.

Quoted 3 lines

Fix this by invalidating ctx->cur_folio when a folio without iomap_folio_state metadata attached to it has been handed to the filesystem's IO helper.

Fix what?

for read_folio nothing ever looks at cur_folio, so I guess that is not what is being fixed, and it's readahead in some form. Can you explain what went wrong and how it is being fixed here a bit better?

Quoted 8 lines

                      *bytes_submitted += plen;

fs/iomap/buffered-io.c +6 -0

+                     /*
+                      * If the folio does not have ifs metadata attached,
+                      * then after ->read_folio_range(), the folio might have
+                      * gotten freed (eg iomap_finish_folio_read() ->
+                      * folio_end_read() followed by page cache eviction,
+                      * which for readahead folios drops the last refcount).

fs/iomap/buffered-io.c static int iomap_read_folio_iter(struct iomap_iter *iter, · line 564

Quoted 10 lines

fs/iomap/buffered-io.c +10 -0

+                      * Invalidate ctx->cur_folio here.
+                      *
+                      * For folios without ifs metadata attached, the read
+                      * should be on the entire folio.
+                      */
+                     if (!ifs) {
+                             ctx->cur_folio = NULL;
+                             if (unlikely(plen != folio_len))
+                                     return -EIO;
+                     }

I think the sanity check here is an impossible to hit condition and should probably be a WARN_ON_ONCE?

So to answer the above, I guess the issue that for readahead, the

        if (ctx->cur_folio &&
            offset_in_folio(ctx->cur_folio, iter->pos) == 0) {

in iomap_readahead_iter does a double completion?

I don't really love how this spreads the cur_folio setup logic from iomap_readahead_iter to here, but the other options I can think off to pass that logic to iomap_readahead_iter (overload return value with a positive return, extra output argument) don't feel all that enticing either because they'd now have to spread the ifs logic into iomap_readahead_iter. So I guess this version it is for now.

Maybe in the future I'll look into moving all the cur_folio logic from iomap_readahead_iter into iomap_read_folio_iter and see if that improves things, but that should not get into the way of the bug fix.

Thanks for taking a look at this. The problem I'm trying to fix is this readahead scenario:
* folio with no ifs gets read in by the filesystem through the ->read_folio_range() call
* the filesystem reads in the folio and calls iomap_finish_folio_read() which calls folio_end_read(), which unlocks

the folio
* then the page cache evicts the folio and drops the last refcount on the folio which frees the folio and the folio gets repurposed by another filesystem (eg btrfs) which uses folio->private
* the iomap logic accesses ctx->cur_folio still, and in the call to iomap_read_end(), it'll detect a non-null folio->private and it'll assume that's the ifs and it'll try to do stuff like spin_lock_irq(&ifs->state_lock) which will crash the system.

Quoted 6 lines

fs/iomap/buffered-io.c static int iomap_read_folio_iter(struct iomap_iter *iter, · line 564

Quoted 3 lines

fs/iomap/buffered-io.c +3 -0

+                             if (unlikely(plen != folio_len))
+                                     return -EIO;
+                     }

I think the sanity check here is an impossible to hit condition and should probably be a WARN_ON_ONCE?

I'll be removing this check for the next version.

Looking at this some more, I think we'll need to use ctx->cur_folio for non-readahead reads as well (eg passing ctx->cur_folio to iomap_read_end() in iomap_read_folio()). As Matthew pointed out to me in [1], we still can't access folio->private after folio_end_read() even if we hold an active refcount on the folio.

Thanks for taking a look at this. The problem I'm trying to fix is this readahead scenario:
* folio with no ifs gets read in by the filesystem through the ->read_folio_range() call
* the filesystem reads in the folio and calls iomap_finish_folio_read() which calls folio_end_read(), which unlocks the folio
* then the page cache evicts the folio and drops the last refcount on the folio which frees the folio and the folio gets repurposed by another filesystem (eg btrfs) which uses folio->private
* the iomap logic accesses ctx->cur_folio still, and in the call to iomap_read_end(), it'll detect a non-null folio->private and it'll assume that's the ifs and it'll try to do stuff like spin_lock_irq(&ifs->state_lock) which will crash the system.

This is not a problem for folios with an ifs because the +1 bias we add to ifs->read_bytes_pending makes it so that iomap is the one who invokes folio_end_read() when it's all done with the folio.

Quoted 15 lines · Joanne Koong

Thanks for taking a look at this. The problem I'm trying to fix is this readahead scenario:
* folio with no ifs gets read in by the filesystem through the ->read_folio_range() call
* the filesystem reads in the folio and calls iomap_finish_folio_read() which calls folio_end_read(), which unlocks the folio
* then the page cache evicts the folio and drops the last refcount on the folio which frees the folio and the folio gets repurposed by another filesystem (eg btrfs) which uses folio->private
* the iomap logic accesses ctx->cur_folio still, and in the call to iomap_read_end(), it'll detect a non-null folio->private and it'll assume that's the ifs and it'll try to do stuff like spin_lock_irq(&ifs->state_lock) which will crash the system.

Quoted 3 lines

This is not a problem for folios with an ifs because the +1 bias we add to ifs->read_bytes_pending makes it so that iomap is the one who invokes folio_end_read() when it's all done with the folio.

This is so complicated. I think you made your life harder by adding the bias to read_bytes_pending. What if we just rip most of this out ...

(the key here is to call iomap_finish_folio_read() instead of iomap_set_range_uptodate() when we zero parts of the folio)

I've thrown this at my test VM. See how it ends up doing.

fs/iomap/buffered-io.c +7 -86

index 154456e39fe5..c2ff4f561617 100644
@@ -381,7 +381,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
                ifs_alloc(iter->inode, folio, iter->flags);
 
        folio_fill_tail(folio, offset, iomap->inline_data, size);
-       iomap_set_range_uptodate(folio, offset, folio_size(folio) - offset);
+       iomap_finish_folio_read(folio, offset, folio_size(folio) - offset, 0);
        return 0;
 }
 

@@ -418,92 +418,19 @@ static void iomap_read_init(struct folio *folio)
        struct iomap_folio_state *ifs = folio->private;
 
        if (ifs) {
-               size_t len = folio_size(folio);
-
                /*
-                * ifs->read_bytes_pending is used to track how many bytes are
-                * read in asynchronously by the IO helper. We need to track
-                * this so that we can know when the IO helper has finished
-                * reading in all the necessary ranges of the folio and can end
-                * the read.
-                *
-                * Increase ->read_bytes_pending by the folio size to start, and
-                * add a +1 bias. We'll subtract the bias and any uptodate /
-                * zeroed ranges that did not require IO in iomap_read_end()
-                * after we're done processing the folio.
-                *
-                * We do this because otherwise, we would have to increment
-                * ifs->read_bytes_pending every time a range in the folio needs
-                * to be read in, which can get expensive since the spinlock
-                * needs to be held whenever modifying ifs->read_bytes_pending.
-                *
-                * We add the bias to ensure the read has not been ended on the
-                * folio when iomap_read_end() is called, even if the IO helper
-                * has already finished reading in the entire folio.
+                * Initially all bytes in the folio are pending.
+                * We subtract as either reads complete or we decide
+                * to memset().  Once the count reaches zero, the read
+                * is complete.
                 */
                spin_lock_irq(&ifs->state_lock);
                WARN_ON_ONCE(ifs->read_bytes_pending != 0);
-               ifs->read_bytes_pending = len + 1;
+               ifs->read_bytes_pending = folio_size(folio);
                spin_unlock_irq(&ifs->state_lock);
        }
 }
 
-/*
- * This ends IO if no bytes were submitted to an IO helper.
- *
- * Otherwise, this calibrates ifs->read_bytes_pending to represent only the
- * submitted bytes (see comment in iomap_read_init()). If all bytes submitted
- * have already been completed by the IO helper, then this will end the read.
- * Else the IO helper will end the read after all submitted ranges have been
- * read.
- */
-static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
-{
-       struct iomap_folio_state *ifs = folio->private;
-
-       if (ifs) {
-               bool end_read, uptodate;
-
-               spin_lock_irq(&ifs->state_lock);
-               if (!ifs->read_bytes_pending) {
-                       WARN_ON_ONCE(bytes_submitted);
-                       spin_unlock_irq(&ifs->state_lock);
-                       folio_unlock(folio);
-                       return;
-               }
-
-               /*
-                * Subtract any bytes that were initially accounted to
-                * read_bytes_pending but skipped for IO. The +1 accounts for
-                * the bias we added in iomap_read_init().
-                */
-               ifs->read_bytes_pending -=
-                       (folio_size(folio) + 1 - bytes_submitted);
-
-               /*
-                * If !ifs->read_bytes_pending, this means all pending reads by
-                * the IO helper have already completed, which means we need to
-                * end the folio read here. If ifs->read_bytes_pending != 0,
-                * the IO helper will end the folio read.
-                */
-               end_read = !ifs->read_bytes_pending;
-               if (end_read)
-                       uptodate = ifs_is_fully_uptodate(folio, ifs);
-               spin_unlock_irq(&ifs->state_lock);
-               if (end_read)
-                       folio_end_read(folio, uptodate);
-       } else if (!bytes_submitted) {
-               /*
-                * If there were no bytes submitted, this means we are
-                * responsible for unlocking the folio here, since no IO helper
-                * has taken ownership of it. If there were bytes submitted,
-                * then the IO helper will end the read via
-                * iomap_finish_folio_read().
-                */
-               folio_unlock(folio);
-       }
-}
-
 static int iomap_read_folio_iter(struct iomap_iter *iter,
                struct iomap_read_folio_ctx *ctx, size_t *bytes_submitted)
 {

@@ -544,7 +471,7 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
                /* zero post-eof blocks as the page may be mapped */
                if (iomap_block_needs_zeroing(iter, pos)) {
                        folio_zero_range(folio, poff, plen);
-                       iomap_set_range_uptodate(folio, poff, plen);
+                       iomap_finish_folio_read(folio, poff, plen, 0);
                } else {
                        if (!*bytes_submitted)
                                iomap_read_init(folio);

@@ -588,8 +515,6 @@ void iomap_read_folio(const struct iomap_ops *ops,
 
        if (ctx->ops->submit_read)
                ctx->ops->submit_read(ctx);
-
-       iomap_read_end(folio, bytes_submitted);
 }
 EXPORT_SYMBOL_GPL(iomap_read_folio);
 

@@ -601,7 +526,6 @@ static int iomap_readahead_iter(struct iomap_iter *iter,
        while (iomap_length(iter)) {
                if (ctx->cur_folio &&
                    offset_in_folio(ctx->cur_folio, iter->pos) == 0) {
-                       iomap_read_end(ctx->cur_folio, *cur_bytes_submitted);
                        ctx->cur_folio = NULL;
                }
                if (!ctx->cur_folio) {

@@ -653,9 +577,6 @@ void iomap_readahead(const struct iomap_ops *ops,
 
        if (ctx->ops->submit_read)
                ctx->ops->submit_read(ctx);
-
-       if (ctx->cur_folio)
-               iomap_read_end(ctx->cur_folio, cur_bytes_submitted);
 }
 EXPORT_SYMBOL_GPL(iomap_readahead);
 

Quoted 2 lines · Matthew Wilcox

This is so complicated. I think you made your life harder by adding the bias to read_bytes_pending. What if we just rip most of this out ...

I don't think we can rip this out because when the read starts, ifs->read_bytes_pending gets set to the folio size, but if there are already some uptodate ranges in the folio, the filesystem IO helper will not be reading in the entire folio size, which means we still need all this logic to decrement ifs->read_bytes_pending by the bytes not read in by the IO helper and to end the folio read.

Quoted 10 lines · Joanne Koong

Quoted 2 lines · Matthew Wilcox

This is so complicated. I think you made your life harder by adding the bias to read_bytes_pending. What if we just rip most of this out ...

I don't think we can rip this out because when the read starts, ifs->read_bytes_pending gets set to the folio size, but if there are already some uptodate ranges in the folio, the filesystem IO helper will not be reading in the entire folio size, which means we still need all this logic to decrement ifs->read_bytes_pending by the bytes not read in by the IO helper and to end the folio read.

Well, the patch as-is doesn't work (squinting at it now to see what I missed ...), but that's not an insurmountable problem. If we find already-uptodate blocks in the folio, we can just call iomap_finish_folio_read() for them without starting I/O. That's very much a corner case, so we need not try hard for efficiency (eg skipping the ifs_set_range_uptodate() call).