Re: [PATCH 1/3] mpage: terminate read-ahead on read error

[Chi Zhiling]

On 2025/8/18 10:41, Andrew Morton wrote:
> On Tue, 12 Aug 2025 15:22:23 +0800 Chi Zhiling <chizhiling@xxxxxxx> wrote:
>
> > From: Chi Zhiling <chizhiling@xxxxxxxxxx>
> >
> > For exFAT filesystems with 4MB read_ahead_size, removing the storage device
> > during read operations can delay EIO error reporting by several minutes.
> > This occurs because the read-ahead implementation in mpage doesn't handle
> > errors.
> >
> > Another reason for the delay is that the filesystem requires metadata to
> > issue file read request. When the storage device is removed, the metadata
> > buffers are invalidated, causing mpage to repeatedly attempt to fetch
> > metadata during each get_block call.
> >
> > The original purpose of this patch is terminate read ahead when we fail
> > to get metadata, to make the patch more generic, implement it by checking
> > folio status, instead of checking the return of get_block().
> >
> > ...
> >
> > --- a/fs/mpage.c
> > +++ b/fs/mpage.c
> > @@ -369,6 +369,9 @@ void mpage_readahead(struct readahead_control *rac, get_block_t get_block)
> >   		args.folio = folio;
> >   		args.nr_pages = readahead_count(rac);
> >   		args.bio = do_mpage_readpage(&args);
> > +		if (!folio_test_locked(folio) &&
> > +		    !folio_test_uptodate(folio))
> > +			break;
> >   	}
> >   	if (args.bio)
> >   		mpage_bio_submit_read(args.bio);
>
> So...  this is what the fs does when the device is unplugged?
> Synchronously return an unlocked !uptodate folio?  Or is this specific
> to FAT?

It's fs behavior,

AFAIK, all filesystems that use mpage will lock the folio until I/O 
finishes or encounters an error. This avoids races like buffered writes, 
etc. The uptodate flag being set or not depends on the I/O status.


So, if a folio is synchronously unlocked and non-uptodate, should we 
quit the read ahead?

I think it depends on whether the error is permanent or temporary, and 
whether further read ahead might succeed.

A device being unplugged is one reason for returning such a folio, but 
we could return it for many other reasons (e.g., metadata errors).

I think most errors won't be restored in a short time, so we should quit 
read ahead when they occur.


Besides, IOMAP also quits read ahead when some errors are encountered in 
iomap_begin().

> I think a comment here telling readers why we're doing this would be
> helpful.  It isn't obvious that we're dealing with e removed device!

okay, I will comment here.
/*
 * If read ahead failed synchronously, it may cause by removed device,
 * or some filesystem metadata error.
 */

> Also, boy this is old code.  Basically akpm code from pre-git times.
> It was quite innovative back then, but everybody who understood it has
> since moved on,  got senile or probably died.  Oh well.

Actually, I think this patch is safe, but I'm not sure if we should fix 
this issue. After all, this code has existed for a long time, and it's 
quite rare to unplug the device during a copy operation :)


Thanks,
Chi Zhiling