Re: [PATCH v2 06/11] ntfsplus: add iomap and address space operations

[Christoph Hellwig <hch@infradead.org>]

> +#include "ntfs_iomap.h"
> +
> +static s64 ntfs_convert_page_index_into_lcn(struct ntfs_volume *vol, struct ntfs_inode *ni,
> +		unsigned long page_index)
> +{
> +	sector_t iblock;
> +	s64 vcn;
> +	s64 lcn;
> +	unsigned char blocksize_bits = vol->sb->s_blocksize_bits;
> +
> +	iblock = (s64)page_index << (PAGE_SHIFT - blocksize_bits);
> +	vcn = (s64)iblock << blocksize_bits >> vol->cluster_size_bits;

I've seen this calculate in quite a few places, should there be a
generic helper for it?

> +struct bio *ntfs_setup_bio(struct ntfs_volume *vol, blk_opf_t opf, s64 lcn,
> +		unsigned int pg_ofs)
> +{
> +	struct bio *bio;
> +
> +	bio = bio_alloc(vol->sb->s_bdev, 1, opf, GFP_NOIO);
> +	if (!bio)
> +		return NULL;

bio_alloc never returns NULL if it can sleep.

> +	bio->bi_iter.bi_sector = ((lcn << vol->cluster_size_bits) + pg_ofs) >>
> +		vol->sb->s_blocksize_bits;

With a helper to calculate the sector the ntfs_setup_bio helper becomes
somewhat questionable.

> +static int ntfs_read_folio(struct file *file, struct folio *folio)
> +{
> +	loff_t i_size;
> +	struct inode *vi;
> +	struct ntfs_inode *ni;
> +
> +	vi = folio->mapping->host;
> +	i_size = i_size_read(vi);
> +	/* Is the page fully outside i_size? (truncate in progress) */
> +	if (unlikely(folio->index >= (i_size + PAGE_SIZE - 1) >>
> +			PAGE_SHIFT)) {
> +		folio_zero_segment(folio, 0, PAGE_SIZE);
> +		ntfs_debug("Read outside i_size - truncated?");
> +		folio_mark_uptodate(folio);
> +		folio_unlock(folio);
> +		return 0;
> +	}

iomap should be taking care of this, why do you need the extra
handling?

> +	/*
> +	 * This can potentially happen because we clear PageUptodate() during
> +	 * ntfs_writepage() of MstProtected() attributes.
> +	 */
> +	if (folio_test_uptodate(folio)) {
> +		folio_unlock(folio);
> +		return 0;
> +	}

Clearing the folio uptodate flag sounds fairly dangerous, why is that
done?

> +static int ntfs_write_mft_block(struct ntfs_inode *ni, struct folio *folio,
> +		struct writeback_control *wbc)

Just a very high-level comment here with no immediate action needed:
Is there a reall good reason to use the page cache for metadata?
Our experience with XFS is that a dedicated buffer cache is not only
much easier to use, but also allows for much better caching.

> +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. */
> +		return;
> +	}
> +
> +	if (NInoMstProtected(ni) &&
> +	    (ni->mft_no == FILE_MFT || ni->mft_no == FILE_MFTMirr))
> +		return;

Can you comment on why readahead is skipped here?

> +/**
> + * ntfs_compressed_aops - address space operations for compressed inodes
> + */
> +const struct address_space_operations ntfs_compressed_aops = {

From code in other patches is looks like ntfs never switches between
compressed and non-compressed for live inodes?  In that case the
separate aops should be fine, as switching between them at runtime
would involve races.  Is the compression policy per-directory?

> +		kaddr = kmap_local_folio(folio, 0);
> +		offset = (loff_t)idx << PAGE_SHIFT;
> +		to = min_t(u32, end - offset, PAGE_SIZE);
> +
> +		memcpy(buf + buf_off, kaddr + from, to);
> +		buf_off += to;
> +		kunmap_local(kaddr);
> +		folio_put(folio);
> +	}

Would this be a candidate for memcpy_from_folio?

> +		kaddr = kmap_local_folio(folio, 0);
> +		offset = (loff_t)idx << PAGE_SHIFT;
> +		to = min_t(u32, end - offset, PAGE_SIZE);
> +
> +		memcpy(kaddr + from, buf + buf_off, to);
> +		buf_off += to;
> +		kunmap_local(kaddr);
> +		folio_mark_uptodate(folio);
> +		folio_mark_dirty(folio);

And memcpy_to_folio?

> +++ b/fs/ntfsplus/ntfs_iomap.c

Any reason for the ntfs_ prefix here?

> +static void ntfs_iomap_put_folio(struct inode *inode, loff_t pos,
> +		unsigned int len, struct folio *folio)
> +{

This seems to basically be entirely about extra zeroing.  Can you
explain why this is needed in a comment?

> +static int ntfs_read_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> +		unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
> +{
> +	struct ntfs_inode *base_ni, *ni = NTFS_I(inode);
> +	struct ntfs_attr_search_ctx *ctx;
> +	loff_t i_size;
> +	u32 attr_len;
> +	int err = 0;
> +	char *kattr;
> +	struct page *ipage;
> +
> +	if (NInoNonResident(ni)) {

Can you split the resident and non-resident cases into separate
helpers to keep this easier to follow?
easier to follow?

> +	ipage = alloc_page(__GFP_NOWARN | __GFP_IO | __GFP_ZERO);
> +	if (!ipage) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	memcpy(page_address(ipage), kattr, attr_len);

Is there a reason for this being a page allocation vs a kmalloc
sized to the inline data?

> +static int ntfs_buffered_zeroed_clusters(struct inode *vi, s64 vcn)

I think this should be ntfs_buffered_zero_clusters as it
performans the action?

Also curious why this can't use the existing iomap zeroing helper?

> +int ntfs_zeroed_clusters(struct inode *vi, s64 lcn, s64 num)

ntfs_zero_clusters

Again curious why we need special zeroing code in the file system.

> +	if (NInoNonResident(ni)) {

Another case for splitting the resident/non-resident code instead
of having a giant conditional block that just returns.