Re: [PATCH v3 2/3] btrfs: Split remaining space to discard in chunks

[Qu Wenruo]

在 2024/9/4 13:43, Christoph Hellwig 写道:
> On Tue, Sep 03, 2024 at 07:13:29PM +0930, Qu Wenruo wrote:
> > 在 2024/9/3 16:57, Christoph Hellwig 写道:
> > > You'll also need add a EXPORT_SYMBOL_GPL for blk_alloc_discard_bio.
> >
> > Just curious, shouldn't blk_ioctl_discard() also check frozen status to
> > prevent block device trim from block suspension?
>
> Someone needs to explain what 'block suspension' is for that first.

E.g. a long running full device trim preventing PM suspension/hibernation.

The original report shows the fstrim of btrfs is preventing the system 
entering suspension/hibernation:

 PM: suspend entry (deep)
 Filesystems sync: 0.060 seconds
 Freezing user space processes
 Freezing user space processes failed after 20.007 seconds (1 tasks 
refusing to freeze, wq_busy=0):
 task:fstrim          state:D stack:0     pid:15564 tgid:15564 ppid:1 
   flags:0x00004006
 Call Trace:
  <TASK>
  __schedule+0x381/0x1540
  schedule+0x24/0xb0
  schedule_timeout+0x1ea/0x2a0
  io_schedule_timeout+0x19/0x50
  wait_for_completion_io+0x78/0x140
  submit_bio_wait+0xaa/0xc0
  blkdev_issue_discard+0x65/0xb0
  btrfs_issue_discard+0xcf/0x160 [btrfs 
7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  btrfs_discard_extent+0x120/0x2a0 [btrfs 
7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  do_trimming+0xd4/0x220 [btrfs 7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  trim_bitmaps+0x418/0x520 [btrfs 7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  btrfs_trim_block_group+0xcb/0x130 [btrfs 
7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  btrfs_trim_fs+0x119/0x460 [btrfs 
7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  btrfs_ioctl_fitrim+0xfb/0x160 [btrfs 
7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  btrfs_ioctl+0x11cc/0x29f0 [btrfs 
7ab35b9b86062a46f6ff578bb32d55ecf8e6bf82]
  __x64_sys_ioctl+0x92/0xd0
  do_syscall_64+0x5b/0x80
  entry_SYSCALL_64_after_hwframe+0x7c/0xe6
 RIP: 0033:0x7f5f3b529f9b
 RSP: 002b:00007fff279ebc20 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
 RAX: ffffffffffffffda RBX: 00007fff279ebd60 RCX: 00007f5f3b529f9b
 RDX: 00007fff279ebc90 RSI: 00000000c0185879 RDI: 0000000000000003
 RBP: 000055748718b2d0 R08: 00005574871899e8 R09: 00007fff279eb010
 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000003
 R13: 000055748718ac40 R14: 000055748718b290 R15: 000055748718b290
  </TASK>
 OOM killer enabled.
 Restarting tasks ... done.
 random: crng reseeded on system resumption
 PM: suspend exit
 PM: suspend entry (s2idle)
 Filesystems sync: 0.047 seconds

Considering blkdev_issue_discard() is already doing cond_sched() and 
btrfs is also doing cond_sched() for each block group, it looks like PM 
freezing needs its own freezing() checks, just like what ext4 is doing.

> > And if that's the case, would it make more sense to move the signal and
> > freezing checks into blk_alloc_discard_bio()?
>
> No.  Look at that the recent history of the dicard support.  We tried
> that and it badly breaks callers not expecting the signal handling.

OK, got it, at least for btrfs we would go the blk_alloc_discard_bio() 
and do signal and freezing checks.

Thanks,
Qu