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