Received-SPF: pass (google.com: domain of linux-ext4+bounces-87-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: Thu, 23 Nov 2023 01:55:27 +0530
Message-Id: <87pm01r0w8.fsf@doe.com>
From: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To: Jan Kara <jack@suse.cz>
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
In-Reply-To: <20231122122946.wg3jqvem6fkg3tgw@quack3>
Precedence: bulk

Jan Kara <jack@suse.cz> writes:

> 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.
>

yes, I will fix it.

>> +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).
>

Yes, looking into ->map_blocks was on my todo list, since this RFC only
maps 1 block at a time which can be expensive. I missed adding that as a
comment in cover letter.

But also thanks for providing many details on the same. I am taking a
look at it and will get back with more details on how can we get this
working, as it will be anyway required for ext4 too.

-ritesh