Re: [PATCH v4 13/19] ext4: call ext4_mb_mark_group_bb in ext4_free_blocks_simple

[Kemeng Shi]

on 6/13/2023 9:22 AM, Kemeng Shi wrote:
> 
> 
> on 6/12/2023 11:49 AM, Theodore Ts'o wrote:
>> On Mon, Jun 12, 2023 at 10:24:55AM +0800, Kemeng Shi wrote:
>>
>>> Hi ted, sorry for this issue. This patch added a WARN_ON for case that we free block
>>> to uninitialized block group which should be invalid.
>>> We can simply remove the WARN_ON to allow free on uninitialized block group as old
>>> way for emergency fix and I will find out why we free blocks to uninitialized block
>>> group in fast commit code path and is it a valid behavior.
>>
>> What I've done for now in the dev branch was to drop patches 12
>> through 19 of this patch series.  That seemed to be a good break
>> point, and I wanted to make sure we had something working so we can
>> start doing a lot more intesive testing on the patches so far.
>>
>> Also, that way, when you resend the last 8 patches in the patch
>> series, we can make sure they get a proper review as opposed to making
>> changes on the fly.
> Sure, I will resend last 8 patches after I solve the issue. I can also take my time
> to look at problem in this way :)
Updates for how WARN_ON of free blocks to uninitialized block group is triggerred under
fast commit path in test generic/468.

# /sbin/mkfs.ext4  -F  -q -O inline_data,fast_commit  /dev/vdc
# /bin/mount -t ext4 -o acl,user_xattr -o block_validity /dev/vdc /vdc
# /root/xfstests/bin/xfs_io -i -f -c 'truncate 4202496' -c 'pwrite 0
4202496' -c fsync -c 'falloc  4202496 104857600' /vdc/testfile

The "falloc  4202496 104857600" will trigger block allocation in a
new uninitialized block group for file range "4202496 104857600" as
following:
ext4_map_blocks
  /*
   * Alloc blocks from uninitialized block group. Change to set
   * group intialized will be full journaled.
   */
  ext4_mb_new_blocks

  [...]

  /*
   * New extents will be tracked in fast commit.
   */
  ext4_fc_track_range

  /*
   * Add new extents of allocated range to inode which still has sapce
   * in ext_inode_hdr
   */
  ext4_ext_insert_extent
    [...]
    /*
     * depth is 0 as inode has space in ext_inode_hdr, this will track
     * inode in fast commit.
     */
    ext4_ext_dirty(handle, inode, path + path->p_depth);
      ext4_mark_inode_dirty
        ext4_mark_iloc_dirty
          ext4_fc_track_inode

# /root/xfstests/bin/xfs_io -i -c fsync /vdc/testfile
The fast commit is performed in fsync as following:
vfs_fsync
  ext4_fsync_journal
    ext4_fc_commit
      ext4_fc_perform_commit
        add EXT4_FC_TAG_ADD_RANGE of new extent range
        add EXT4_FC_TAG_INODE of changed inode

# /root/xfstests/src/godown /vdc
Journaled change to set group intialized is discard as following:
ext4_shutdown
  jbd2_journal_abort

# /bin/umount /dev/vdc
# /bin/mount -t ext4 -o acl,user_xattr -o block_validity /dev/vdc /vdc
Replay fast commit when mounting and added WARN_ON is triggered as
following:
ext4_fc_replay
  /*
   * replay EXT4_FC_TAG_ADD_RANGE, add extents contains blocks from
   * uninitialized group back to inode
   */
  ext4_fc_replay_add_range

  /*
   * replay EXT4_FC_TAG_INODE, this will mark trigger WARN_ON
   */
  ext4_fc_replay_inode
    /*
     * mark all blocks in old inode free, then blocks from uninitialized
     * block is freed and WARN_ON occurs
     */
    ext4_ext_clear_bb

    /* update inode with data journaled in fast commit */
    [...]

    /*
     * mark all blocks in new inode in use, and gdp will be mark
     * initialized normally
     */
    ext4_fc_record_modified_inode
    [...]
    ext4_fc_set_bitmaps_and_counters

In this situation, free blocks to uninitialized block group do no harm.
And there may be more harmless situations, so I would like to simply
drop WARN_ON in next version.

-- 
Best wishes
Kemeng Shi