mbox series

[v6,0/2] btrfs: Don't block system suspend during fstrim

Message ID 20240917203346.9670-1-luca.stefani.ge1@gmail.com (mailing list archive)
Headers show
Series btrfs: Don't block system suspend during fstrim | expand

Message

Luca Stefani Sept. 17, 2024, 8:33 p.m. UTC
Changes since v5:
* Make chunk size a define
* Remove superfluous trim_interrupted checks
  after moving them to trim_no_bitmap/trim_bitmaps

Changes since v4:
* Set chunk size to 1G
* Set proper error return codes in case of interruption
* Dropped fstrim_range fixup as pulled in -next

Changes since v3:
* Went back to manual chunk size

Changes since v2:
* Use blk_alloc_discard_bio directly
* Reset ret to ERESTARTSYS

Changes since v1:
* Use bio_discard_limit to calculate chunk size
* Makes use of the split chunks

Original discussion: https://lore.kernel.org/lkml/20240822164908.4957-1-luca.stefani.ge1@gmail.com/
v1: https://lore.kernel.org/lkml/20240902114303.922472-1-luca.stefani.ge1@gmail.com/
v2: https://lore.kernel.org/lkml/20240902205828.943155-1-luca.stefani.ge1@gmail.com/
v3: https://lore.kernel.org/lkml/20240903071625.957275-4-luca.stefani.ge1@gmail.com/
v4: https://lore.kernel.org/lkml/20240916101615.116164-1-luca.stefani.ge1@gmail.com/
v5: https://lore.kernel.org/lkml/20240916125707.127118-1-luca.stefani.ge1@gmail.com/

---

NB: I didn't change btrfs_discard_workfn yet to add error checks
as I don't know what semantics we should have in that case.
The work queue is always re-scheduled and created with WQ_FREEZABLE
so it should be automatically frozen. Shall I simply add some logs?

---

Luca Stefani (2):
  btrfs: Split remaining space to discard in chunks
  btrfs: Don't block system suspend during fstrim

 fs/btrfs/extent-tree.c      | 26 +++++++++++++++++++++-----
 fs/btrfs/free-space-cache.c |  4 ++--
 fs/btrfs/free-space-cache.h |  6 ++++++
 fs/btrfs/volumes.h          |  1 +
 4 files changed, 30 insertions(+), 7 deletions(-)

Comments

David Sterba Sept. 18, 2024, 11:52 a.m. UTC | #1
On Tue, Sep 17, 2024 at 10:33:03PM +0200, Luca Stefani wrote:
> Changes since v5:
> * Make chunk size a define
> * Remove superfluous trim_interrupted checks
>   after moving them to trim_no_bitmap/trim_bitmaps

Thanks, I haven't spotted anything left to fix or update. The patches
are short enough to be backported so that can happen around 6.12-rc1
time.
David Sterba Sept. 18, 2024, 11:53 a.m. UTC | #2
On Tue, Sep 17, 2024 at 10:33:03PM +0200, Luca Stefani wrote:
> Changes since v5:
> * Make chunk size a define
> * Remove superfluous trim_interrupted checks
>   after moving them to trim_no_bitmap/trim_bitmaps
> 
> Changes since v4:
> * Set chunk size to 1G
> * Set proper error return codes in case of interruption
> * Dropped fstrim_range fixup as pulled in -next
> 
> Changes since v3:
> * Went back to manual chunk size
> 
> Changes since v2:
> * Use blk_alloc_discard_bio directly
> * Reset ret to ERESTARTSYS
> 
> Changes since v1:
> * Use bio_discard_limit to calculate chunk size
> * Makes use of the split chunks
> 
> Original discussion: https://lore.kernel.org/lkml/20240822164908.4957-1-luca.stefani.ge1@gmail.com/
> v1: https://lore.kernel.org/lkml/20240902114303.922472-1-luca.stefani.ge1@gmail.com/
> v2: https://lore.kernel.org/lkml/20240902205828.943155-1-luca.stefani.ge1@gmail.com/
> v3: https://lore.kernel.org/lkml/20240903071625.957275-4-luca.stefani.ge1@gmail.com/
> v4: https://lore.kernel.org/lkml/20240916101615.116164-1-luca.stefani.ge1@gmail.com/
> v5: https://lore.kernel.org/lkml/20240916125707.127118-1-luca.stefani.ge1@gmail.com/
> 
> ---
> 
> NB: I didn't change btrfs_discard_workfn yet to add error checks
> as I don't know what semantics we should have in that case.
> The work queue is always re-scheduled and created with WQ_FREEZABLE
> so it should be automatically frozen. Shall I simply add some logs?

