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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9b3116ba-0f68-44bb-9ec9-36871fe6096e@163.com>
Date: Mon, 18 Aug 2025 18:04:23 +0800
From: Chi Zhiling <chizhiling@....com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Matthew Wilcox <willy@...radead.org>, Namjae Jeon <linkinjeon@...nel.org>,
 Sungjong Seo <sj1557.seo@...sung.com>, Yuezhang Mo <yuezhang.mo@...y.com>,
 Chi Zhiling <chizhiling@...inos.cn>
Subject: Re: [PATCH 1/3] mpage: terminate read-ahead on read error

On 2025/8/18 10:41, Andrew Morton wrote:
> On Tue, 12 Aug 2025 15:22:23 +0800 Chi Zhiling <chizhiling@....com> wrote:
> 
>> From: Chi Zhiling <chizhiling@...inos.cn>
>>
>> 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