Re: [PATCH v6 09/16] ntfs: update iomap and address space operations

[Christoph Hellwig <hch@lst.de>]

Suggested commit message:

Update the address space operations to use the iomap framework,
replacing legacy buffer-head based code.

> +#include <linux/mpage.h>

This include should not be needed (same in iomap.c).

> +#include <linux/uio.h>

This should not be needed either (same in iomap.c).

> -};
> +static void ntfs_readahead(struct readahead_control *rac)
> +{
> +	struct address_space *mapping = rac->mapping;
> +	struct inode *inode = mapping->host;
> +	struct ntfs_inode *ni = NTFS_I(inode);
>  
> +	if (!NInoNonResident(ni) || NInoCompressed(ni)) {
> +		/* No readahead for resident and compressed. */

As-is this comment is useless ads it states the obvious.  If you
want to make it useful add why it is not implemented.

> +static int ntfs_writepages(struct address_space *mapping,
> +		struct writeback_control *wbc)
> +{
> +	struct inode *inode = mapping->host;
> +	struct ntfs_inode *ni = NTFS_I(inode);
> +	struct iomap_writepage_ctx wpc = {
> +		.inode		= mapping->host,
> +		.wbc		= wbc,
> +		.ops		= &ntfs_writeback_ops,
> +	};
> +
> +	if (NVolShutdown(ni->vol))
> +		return -EIO;
>  
> +	if (!NInoNonResident(ni))
> +		return 0;
>  
> +	/* If file is encrypted, deny access, just like NT4. */

I don't understand this comment.

> +void mark_ntfs_record_dirty(struct folio *folio)
> +{
> +	iomap_dirty_folio(folio->mapping, folio);
> +}

Should this be in mft.c and have a mft_ compoenent in the name?

> +	if (!NInoNonResident(ni))
> +		goto out;

Split the resident handling into a helper to keep the method
simple?

> +static int __ntfs_read_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> +		unsigned int flags, struct iomap *iomap, struct iomap *srcmap,
> +		bool need_unwritten)
> +{
> +	struct ntfs_inode *ni = NTFS_I(inode);
> +	int ret;
> +
> +	if (NInoNonResident(ni))
> +		ret = ntfs_read_iomap_begin_non_resident(inode, offset, length,
> +				flags, iomap, need_unwritten);
> +	else
> +		ret = ntfs_read_iomap_begin_resident(inode, offset, length,
> +				flags, iomap);
> +
> +	return ret;

This could be simplified to:

	if (NInoNonResident(NTFS_I(inode)))
		return ntfs_read_iomap_begin_non_resident(inode, offset, length,
				flags, iomap, need_unwritten);
	return ntfs_read_iomap_begin_resident(inode, offset, length, flags,
			iomap);

> +int ntfs_zero_range(struct inode *inode, loff_t offset, loff_t length, bool bdirect)
> +{
> +	if (bdirect) {
> +		if ((offset | length) & (SECTOR_SIZE - 1))
> +			return -EINVAL;
> +
> +		return  blkdev_issue_zeroout(inode->i_sb->s_bdev,
> +					     offset >> SECTOR_SHIFT,
> +					     length >> SECTOR_SHIFT,
> +					     GFP_NOFS,
> +					     BLKDEV_ZERO_NOUNMAP);
> +	}
> +
> +	return iomap_zero_range(inode,
> +				offset, length,
> +				NULL,
> +				&ntfs_zero_read_iomap_ops,
> +				&ntfs_zero_iomap_folio_ops,
> +				NULL);
> +}

This really should be two separate helpers.

> +	if (NInoNonResident(ni)) {

As separate non-resident helper would be useful here.

> +		if (ntfs_iomap_flags & NTFS_IOMAP_FLAGS_BEGIN)
> +			ret = ntfs_write_iomap_begin_non_resident(inode, offset,
> +					length, iomap);
> +		else
> +			ret = ntfs_write_da_iomap_begin_non_resident(inode,
> +					offset, length, flags, iomap,
> +					ntfs_iomap_flags);
> +	} else {
> +		mutex_lock(&ni->mrec_lock);
> +		ret = ntfs_write_iomap_begin_resident(inode, offset, iomap);
> +	}
> +
> +	return ret;

But eveven without that, direct returns would really help here:

	if (!NInoNonResident(ni)) {
		mutex_lock(&ni->mrec_lock);
		return ntfs_write_iomap_begin_resident(inode, offset, iomap);
	}

	...

	if (ntfs_iomap_flags & NTFS_IOMAP_FLAGS_BEGIN)
		return ntfs_write_iomap_begin_non_resident(inode, offset,
				length, iomap);
	return ntfs_write_da_iomap_begin_non_resident(inode, offset, length,
			flags, iomap,

> +static int ntfs_write_iomap_end(struct inode *inode, loff_t pos, loff_t length,
> +		ssize_t written, unsigned int flags, struct iomap *iomap)
> +{
> +	if (iomap->type == IOMAP_INLINE) {

A separate inline helper would help here as well.