Re: [PATCH] ext4: get discard out of jbd2 commit kthread

[Wang Jianchao]

On 2021/5/19 11:08 PM, Theodore Y. Ts'o wrote:
> On Wed, May 19, 2021 at 09:27:56AM +0800, Wang Jianchao wrote:
>>
>> We're running ext4 with discard on a nbd device whose backend is storage
>> cluster. The discard can help to free the unused space to storage pool.
>>
>> And sometimes application delete a lot of data and discard is flooding. 
>> Then we see the jbd2 commit kthread is blocked for a long time. Even
>> move the discard out of jbd2, we still see the write IO of jbd2 log
>> could be blocked. blk-wbt could help to relieve this. Finally the delay
>> is shift to allocation path. But this is better than blocking the page
>> fault path which holds the read mm->mmap_sem.
> 
> I'm assuming that the problem is when the application deletes a lot of
> data, the discard flood is causing performance problems on your nbd
> server.  Is that the high level problem that you are trying to solve?
> 

Yes, not only the discard sometimes could be very slow, but also it
would degrade the performance of normal write IO

> So if that's the case, I'd suggest a different approach.  First, move
> kmem_cache_free(ext4_free_data_cachep, entry) out of
> ext4_free_data_in_buddy() to its caller, ext4_process_data.  Then if
> discard is enabled, after calling ext4_free_data_in_buddy(), the
> ext4_free_data struct will be detached from rbtree rooted in
> ext4_group_info.bb_free_root, and then we can attach it to a new
> rbtree rooted in ext4_group_info.bb_discard_root.
> 
> This allows the block to be reused as soon the commit is finished
> (allowing for potentially more efficient block allocations), but we
> can now keep track of which blocks would be useful for discarding and
> decouple that from when we release the blocks to be reused.  We can
> now use the pre-existing fstrim kernel thread infrastructure to lock a
> block group, and we can now iterate over the rbtree, and take into
> account which blocks have since become allocated --- since if a block
> has been allocated, there's no need to send a discard for it.
> 
> I think this will be more efficient, and will allow us to share more
> of the code for fstrim and the discard-at-runtime model used by "mount
> -o discard".  We can also fine-tune how quickly we issue discards; it
> might be that if user has executed "rm -rf" it might actually better
> to wait until the deletes have completed, even if it takes several
> commit intervals, since it might allow us to combine discards if the
> blocks 100-199 and 400-500 are released in one commit, and blocks
> 200-399 are released two or three commits later.


Yes, this is more efficient and fair. I will cook the next version path
based on the suggestion above.

> 
> Something else I'd urge you to consider is whether it's possible to
> enhance the nbd protocol to add some kind of back-channel notification
> when the shared storage is getting low on space.  In that case, when
> the nbd client code a request from the nbd server indicating, "please
> issue discards if possible", it could either trigger an upcall to
> userspace, which could then issue the fstrim ioctl, which in the case
> where "mount -o discard" is enabled, would accelerate when discards
> took place.
> 
> We could then make the fstrim thread normally work on a much slower
> pace, but when there is a signal from the shared storage that space is
> needed, clients could accelerate when they issue discards to free up
> shared space.

This sounds great !!!
I will share this with my colleagues to see how to implement it.

> 
> Cheers,
> 
> 						- Ted
> 
> P.S.  One other potential thought; if we have established a new
> bb_discard_root rbtree, it *might* actually be beneficial to consider
> using that information in the block allocator.  One of the best way to
> tell an SSD that block is no longer needed is to simply overwrite that
> block.  If we do that, we don't need to send a discard to that block
> any more.

This seems also true for the storage server. If the nbd client reuse the
blocks that's just freed, the server needn't to do new allocation.

> 
> Of course, we still want to keep blocks contiguous since even though
> seeks are free for SSD's, we want to keep large reads contiguous as
> much as possible, and we want to keep the extent tree as compact as
> possible.  But if we have just released a 12k file, and we are writing
> a new 12k file, and don't really care *where* in the block group we
> are writing that file, reusing blocks that had just been freed might
> actually be a good strategy.
> 
> That's not something you need to implement in this patch series, but
> it might be an interesting optimization.
> 

And thanks a million for your suggestions

Best Regards
Jianchao