Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCHv5 3/6] ext4: Move ext4 bmap to use iomap infrastructure.
To:     "Darrick J. Wong" <darrick.wong@oracle.com>
Cc:     Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, linux-fsdevel@vger.kernel.org,
        hch@infradead.org, cmaiolino@redhat.com, david@fromorbit.com
References: <cover.1582880246.git.riteshh@linux.ibm.com>
 <8bbd53bd719d5ccfecafcce93f2bf1d7955a44af.1582880246.git.riteshh@linux.ibm.com>
 <20200228152524.GE8036@magnolia>
 <20200302085840.A41E3A4053@d06av23.portsmouth.uk.ibm.com>
 <20200303154709.GB8037@magnolia> <20200304124211.GC21048@quack2.suse.cz>
 <20200306174932.9D81D4C04E@d06av22.portsmouth.uk.ibm.com>
 <20200307005122.GI1752567@magnolia>
From:   Ritesh Harjani <riteshh@linux.ibm.com>
Date:   Sat, 7 Mar 2020 11:20:42 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <20200307005122.GI1752567@magnolia>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <20200307055043.5FBEC52051@d06av21.portsmouth.uk.ibm.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk


On 3/7/20 6:21 AM, Darrick J. Wong wrote:
> 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."

Yes. Thanks.


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

What I meant was if filemap_write_and_wait() is required for JDATA case
then the above diff which you just showed, I will add as a separate
patch before moving ext4_bmap() to use iomap_bmap(). i.e. rather then
clubbing it with Patch-3, it will be a separate patch before patch-3.

Sorry about the confusion.

-ritesh

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