Re: [PATCH v2 0/11] ext4: Stop using ext4_writepage() for writeout of ordered data

From: Jan Kara <jack@suse.cz>
To: Ritesh Harjani <ritesh.list@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Ted Tso <tytso@mit.edu>,
	linux-ext4@vger.kernel.org, Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH v2 0/11] ext4: Stop using ext4_writepage() for writeout of ordered data
Date: Mon, 5 Dec 2022 10:13:35 +0100	[thread overview]
Message-ID: <20221205091335.j2niiws6knrxmjjx@quack3> (raw)
In-Reply-To: <20221203005256.cqrvojj47blasal7@riteshh-domain>

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 <jack@suse.com>
SUSE Labs, CR