Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 3 Dec 2022 06:22:56 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Jan Kara <jack@suse.cz>
Cc:     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
Message-ID: <20221203005256.cqrvojj47blasal7@riteshh-domain>
References: <20221202163815.22928-1-jack@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221202163815.22928-1-jack@suse.cz>
Precedence: bulk

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/20221130162435.2324-1-jack@suse.cz # v1