On Tue 05-12-23 20:52:26, Ritesh Harjani wrote:
> Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxx>
SUSE Labs, CR
