Re: [PATCH 1/5] exfat: add iomap support

[Christoph Hellwig <hch@lst.de>]

>  
> -static int exfat_extend_valid_size(struct inode *inode, loff_t new_valid_size)
> +int exfat_extend_valid_size(struct inode *inode, loff_t off, bool bsync)

This looks like at least partially unrelated refactoring.  Can you
split this out into a separate well-documented patch, or maybe even
two where one has the bulk reformatting changes, and one adds the
new bsync option?  Also it might help to document the bsync option.
Often it might be more readable to add a flags parameter with a named
flag to make things easier to follow.

> -static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
> -		unsigned int *clu, unsigned int *count, int create)
> +int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
> +		unsigned int *clu, unsigned int *count, int create,
> +		bool *balloc)

Similarly, this would be a good prep patch.

> diff --git a/fs/exfat/iomap.c b/fs/exfat/iomap.c
> new file mode 100644
> index 000000000000..e4135a13454f
> --- /dev/null
> +++ b/fs/exfat/iomap.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * iomap callack functions
> + *
> + * Copyright (C) 2026 Namjae Jeon <linkinjeon@kernel.org>
> + */

Also normally new infrastructure would get added with the users.
I.e. the bits you're using in the first iomap conversion would go
with that and so on.  But that's not a strict rule.

> +/*
> + * exfat_iomap_put_folio - Put folio after iomap operation
> + *
> + * Called when iomap is finished with a folio zero-fills portions of
> + * the folio beyond ->valid_size to prevent exposing uninitialized data.
> + */
> +static void exfat_iomap_put_folio(struct inode *inode, loff_t pos,
> +		unsigned int len, struct folio *folio)

Can you explain the logic here?  Shouldn't the iomap buffered I/O
code do all the needed zeroing for you based on the map type?  If not
how could we enhance the core iomap code so that we don't need this
in the file system, which feels like a bit of break of abstraction
barriers?

> +/*
> + * exfat_file_write_dio_end_io - Direct I/O write completion handler
> + *
> + * Updates i_size if the write extended the file. Called from the dio layer
> + * after I/O completion.
> + */
> +static int exfat_file_write_dio_end_io(struct kiocb *iocb, ssize_t size,
> +		int error, unsigned int flags)
> +{
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +
> +	if (error)
> +		return error;
> +
> +	if (size && i_size_read(inode) < iocb->ki_pos + size) {
> +		i_size_write(inode, iocb->ki_pos + size);
> +		mark_inode_dirty(inode);
> +	}
> +
> +	return 0;
> +}

I think in the long run we should just do this as the default in
the core iomap dio code when no end_io routine is provided for
writes.  But I can refactor this later to not hold you up.

> +static int exfat_read_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> +		unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
> +{

The read and write routines look very similar, and chance we could
extra most of the logic into a common helper?  Also the rounding of
the length using the start in the write version looks like it would
be the right thing for the read side anyway.

> +static int exfat_mkwrite_iomap_begin(struct inode *inode, loff_t offset,
> +		loff_t length, unsigned int flags, struct iomap *iomap,
> +		struct iomap *srcmap)
> +{
> +	return __exfat_write_iomap_begin(inode, offset, length, iomap);
> +}

The special mkwrite handling looks a bit odd to me.  I'll return to
that later in the series, but this is a good example where keeping
all the related code together would help the reviewer.

> +	if (offset < wpc->iomap.offset ||
> +	    offset >= wpc->iomap.offset + wpc->iomap.length) {
> +		int error;
> +
> +		error = exfat_read_iomap_begin(wpc->inode, offset, len,
> +				0, &wpc->iomap, NULL);

The read confused me a bit here, but I guess by the time we do writeback
everything is allocated in exfat.  This would be another good candidate
to directly call the low-level helper suggested above.

> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 83396fd265cd..b69c4b0a926b 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -499,6 +499,7 @@ static int exfat_read_boot_sector(struct super_block *sb)
>  	if (p_boot->num_fats == 2)
>  		sbi->FAT2_start_sector += sbi->num_FAT_sectors;
>  	sbi->data_start_sector = le32_to_cpu(p_boot->clu_offset);
> +	sbi->data_start_bytes = sbi->data_start_sector << p_boot->sect_size_bits;

Is this related to iomap?