linux-ext4 - Re: [RFC 2/3] ext2: Convert ext2 regular file buffered I/O to use iomap

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20231207085831.udom5ozzsdqucxzm@quack3>
Date: Thu, 7 Dec 2023 09:58:31 +0100
From: Jan Kara <jack@...e.cz>
To: Ritesh Harjani <ritesh.list@...il.com>
Cc: Dave Chinner <david@...morbit.com>,
	Christoph Hellwig <hch@...radead.org>, Jan Kara <jack@...e.cz>,
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [RFC 2/3] ext2: Convert ext2 regular file buffered I/O to use
 iomap

On Tue 05-12-23 20:52:26, Ritesh Harjani wrote:
> Dave Chinner <david@...morbit.com> writes:
> >>  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)
> >> +	loff_t maxblocks = (loff_t)INT_MAX;
> >> +	u8 blkbits = inode->i_blkbits;
> >> +	u32 bno;
> >> +	bool new, boundary;
> >> +	int ret;
> >> +
> >> +	if (ext2_imap_valid(wpc, inode, offset))
> >>  		return 0;
> >>  
> >> -	return ext2_iomap_begin(inode, offset, inode->i_sb->s_blocksize,
> >> +	EXT2_WPC(wpc)->ib_seq = READ_ONCE(EXT2_I(inode)->ib_seq);
> >> +
> >> +	ret = ext2_get_blocks(inode, offset >> blkbits, maxblocks << blkbits,
> >> +			      &bno, &new, &boundary, 0);
> >
> > This is incorrectly ordered. ext2_get_blocks() bumps ib_seq when it
> > does allocation, so the newly stored EXT2_WPC(wpc)->ib_seq is
> > immediately staled and the very next call to ext2_imap_valid() will
> > fail, causing a new iomap to be mapped even when not necessary.
> 
> In case of ext2 here, the allocation happens at the write time itself
> for buffered writes. So what we are essentially doing here (at the time
> of writeback) is querying ->get_blocks(..., create=0) and passing those
> no. of blocks (ret) further. So it is unlikely that the block
> allocations will happen right after we sample ib_seq
> (unless we race with truncate).
> 
> For mmapped writes, we expect to find a hole and in case of a hole at
> the offset, we only pass & cache 1 block in wpc. 
> 
> For mmapped writes case since we go and allocate 1 block, so I agree
> that the ib_seq will change right after in ->get_blocks. But since in
> this case we only alloc and cache 1 block, we anyway will have to call
> ->get_blocks irrespective of ib_seq checking.

I agree with your reasoning Ritesh but I guess it would deserve a comment
because it is a bit subtle and easily forgotten detail.

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