Re: [RFC PATCH 1/8] fat: support FIEMAP

[Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>]

On Sun, Nov 08, 2026 at 11:51:53PM +0900, OGAWA Hirofumi wrote:
>Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br> writes:
>
>> Implement support for ->fiemap() using iomap_fiemap(). Add a read-only
>> fat_iomap_begin() and a fat_get_cont_clusters() helper for finding
>> contiguous clusters.
>
>Not fully reviewed and I'm not understanding iomap fully though.
>
>> Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
>> ---
>>  fs/fat/Kconfig  |  1 +
>>  fs/fat/Makefile |  2 +-
>>  fs/fat/cache.c  | 80 ++++++++++++++++++++++++++++++++++++++-----------
>>  fs/fat/fat.h    |  5 ++++
>>  fs/fat/file.c   | 12 ++++++++
>>  fs/fat/iomap.c  | 71 +++++++++++++++++++++++++++++++++++++++++++
>>  6 files changed, 152 insertions(+), 19 deletions(-)
>>  create mode 100644 fs/fat/iomap.c
>>
>
>[...]
>
>> diff --git a/fs/fat/cache.c b/fs/fat/cache.c
>> index 1b87354e24ba..f545e1db1d47 100644
>> --- a/fs/fat/cache.c
>> +++ b/fs/fat/cache.c
>> @@ -76,7 +76,7 @@ static int fat_cache_lookup(struct inode *inode, int fclus,
>>  			    struct fat_cache_id *cid,
>>  			    int *cached_fclus, int *cached_dclus)
>>  {
>> -	static struct fat_cache nohit = { .fcluster = 0, };
>> +	static struct fat_cache nohit = { .fcluster = -1, };
>
>Maybe this change is pointless?
>

It is needed for the cache_init() change, as otherwise it would not find
an entry at cluster 0. See below for the rationale as to why I was trying
to do it.

>>  	struct fat_cache *hit = &nohit, *p;
>>  	int offset = -1;
>> @@ -217,14 +217,16 @@ static inline void cache_init(struct fat_cache_id *cid, int fclus, int dclus)
>>  	cid->nr_contig = 0;
>>  }
>>  
>> -int fat_get_cluster(struct inode *inode, int cluster, int *fclus, int *dclus)
>> +static int __fat_get_cluster(struct inode *inode, int cluster, int *fclus,
>> +			     int *dclus, int *nr_cont)
>>  {
>>  	struct super_block *sb = inode->i_sb;
>>  	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>>  	const int limit = sb->s_maxbytes >> sbi->cluster_bits;
>>  	struct fat_entry fatent;
>>  	struct fat_cache_id cid;
>
>[...]
>
>>  
>>  	if (fat_cache_lookup(inode, cluster, &cid, fclus, dclus) < 0) {
>> -		/*
>> -		 * dummy, always not contiguous
>> -		 * This is reinitialized by cache_init(), later.
>> -		 */
>> -		cache_init(&cid, -1, -1);
>> +		/* Cache the contiguous prefix from the known first cluster. */
>> +		cache_init(&cid, 0, *dclus);
>
>Doesn't this add a needless cache? 0 is cached in the inode itself,
>isn't it? (and can be waste small fatent cache slots?)
>

This is mostly for caching the number of contiguous clusters starting
from 0. i_start only really tells us the first cluster of the file. I'm
not exactly sure of the best way to approach this, though. I guess we
can skip caching when its a one cluster range in this case?

What do you think?

>>  	}
>>  
>> +	fcluster = *fclus;
>> +	dcluster = *dclus;
>>  	fatent_init(&fatent);
>> -	while (*fclus < cluster) {
>> +	while (fcluster < last) {
>> +		if (nr_cont && fcluster == cluster && cid.fcluster != -1) {
>
>cid.fcluster == -1 is possible with this patch?
>

Good catch.

>> +			fcluster = min(last, cid.fcluster + cid.nr_contig);
>> +			dcluster = cid.dcluster + fcluster - cid.fcluster;
>> +			if (fcluster == last)
>> +				break;
>> +		}
>> +

Thanks,
Matheus