Received-SPF: pass (google.com: domain of linux-ext4+bounces-80-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 22 Nov 2023 13:29:46 +0100
From: Jan Kara <jack@suse.cz>
To: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc: linux-ext4@vger.kernel.org, Jan Kara <jack@suse.cz>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [RFC 2/3] ext2: Convert ext2 regular file buffered I/O to use
 iomap
Message-ID: <20231122122946.wg3jqvem6fkg3tgw@quack3>
References: <cover.1700506526.git.ritesh.list@gmail.com>
 <f5e84d3a63de30def2f3800f534d14389f6ba137.1700506526.git.ritesh.list@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f5e84d3a63de30def2f3800f534d14389f6ba137.1700506526.git.ritesh.list@gmail.com>

On Tue 21-11-23 00:35:20, Ritesh Harjani (IBM) wrote:
> This patch converts ext2 regular file's buffered-io path to use iomap.
> - buffered-io path using iomap_file_buffered_write
> - DIO fallback to buffered-io now uses iomap_file_buffered_write
> - writeback path now uses a new aops - ext2_file_aops
> - truncate now uses iomap_truncate_page
> - mmap path of ext2 continues to use generic_file_vm_ops
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Looks nice and simple. Just one comment below:

> +static void ext2_file_readahead(struct readahead_control *rac)
> +{
> +	return iomap_readahead(rac, &ext2_iomap_ops);
> +}

As Matthew noted, the return of void here looks strange.

> +static int ext2_write_map_blocks(struct iomap_writepage_ctx *wpc,
> +				 struct inode *inode, loff_t offset)
> +{
> +	if (offset >= wpc->iomap.offset &&
> +	    offset < wpc->iomap.offset + wpc->iomap.length)
> +		return 0;
> +
> +	return ext2_iomap_begin(inode, offset, inode->i_sb->s_blocksize,
> +				IOMAP_WRITE, &wpc->iomap, NULL);
> +}

So this will end up mapping blocks for writeback one block at a time if I'm
understanding things right because ext2_iomap_begin() never sets extent
larger than 'length' in the iomap result. Hence the wpc checking looks
pointless (and actually dangerous - see below). So this will probably get
more expensive than necessary by calling into ext2_get_blocks() for each
block? Perhaps we could first call ext2_iomap_begin() for reading with high
length to get as many mapped blocks as we can and if we find unmapped block
(should happen only for writes through mmap), we resort to what you have
here... Hum, but this will not work because of the races with truncate
which could remove blocks whose mapping we have cached in wpc. We can
safely provide a mapping under a folio only once it is locked or has
writeback bit set. XFS plays the revalidation sequence counter games
because of this so we'd have to do something similar for ext2. Not that I'd
care as much about ext2 writeback performance but it should not be that
hard and we'll definitely need some similar solution for ext4 anyway. Can
you give that a try (as a followup "performance improvement" patch).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR