This patch appears to be working on my machine as well.
On Tue, Aug 25, 2020 at 1:37 PM Ritesh Harjani <riteshh@xxxxxxxxxxxxx> wrote:
>
>
>
> On 5/6/20 1:00 AM, Darrick J. Wong wrote:
> > On Tue, May 05, 2020 at 07:36:08PM +0100, Yuxuan Shui wrote:
> >> commit ac58e4fb03f9d111d733a4ad379d06eef3a24705 moved ext4_bmap from
> >> generic_block_bmap to iomap_bmap, this introduced a regression which
> >> prevents some user from using previously working swapfiles. The kernel
> >> will complain about holes while there is none.
> >>
> >> What is happening here is that the swapfile has unwritten mappings,
> >> which is rejected by iomap_bmap, but was accepted by ext4_get_block.
> >
> > ...which is why ext4 ought to use iomap_swapfile_activate.
>
> I tested this patch (diff below), which seems to be working fine for me
> for straight forward use case of swapon/swapoff on ext4.
> Could you give it a try?
>
> <log showing ext4_iomap_swap_activate path kicking in>
> swapon 1283 [000] 438.651028: 250000 cpu-clock:pppH:
> ffffffff817f7f56 percpu_counter_add_batch+0x26 (/boot/vmlinux)
> ffffffff813a61d0 ext4_es_lookup_extent+0x1d0 (/boot/vmlinux)
> ffffffff813b8095 ext4_map_blocks+0x65 (/boot/vmlinux)
> ffffffff813b8d4b ext4_iomap_begin_report+0x10b (/boot/vmlinux)
> ffffffff81367f58 iomap_apply+0xa8 (/boot/vmlinux)
> ffffffff8136d1c3 iomap_swapfile_activate+0xb3 (/boot/vmlinux)
> ffffffff813b51a5 ext4_iomap_swap_activate+0x15 (/boot/vmlinux)
> ffffffff812a3a27 __do_sys_swapon+0xb37 (/boot/vmlinux)
> ffffffff812a40f6 __x64_sys_swapon+0x16 (/boot/vmlinux)
> ffffffff820b760a do_syscall_64+0x5a (/boot/vmlinux)
> ffffffff8220007c entry_SYSCALL_64+0x7c (/boot/vmlinux)
> 7ffff7de68bb swapon+0xb (/usr/lib/x86_64-linux-gnu/libc-2.30.so)
> 66706177732f756d [unknown] ([unknown])
>
> <shows that swapfile(which I setup using fallocate) has some used bytes>
> $ swapon -s
> Filename Type Size Used
> Priority
> /home/qemu/swapfile-test file 2097148 42312 -2
>
>
> @Jan/Ted/Darrick,
>
> I am not that familiar with how swap subsystem works.
> So, is there anything else you feel is required apart from below changes
> for supporting swap_activate via iomap? I did test both swapon/swapoff
> and see that swap is getting used up on ext4 with delalloc mount opt.
>
> As I see from code, iomap_swapfile_activate is mainly looking for
> extent mapping information of that file to pass to swap subsystem.
> And IIUC, "ext4_iomap_report_ops" is meant exactly for that.
> Same as how we use it in ext4_fiemap().
>
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6eae17758ece..1e390157541d 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3614,6 +3614,13 @@ static int ext4_set_page_dirty(struct page *page)
> return __set_page_dirty_buffers(page);
> }
>
> +static int ext4_iomap_swap_activate(struct swap_info_struct *sis,
> + struct file *file, sector_t *span)
> +{
> + return iomap_swapfile_activate(sis, file, span,
> + &ext4_iomap_report_ops);
> +}
> +
> static const struct address_space_operations ext4_aops = {
> .readpage = ext4_readpage,
> .readahead = ext4_readahead,
> @@ -3629,6 +3636,7 @@ static const struct address_space_operations
> ext4_aops = {
> .migratepage = buffer_migrate_page,
> .is_partially_uptodate = block_is_partially_uptodate,
> .error_remove_page = generic_error_remove_page,
> + .swap_activate = ext4_iomap_swap_activate,
> };
>
> static const struct address_space_operations ext4_journalled_aops = {
> @@ -3645,6 +3653,7 @@ static const struct address_space_operations
> ext4_journalled_aops = {
> .direct_IO = noop_direct_IO,
> .is_partially_uptodate = block_is_partially_uptodate,
> .error_remove_page = generic_error_remove_page,
> + .swap_activate = ext4_iomap_swap_activate,
> };
>
> static const struct address_space_operations ext4_da_aops = {
> @@ -3662,6 +3671,7 @@ static const struct address_space_operations
> ext4_da_aops = {
> .migratepage = buffer_migrate_page,
> .is_partially_uptodate = block_is_partially_uptodate,
> .error_remove_page = generic_error_remove_page,
> + .swap_activate = ext4_iomap_swap_activate,
> };
>
> static const struct address_space_operations ext4_dax_aops = {
> @@ -3670,6 +3680,7 @@ static const struct address_space_operations
> ext4_dax_aops = {
> .set_page_dirty = noop_set_page_dirty,
> .bmap = ext4_bmap,
> .invalidatepage = noop_invalidatepage,
> + .swap_activate = ext4_iomap_swap_activate,
> };
>
> void ext4_set_aops(struct inode *inode)
--
Regards
Yuxuan Shui
