Re: [PATCH 3/5] exfat: add iomap buffered I/O support

[Christoph Hellwig <hch@lst.de>]

> -#define EXFAT_CLU_TO_B(b, sbi)		((b) << (sbi)->cluster_size_bits)
> +#define EXFAT_CLU_TO_B(b, sbi)		((loff_t)(b) << (sbi)->cluster_size_bits)

This should be a prep patch.

>  	if ((attr->ia_valid & ATTR_SIZE) &&
>  	    attr->ia_size > i_size_read(inode)) {
> +		loff_t old_size = i_size_read(inode);
> +
>  		error = exfat_cont_expand(inode, attr->ia_size);
> +		if (!error && attr->ia_size > old_size &&
> +		    old_size % PAGE_SIZE != 0) {
> +			loff_t len = min_t(loff_t,
> +					round_up(old_size, PAGE_SIZE) - old_size,
> +					attr->ia_size - old_size);
> +			error = iomap_zero_range(inode, old_size, len,
> +					NULL, &exfat_read_iomap_ops,
> +					&exfat_iomap_folio_ops, NULL);
> +		}

Why is this needed for iomap?  Shouldn't explicit zeroing on size change
be a prep patch?

>  	if (unlikely(exfat_forced_shutdown(inode->i_sb)))
>  		return -EIO;
>  
> -	err = __generic_file_fsync(filp, start, end, datasync);
> +	err = file_write_and_wait_range(filp, start, end);
>  	if (err)
>  		return err;
>  
> +	if (!datasync)
> +		err = __exfat_write_inode(inode, 1);
> +	write_inode_now(inode, !datasync);

This seems to have lost the i_state check to see if a sync
is needed before __exfat_write_inode.  Also doing write_inode_now
plus the explicit inode sync seems duplicate.  Last but not least
I don't understand how this is related to iomap?

> +	ssize_t ret;
> +
> +	ret = iomap_dio_rw(iocb, from, &exfat_write_iomap_ops,
> +			&exfat_write_dio_ops, 0, NULL, 0);
> +	if (ret == -ENOTBLK)
> +		ret = 0;
> +	else if (ret < 0)
> +		goto out;
> +
> +	if (iov_iter_count(from)) {
> +		loff_t offset, end;
> +		ssize_t written;
> +		int ret2;
> +
> +		offset = iocb->ki_pos;
> +		iocb->ki_flags &= ~IOCB_DIRECT;
> +		written = iomap_file_buffered_write(iocb, from,
> +				&exfat_write_iomap_ops, &exfat_iomap_folio_ops,
> +				NULL);
> +		if (written < 0) {
> +			ret = written;
> +			goto out;
> +		}
> +
> +		ret += written;
> +		end = iocb->ki_pos + written - 1;
> +		ret2 = filemap_write_and_wait_range(iocb->ki_filp->f_mapping,
> +				offset, end);
> +		if (ret2) {
> +			ret = -EIO;
> +			goto out;
> +		}
> +		if (!ret2)
> +			invalidate_mapping_pages(iocb->ki_filp->f_mapping,
> +					offset >> PAGE_SHIFT,
> +					end >> PAGE_SHIFT);
> +	}

Eventually we should factor this code int oa common helper.  Not needed
for this submission, though.

>  {
> -	int err;
>  	struct inode *inode = file_inode(vmf->vma->vm_file);
> -	struct exfat_inode_info *ei = EXFAT_I(inode);
> -	loff_t new_valid_size;
> +	vm_fault_t ret;
>  
>  	if (!inode_trylock(inode))
>  		return VM_FAULT_RETRY;
>  
> -	new_valid_size = ((loff_t)vmf->pgoff + 1) << PAGE_SHIFT;
> -	new_valid_size = min(new_valid_size, i_size_read(inode));
> -
> -	if (ei->valid_size < new_valid_size) {
> -		err = exfat_extend_valid_size(inode, new_valid_size, false);

Why is this moving out?  I think the zeroing changes in this patch
are really something that should be split out and explained in the
commit log for better understanding and bisectability.

> +static int exfat_file_open(struct inode *inode, struct file *filp)
> +{
> +	int err;
> +
> +	if (unlikely(exfat_forced_shutdown(inode->i_sb)))
> +		return -EIO;

This doesn't look iomap-related.

> +
> +	err = generic_file_open(inode, filp);
> +	if (err)
> +		return err;
> +
> +	filp->f_mode |= FMODE_CAN_ODIRECT;

This should go into the previous patch.