Re: [PATCH] ext4: add a configurable parameter to prevent endless loop in ext4_mb_discard_group_p

On Apr 8, 2021, at 12:50 PM, Wen Yang [off-list ref] 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.

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