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@xxxxxxxx> SUSE Labs, CR
