On Wed, Mar 04, 2020 at 01:42:11PM +0100, 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@xxxxxxxxxxxxx> > > > > > Reviewed-by: Jan Kara <jack@xxxxxxx> > > > > > --- > > > > > 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@xxxxxxxxxxxxx> > > > 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@xxxxxxxxxxxxx> > > > > 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 that lilo can read it from the devices. So removing > filemap_write_and_wait() when journalling data is wrong. <nod> > > 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... If we ever decide to deprecate FIBMAP officially and push bootloaders to use FIEMAP, then we'll have to emulate all the flushing behaviors. But that's something for a separate patch. --D > Honza > > -- > Jan Kara <jack@xxxxxxxx> > SUSE Labs, CR
