Received-SPF: pass (google.com: domain of linux-ext4+bounces-346-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Subject: Re: [PATCH 13/14] iomap: map multiple blocks at a time
To: Christoph Hellwig <hch@lst.de>
Cc: "Darrick J. Wong" <djwong@kernel.org>,
 Chandan Babu R <chandan.babu@oracle.com>,
 Ritesh Harjani <ritesh.list@gmail.com>, Jens Axboe <axboe@kernel.dk>,
 Andreas Gruenbacher <agruenba@redhat.com>,
 Damien Le Moal <dlemoal@kernel.org>, Naohiro Aota <naohiro.aota@wdc.com>,
 Johannes Thumshirn <jth@kernel.org>, linux-xfs@vger.kernel.org,
 linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
 gfs2@lists.linux.dev, Christian Brauner <brauner@kernel.org>,
 linux-ext4@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>
References: <20231207072710.176093-1-hch@lst.de>
 <20231207072710.176093-14-hch@lst.de>
 <4e4a86a0-5681-210f-0c94-263126967082@huaweicloud.com>
 <20231207150311.GA18830@lst.de>
From: Zhang Yi <yi.zhang@huaweicloud.com>
Message-ID: <d5f3979b-344a-bf01-8a45-49ae02b0bed0@huaweicloud.com>
Date: Fri, 8 Dec 2023 15:33:57 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20231207150311.GA18830@lst.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 2023/12/7 23:03, Christoph Hellwig wrote:
> On Thu, Dec 07, 2023 at 09:39:44PM +0800, Zhang Yi wrote:
>>> +	do {
>>> +		unsigned map_len;
>>> +
>>> +		error = wpc->ops->map_blocks(wpc, inode, pos);
>>> +		if (error)
>>> +			break;
>>> +		trace_iomap_writepage_map(inode, &wpc->iomap);
>>> +
>>> +		map_len = min_t(u64, dirty_len,
>>> +			wpc->iomap.offset + wpc->iomap.length - pos);
>>> +		WARN_ON_ONCE(!folio->private && map_len < dirty_len);
>>
>> While I was debugging this series on ext4, I would suggest try to add map_len
>> or dirty_len into this trace point could be more convenient.
> 
> That does seem useful, but it means we need to have an entirely new
> even class.  Can I offload this to you for inclusion in your ext4
> series? :)
> 

Sure, I'm glad to do it.

>>> +		case IOMAP_HOLE:
>>> +			break;
>>
>> BTW, I want to ask an unrelated question of this patch series. Do you
>> agree with me to add a IOMAP_DELAYED case and re-dirty folio here? The
>> background is that on ext4, jbd2 thread call ext4_normal_submit_inode_data_buffers()
>> submit data blocks in data=ordered mode, but it can only submit mapped
>> blocks, now we skip unmapped blocks and re-dirty folios in
>> ext4_do_writepages()->mpage_prepare_extent_to_map()->..->ext4_bio_write_folio().
>> So we have to inherit this logic when convert to iomap, I suppose ext4's
>> ->map_blocks() return IOMAP_DELALLOC for this case, and iomap do something
>> like:
>>
>> +               case IOMAP_DELALLOC:
>> +                       iomap_set_range_dirty(folio, offset_in_folio(folio, pos),
>> +                                             map_len);
>> +                       folio_redirty_for_writepage(wbc, folio);
>> +                       break;
> 
> I guess we could add it, but it feels pretty quirky to me, so it would at
> least need a very big comment.
> 
> But I think Ted mentioned a while ago that dropping the classic
> data=ordered mode for ext4 might be a good idea eventually no that ext4
> can update the inode size at I/O completion time (Ted, correct me if
> I'm wrong).  If that's the case it might make sense to just drop the
> ordered mode instead of adding these quirks to iomap.
> 

Yeah, make sense, we could remove these quirks after ext4 drop
data=ordered mode. For now, let me implement it according to this
temporary method.

Thanks,
Yi.