On Apr 8, 2021, at 12:50 PM, Wen Yang <wenyang@xxxxxxxxxxxxxxxxx> wrote:
>
> Hi Ritesh and Andreas,
>
> Thank you for your reply. Since there is still a faulty machine, we have analyzed it again and found it is indeed a very special case:
>
>
> crash> struct ext4_group_info ffff8813bb5f72d0
> struct ext4_group_info {
> bb_state = 0,
> bb_free_root = {
> rb_node = 0x0
> },
> bb_first_free = 1681,
> bb_free = 0,
> bb_fragments = 0,
> bb_largest_free_order = -1,
> bb_prealloc_list = {
> next = 0xffff880268291d78,
> prev = 0xffff880268291d78 ---> *** The list is empty
> },
> alloc_sem = {
> count = {
> counter = 0
> },
> wait_list = {
> next = 0xffff8813bb5f7308,
> prev = 0xffff8813bb5f7308
> },
> wait_lock = {
> raw_lock = {
> {
> val = {
> counter = 0
> },
> {
> locked = 0 '\000',
> pending = 0 '\000'
> },
> {
> locked_pending = 0,
> tail = 0
> }
> }
> }
> },
> osq = {
> tail = {
> counter = 0
> }
> },
> owner = 0x0
> },
> bb_counters = 0xffff8813bb5f7328
> }
> crash>
>
>
> crash> list 0xffff880268291d78 -l ext4_prealloc_space.pa_group_list -s ext4_prealloc_space.pa_count
> ffff880268291d78
> pa_count = {
> counter = 1 ---> ****pa->pa_count
> }
> ffff8813bb5f72f0
> pa_count = {
> counter = -30701
> }
>
>
> crash> struct -xo ext4_prealloc_space
> struct ext4_prealloc_space {
> [0x0] struct list_head pa_inode_list;
> [0x10] struct list_head pa_group_list;
> union {
> struct list_head pa_tmp_list;
> struct callback_head pa_rcu;
> [0x20] } u;
> [0x30] spinlock_t pa_lock;
> [0x34] atomic_t pa_count;
> [0x38] unsigned int pa_deleted;
> [0x40] ext4_fsblk_t pa_pstart;
> [0x48] ext4_lblk_t pa_lstart;
> [0x4c] ext4_grpblk_t pa_len;
> [0x50] ext4_grpblk_t pa_free;
> [0x54] unsigned short pa_type;
> [0x58] spinlock_t *pa_obj_lock;
> [0x60] struct inode *pa_inode;
> }
> SIZE: 0x68
>
>
> Before "goto repeat", it is necessary to check whether grp->bb_prealloc_list is empty. This patch may fix it.
> Please kindly give us your comments and suggestions.
> Thanks.
This patch definitely looks more reasonable than the previous one, but
I don't think it is correct?
It isn't clear how the system could get into this state, or stay there.
If bb_prealloc_list is empty, then "busy" should be 0, since busy = 1
is only set inside "list_for_each_entry_safe(bb_prealloc_list)", and
that implies the list is *not* empty? The ext4_lock_group() is held
over this code, so the list should not be changing in this case, and
even if the list changed, it _should_ only affect the loop one time.
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 99bf091..8082e2d 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4290,7 +4290,7 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
> free_total += free;
>
> /* if we still need more blocks and some PAs were used, try again */
> - if (free_total < needed && busy) {
> + if (free_total < needed && busy && !list_empty(&grp->bb_prealloc_list)) {
> ext4_unlock_group(sb, group);
> cond_resched();
> busy = 0;
> goto repeat;
Minor suggestion - moving "busy = 0" right after "repeat:" and removing
the "int busy = 0" initializer would make this code a bit more clear.
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