About that, this may be a bit tricky, interrupting trim there is handled
but the accounting may get wrong. This is related but a different
problem than the suspend vs trim.
Qu Wenruo Sept. 18, 2024, 9:34 p.m. UTC | #3
在 2024/9/18 06:03, Luca Stefani 写道:
> Changes since v5:
> * Make chunk size a define
> * Remove superfluous trim_interrupted checks
>    after moving them to trim_no_bitmap/trim_bitmaps
> 
> Changes since v4:
> * Set chunk size to 1G
> * Set proper error return codes in case of interruption
> * Dropped fstrim_range fixup as pulled in -next
> 
> Changes since v3:
> * Went back to manual chunk size
> 
> Changes since v2:
> * Use blk_alloc_discard_bio directly
> * Reset ret to ERESTARTSYS
> 
> Changes since v1:
> * Use bio_discard_limit to calculate chunk size
> * Makes use of the split chunks
> 
> Original discussion: https://lore.kernel.org/lkml/20240822164908.4957-1-luca.stefani.ge1@gmail.com/
> v1: https://lore.kernel.org/lkml/20240902114303.922472-1-luca.stefani.ge1@gmail.com/
> v2: https://lore.kernel.org/lkml/20240902205828.943155-1-luca.stefani.ge1@gmail.com/
> v3: https://lore.kernel.org/lkml/20240903071625.957275-4-luca.stefani.ge1@gmail.com/
> v4: https://lore.kernel.org/lkml/20240916101615.116164-1-luca.stefani.ge1@gmail.com/
> v5: https://lore.kernel.org/lkml/20240916125707.127118-1-luca.stefani.ge1@gmail.com/
> 
Reviewed-by: Qu Wenruo <wqu@suse.com>

> ---
> 
> NB: I didn't change btrfs_discard_workfn yet to add error checks
> as I don't know what semantics we should have in that case.
> The work queue is always re-scheduled and created with WQ_FREEZABLE
> so it should be automatically frozen. Shall I simply add some logs?

And that is for async discard, which has a much smaller discard size and 
iops limits.

The main part of large discard is the free extents discarding, which is 
already addressed by your patch correctly now.

IIRC we do not need to bother that much for it.

Thanks,
Qu

> 
> ---
> 
> Luca Stefani (2):
>    btrfs: Split remaining space to discard in chunks
>    btrfs: Don't block system suspend during fstrim
> 
>   fs/btrfs/extent-tree.c      | 26 +++++++++++++++++++++-----
>   fs/btrfs/free-space-cache.c |  4 ++--
>   fs/btrfs/free-space-cache.h |  6 ++++++
>   fs/btrfs/volumes.h          |  1 +
>   4 files changed, 30 insertions(+), 7 deletions(-)
>
David Sterba Sept. 19, 2024, 12:55 p.m. UTC | #4
On Tue, Sep 17, 2024 at 10:33:03PM +0200, Luca Stefani wrote:
> Changes since v5:
> * Make chunk size a define
> * Remove superfluous trim_interrupted checks
>   after moving them to trim_no_bitmap/trim_bitmaps
> 
> Changes since v4:
> * Set chunk size to 1G
> * Set proper error return codes in case of interruption
> * Dropped fstrim_range fixup as pulled in -next
> 
> Changes since v3:
> * Went back to manual chunk size
> 
> Changes since v2:
> * Use blk_alloc_discard_bio directly
> * Reset ret to ERESTARTSYS
> 
> Changes since v1:
> * Use bio_discard_limit to calculate chunk size
> * Makes use of the split chunks
> 
> Original discussion: https://lore.kernel.org/lkml/20240822164908.4957-1-luca.stefani.ge1@gmail.com/
> v1: https://lore.kernel.org/lkml/20240902114303.922472-1-luca.stefani.ge1@gmail.com/
> v2: https://lore.kernel.org/lkml/20240902205828.943155-1-luca.stefani.ge1@gmail.com/
> v3: https://lore.kernel.org/lkml/20240903071625.957275-4-luca.stefani.ge1@gmail.com/
> v4: https://lore.kernel.org/lkml/20240916101615.116164-1-luca.stefani.ge1@gmail.com/
> v5: https://lore.kernel.org/lkml/20240916125707.127118-1-luca.stefani.ge1@gmail.com/
> 
> ---
> 
> NB: I didn't change btrfs_discard_workfn yet to add error checks
> as I don't know what semantics we should have in that case.
> The work queue is always re-scheduled and created with WQ_FREEZABLE
> so it should be automatically frozen. Shall I simply add some logs?
> 
> ---
> 
> Luca Stefani (2):
>   btrfs: Split remaining space to discard in chunks
>   btrfs: Don't block system suspend during fstrim

Added to for-next, with some minor updates to changelogs. Thanks.