Hello,
this patch series modifies ext4 so that we stop using ext4_writepage() for
writeout of ordered data during transaction commit (through
generic_writepages() from jbd2_journal_submit_inode_data_buffers()). Instead we
directly call ext4_writepages() from the
ext4_journal_submit_inode_data_buffers(). This is part of Christoph's effort
to get rid of the .writepage() callback in all filesystems.
I have also modified ext4_writepages() to use write_cache_pages() instead of
generic_writepages() so now we don't expose .writepage hook at all. We still
keep ext4_writepage() as a callback for write_cache_pages(). We should refactor
that path as well and get rid of ext4_writepage() completely but that is for a
separate cleanup. Also note that jbd2 still uses generic_writepages() in its
jbd2_journal_submit_inode_data_buffers() helper because it is still used from
OCFS2. Again, something to be dealt with in a separate patchset.
Changes since v1:
* Added Reviewed-by tags from Ritesh
* Added patch to get rid of generic_writepages() in ext4_writepages()
* Added patch to get rid of .writepage hook
Honza
Previous versions:
Link: http://lore.kernel.org/r/[email protected] # v1
On 22/12/02 07:39PM, Jan Kara wrote:
> Hello,
>
> this patch series modifies ext4 so that we stop using ext4_writepage() for
> writeout of ordered data during transaction commit (through
> generic_writepages() from jbd2_journal_submit_inode_data_buffers()). Instead we
> directly call ext4_writepages() from the
> ext4_journal_submit_inode_data_buffers(). This is part of Christoph's effort
> to get rid of the .writepage() callback in all filesystems.
>
> I have also modified ext4_writepages() to use write_cache_pages() instead of
> generic_writepages() so now we don't expose .writepage hook at all. We still
> keep ext4_writepage() as a callback for write_cache_pages(). We should refactor
> that path as well and get rid of ext4_writepage() completely but that is for a
> separate cleanup. Also note that jbd2 still uses generic_writepages() in its
> jbd2_journal_submit_inode_data_buffers() helper because it is still used from
> OCFS2. Again, something to be dealt with in a separate patchset.
>
> Changes since v1:
> * Added Reviewed-by tags from Ritesh
> * Added patch to get rid of generic_writepages() in ext4_writepages()
> * Added patch to get rid of .writepage hook
Oh! And what about the WARN_ON_ONCE in ext4_writepages() while loop, which we
were discussing here [1]. Do you think that will help in catching anything nasty?
[1]: https://lore.kernel.org/linux-ext4/20221201115500.kbxtteft3v4pzqqx@quack3/T/#mcf7b6cc301062e52a3600194b03a9fd872ba52c5
One thing I guess I missed in my previous review is the fast commit path.
In my overnight testing of previous patch series I observed this warning.
WARNING: CPU: 1 PID: 1746936 at fs/ext4/inode.c:1994 ext4_writepage+0x4e6/0x5e0
RIP: 0010:ext4_writepage+0x4e6/0x5e0
Call Trace:
<TASK>
__writepage+0x17/0x70
write_cache_pages+0x166/0x3c0
? dirty_background_bytes_handler+0x30/0x30
? finish_task_switch.isra.0+0x8e/0x260
? _raw_spin_lock_irqsave+0x19/0x50
? finish_wait+0x34/0x70
? _raw_spin_unlock_irqrestore+0x1e/0x40
generic_writepages+0x4f/0x80
jbd2_journal_submit_inode_data_buffers+0x64/0x90
ext4_fc_commit+0x2e0/0x830
? file_check_and_advance_wb_err+0x2e/0xd0
? preempt_count_add+0x70/0xa0
ext4_sync_file+0x15c/0x380
__do_sys_msync+0x1c1/0x2a0
do_syscall_64+0x38/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
-ritesh
>
> Honza
> Previous versions:
> Link: http://lore.kernel.org/r/[email protected] # v1
On Fri, Dec 02, 2022 at 07:39:25PM +0100, Jan Kara wrote:
> I have also modified ext4_writepages() to use write_cache_pages() instead of
> generic_writepages() so now we don't expose .writepage hook at all. We still
> keep ext4_writepage() as a callback for write_cache_pages().
Nice!
> We should refactor
> that path as well and get rid of ext4_writepage() completely but that is for a
> separate cleanup.
Agreed.
> Also note that jbd2 still uses generic_writepages() in its
> jbd2_journal_submit_inode_data_buffers() helper because it is still used from
> OCFS2. Again, something to be dealt with in a separate patchset.
Indeed. I think simply moving jbd2_journal_submit_inode_data_buffers to
ocfs2 and then open coding generic_writepages will be the right thing to
do here.
On Sat 03-12-22 06:22:56, Ritesh Harjani wrote:
> On 22/12/02 07:39PM, Jan Kara wrote:
> > Hello,
> >
> > this patch series modifies ext4 so that we stop using ext4_writepage() for
> > writeout of ordered data during transaction commit (through
> > generic_writepages() from jbd2_journal_submit_inode_data_buffers()). Instead we
> > directly call ext4_writepages() from the
> > ext4_journal_submit_inode_data_buffers(). This is part of Christoph's effort
> > to get rid of the .writepage() callback in all filesystems.
> >
> > I have also modified ext4_writepages() to use write_cache_pages() instead of
> > generic_writepages() so now we don't expose .writepage hook at all. We still
> > keep ext4_writepage() as a callback for write_cache_pages(). We should refactor
> > that path as well and get rid of ext4_writepage() completely but that is for a
> > separate cleanup. Also note that jbd2 still uses generic_writepages() in its
> > jbd2_journal_submit_inode_data_buffers() helper because it is still used from
> > OCFS2. Again, something to be dealt with in a separate patchset.
> >
> > Changes since v1:
> > * Added Reviewed-by tags from Ritesh
> > * Added patch to get rid of generic_writepages() in ext4_writepages()
> > * Added patch to get rid of .writepage hook
>
> Oh! And what about the WARN_ON_ONCE in ext4_writepages() while loop,
> which we were discussing here [1]. Do you think that will help in
> catching anything nasty?
>
> [1]: https://lore.kernel.org/linux-ext4/20221201115500.kbxtteft3v4pzqqx@quack3/T/#mcf7b6cc301062e52a3600194b03a9fd872ba52c5
Ah, right. Forgot about this. Thanks for reminder.
> One thing I guess I missed in my previous review is the fast commit path.
Good point, I didn't think about that one :)
> In my overnight testing of previous patch series I observed this warning.
>
> WARNING: CPU: 1 PID: 1746936 at fs/ext4/inode.c:1994 ext4_writepage+0x4e6/0x5e0
> RIP: 0010:ext4_writepage+0x4e6/0x5e0
> Call Trace:
> <TASK>
> __writepage+0x17/0x70
> write_cache_pages+0x166/0x3c0
> ? dirty_background_bytes_handler+0x30/0x30
> ? finish_task_switch.isra.0+0x8e/0x260
> ? _raw_spin_lock_irqsave+0x19/0x50
> ? finish_wait+0x34/0x70
> ? _raw_spin_unlock_irqrestore+0x1e/0x40
> generic_writepages+0x4f/0x80
> jbd2_journal_submit_inode_data_buffers+0x64/0x90
> ext4_fc_commit+0x2e0/0x830
> ? file_check_and_advance_wb_err+0x2e/0xd0
> ? preempt_count_add+0x70/0xa0
> ext4_sync_file+0x15c/0x380
> __do_sys_msync+0x1c1/0x2a0
> do_syscall_64+0x38/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
Yep, that path needs conversion as well.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR