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

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

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?

>  	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?)

>  	}
>  
> +	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?

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

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>