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

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

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.

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

--D

> ---
>  fs/ext4/inode.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6cf3b969dc86..fac8adbbb3f6 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3174,16 +3174,6 @@ static sector_t ext4_bmap(struct address_space *mapping, sector_t block)
>  	if (ext4_has_inline_data(inode))
>  		return 0;
>  
> -	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
> -			test_opt(inode->i_sb, DELALLOC)) {
> -		/*
> -		 * With delalloc we want to sync the file
> -		 * so that we can make sure we allocate
> -		 * blocks for file
> -		 */
> -		filemap_write_and_wait(mapping);
> -	}
> -
>  	if (EXT4_JOURNAL(inode) &&
>  	    ext4_test_inode_state(inode, EXT4_STATE_JDATA)) {
>  		/*
> @@ -3214,7 +3204,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);
>  }
>  
>  static int ext4_readpage(struct file *file, struct page *page)
> -- 
> 2.21.0
>