Re: [PATCH V2 4/7] ext4: add new helper interface ext4_insert_free_data

[Wang Jianchao <jianchao.wan9@gmail.com>]

On 2021/5/28 4:09 AM, Andreas Dilger wrote:
> On May 26, 2021, at 2:43 AM, Wang Jianchao <jianchao.wan9@gmail.com> wrote:
>>
>> Split the codes that inserts and merges ext4_free_data structures
>> into a new interface ext4_insert_free_data. This is preparing for
>> following async background discard.
> 
> Thank you for your patch series.  I think this is an important area to
> improve, since the current "-o discard" option adds too much overhead
> to be really usable in practice.

Yes, indeed
The discard can help to free unusable spaces back to storage cluster.
But do discard after every commit can be disaster,
 - the jbd2 commit kthread can be blocked for long time sometimes, and
   then all of the metadata modify operations are blocked due to no log
   space
 - the flooding discard can saturate the storage backend and then the
   real write operations are blocked, especially the jbd2 log records

Even in the system with this patch, we can still observed the log write IO
can be blocked by the discard T_T...

> 
> One problem with tracking the fine-grained freed extents and then using
> them directly to submit TRIM requests is that the underlying device may
> ignore TRIM requests that are too small.  Submitting the TRIM right
> after each transaction commit does not allow much time for freed blocks
> to be aggregated (e.g. "rm -r" of a big directory tree), so it would be
> better to delay TRIM requests until more freed extents can be merged.
> Since most users only run fstrim once a day or every few days, it makes
> sense to allow time to merge freed space (tunable, maybe 5-15 minutes).
> 
> However, tracking the rbtree for each group may be quite a lot of overhead
> if this is kept in memory for minutes or hours, so minimizing the memory
> usage to track freed extents is also important.
> 
> We discussed on the ext4 developer call today whether it is necessary
> to track the fine-grained free extents in memory, or if it would be
> better to only track min/max freed blocks within each group?  Depending
> on the fragmentation of the free blocks in the group, it may be enough
> to just store a single bit in each group (as is done today), and only
> clear this when there are blocks freed in the group.
> 
> Either way, the improvement would be that the kernel is scheduling
> groups to be trimmed, and submitting TRIM requests at a much larger size,
> instead of depending on userspace to run fstrim.  This also allows the
> fstrim scheduler to decide when the device is less busy and submit more
> TRIM requests, and back off when the device is busy.

Schedule a background trim task in kernel when the storage is not so busy 
and pick up a block group that that has bigger enough free blocks.
This sounds fair. 

> 
> The other potential improvement is to track the TRIMMED state persistently
> in the block groups, so that unmount/remount doesn't result in every group
> being trimmed again.  It would be good to refresh and include patches from:
> 
> "ext4: introduce EXT4_BG_WAS_TRIMMED to optimize trim"
> https://patchwork.ozlabs.org/project/linux-ext4/list/?series=184981
> 
> and
> 
> e2fsprogs: add EXT2_FLAG_BG_WAS_TRIMMED to optimize fstrim
> https://patchwork.ozlabs.org/project/linux-ext4/list/?series=179639
> 
> along with this series.
> 

Yes， thanks a million

Best regard
Jianchao

>> Signed-off-by: Wang Jianchao <wangjianchao@kuaishou.com>

> 
>