linux-ext4 - Re: [PATCH 13/14] iomap: map multiple blocks at a time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 7 Dec 2023 16:03:11 +0100
From: Christoph Hellwig <hch@....de>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: Christoph Hellwig <hch@....de>, "Darrick J. Wong" <djwong@...nel.org>,
	Chandan Babu R <chandan.babu@...cle.com>,
	Ritesh Harjani <ritesh.list@...il.com>,
	Jens Axboe <axboe@...nel.dk>,
	Andreas Gruenbacher <agruenba@...hat.com>,
	Damien Le Moal <dlemoal@...nel.org>,
	Naohiro Aota <naohiro.aota@....com>,
	Johannes Thumshirn <jth@...nel.org>, linux-xfs@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-block@...r.kernel.org,
	gfs2@...ts.linux.dev, Christian Brauner <brauner@...nel.org>,
	linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH 13/14] iomap: map multiple blocks at a time

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? :)

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