Message-Id: <20200306174932.9D81D4C04E@d06av22.portsmouth.uk.ibm.com>
Date:   Fri, 6 Mar 2020 23:19:31 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     Jan Kara <jack@...e.cz>,
        "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, linux-fsdevel@...r.kernel.org,
        hch@...radead.org, cmaiolino@...hat.com, david@...morbit.com
Subject: Re: [PATCHv5 3/6] ext4: Move ext4 bmap to use iomap infrastructure.



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@...ux.ibm.com>
>>>>> Reviewed-by: Jan Kara <jack@...e.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@...ux.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@...ux.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?

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.


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

Powered by blists - more mailing lists