Re: [PATCHv5 3/6] ext4: Move ext4 bmap to use iomap infrastructure.

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Mar 06, 2020 at 11:19:31PM +0530, Ritesh Harjani wrote:
> 
> 
> On 3/4/20 6:12 PM, Jan Kara wrote:
> > On Tue 03-03-20 07:47:09, Darrick J. Wong wrote:
> > > On Mon, Mar 02, 2020 at 02:28:39PM +0530, Ritesh Harjani wrote:
> > > > 
> > > > 
> > > > On 2/28/20 8:55 PM, Darrick J. Wong wrote:
> > > > > On Fri, Feb 28, 2020 at 02:56:56PM +0530, Ritesh Harjani wrote:
> > > > > > ext4_iomap_begin is already implemented which provides ext4_map_blocks,
> > > > > > so just move the API from generic_block_bmap to iomap_bmap for iomap
> > > > > > conversion.
> > > > > > 
> > > > > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > > > > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > > > > ---
> > > > > >    fs/ext4/inode.c | 2 +-
> > > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > > > > > index 6cf3b969dc86..81fccbae0aea 100644
> > > > > > --- a/fs/ext4/inode.c
> > > > > > +++ b/fs/ext4/inode.c
> > > > > > @@ -3214,7 +3214,7 @@ static sector_t ext4_bmap(struct address_space *mapping, sector_t block)
> > > > > >    			return 0;
> > > > > >    	}
> > > > > > -	return generic_block_bmap(mapping, block, ext4_get_block);
> > > > > > +	return iomap_bmap(mapping, block, &ext4_iomap_ops);
> > > > > 
> > > > > /me notes that iomap_bmap will filemap_write_and_wait for you, so one
> > > > > could optimize ext4_bmap to avoid the double-flush by moving the
> > > > > filemap_write_and_wait at the top of the function into the JDATA state
> > > > > clearing block.
> > > > 
> > > > IIUC, delalloc and data=journal mode are both mutually exclusive.
> > > > So we could get rid of calling filemap_write_and_wait() all together
> > > > from ext4_bmap().
> > > > And as you pointed filemap_write_and_wait() is called by default in
> > > > iomap_bmap which should cover for delalloc case.
> > > > 
> > > > 
> > > > @Jan/Darrick,
> > > > Could you check if the attached patch looks good. If yes then
> > > > will add your Reviewed-by and send a v6.
> > > > 
> > > > Thanks for the review!!
> > > > 
> > > > -ritesh
> > > > 
> > > > 
> > > 
> > > >  From 93f560d9a483b4f389056e543012d0941734a8f4 Mon Sep 17 00:00:00 2001
> > > > From: Ritesh Harjani <riteshh@linux.ibm.com>
> > > > Date: Tue, 20 Aug 2019 18:36:33 +0530
> > > > Subject: [PATCH 3/6] ext4: Move ext4 bmap to use iomap infrastructure.
> > > > 
> > > > ext4_iomap_begin is already implemented which provides ext4_map_blocks,
> > > > so just move the API from generic_block_bmap to iomap_bmap for iomap
> > > > conversion.
> > > > 
> > > > Also no need to call for filemap_write_and_wait() any more in ext4_bmap
> > > > since data=journal mode anyway doesn't support delalloc and for all other
> > > > cases iomap_bmap() anyway calls the same function, so no need for doing
> > > > it twice.
> > > > 
> > > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > > 
> > > Hmmm.  I don't recall how jdata actually works, but I get the impression
> > > here that we're trying to flush dirty data out to the journal and then
> > > out to disk, and then drop the JDATA state from the inode.  This
> > > mechanism exists (I guess?) so that dirty file pages get checkpointed
> > > out of jbd2 back into the filesystem so that bmap() returns meaningful
> > > results to lilo.
> > 
> > Exactly. E.g. when we are journalling data, we fill hole through mmap, we will
> > have block allocated as unwritten and we need to write it out so that the
> > data gets to the journal and then do journal flush to get the data to disk
> 
> So in data=journal case in ext4_page_mkwrite the data buffer will also
> be marked as, to be journalled. So does jbd2_journal_flush() itself
> don't take care of writing back any dirty page cache before it commit
> that transaction? and after then checkpoint it?

Er... this sentence is a little garbled, but I think the answer you're
looking for is:

"Yes, writeback (i.e. filemap_write_and_wait) attaches the dirty blocks
to a journal transaction; then jbd2_journal_flush forces the transaction
data out to the on-disk journal; and it also checkpoints the journal so
that the dirty blocks are then written back into the filesystem."

> Sorry my knowledge about jbd2 is very naive.
> 
> > so that lilo can read it from the devices. So removing
> > filemap_write_and_wait() when journalling data is wrong.
> 
> Sure I understand this part. But was just curious on above query.
> Otherwise, IIUC, we will have to add
> filemap_write_and_wait() for JDATA case as well before calling
> for jbd2_journal_flush(). Will add this as a separate patch.

Well you could just move it...

bmap()
{
	/*
	 * In data=journal mode, we must checkpoint the journal to
	 * ensure that any dirty blocks in the journalare checkpointed
	 * to the location that we return to userspace.  Clear JDATA so
	 * that future writes will not be written through the journal.
	 */
	if (JDATA) {
		filemap_write_and_wait(...);
		clear JDATA
		jbd2_journal_flush(...);
	}

	return iomap_bmap(...);
}

(or did "Will add this as a separate patch" refer to fixing FIEMAP?)

--D

> 
> -ritesh
> 
> > 
> > > This makes me wonder if you still need the filemap_write_and_wait in the
> > > JDATA case because otherwise the journal flush won't have the effect of
> > > writing all the dirty pagecache back to the filesystem?  OTOH I suppose
> > > the implicit write-and-wait call after we clear JDATA will not be
> > > writing to the journal.
> > > 
> > > Even more weirdly, the FIEMAP code doesn't drop JDATA at all...?
> > 
> > Yeah, it should do that but that's only performance optimization so that we
> > bother with journal flushing only when someone uses block mapping call on
> > a file with journalled dirty data. So you can hardly notice the bug by
> > testing...
> > 
> > 								Honza
> > 
>