Re: [PATCH v16 00/11] xfs: Delay Ready Attributes

[Allison Henderson <allison.henderson@oracle.com>]

On 3/29/21 2:59 PM, Darrick J. Wong wrote:
> On Thu, Mar 25, 2021 at 05:32:57PM -0700, Allison Henderson wrote:
>> Hi all,
>>
>> This set is a subset of a larger series for Dealyed Attributes. Which is a
>> subset of a yet larger series for parent pointers. Delayed attributes allow
>> attribute operations (set and remove) to be logged and committed in the same
>> way that other delayed operations do. This allows more complex operations (like
>> parent pointers) to be broken up into multiple smaller transactions. To do
>> this, the existing attr operations must be modified to operate as a delayed
>> operation.  This means that they cannot roll, commit, or finish transactions.
>> Instead, they return -EAGAIN to allow the calling function to handle the
>> transaction.  In this series, we focus on only the delayed attribute portion.
>> We will introduce parent pointers in a later set.
>>
>> In this version I have reduced the set back to the "Delay Ready Attrs" sub series to
>> avoid reviewer burn out, but the extended series is available to view in the inlcuded
>> git hub links, which extend all the way through parent pointers.  Feel free to review
>> as much as feels reasonable.  The set as a whole is a bit much to digest at once, so
>> working through it in progressive subsets seems like a reasonable way to manage its
>> dev efforts.
>>
>> Lastly, in the last revision folks asked for some stress testing on the set.  On my
>> system, I found that in an fsstress test with all patches applied, we spend at most
>> %0.17 of the time in the attr routines, compared to at most %0.12 with out the set applied.
>> Both can fluctuate quite a bit depending on the other operations going on that seem to
>> occupy most of the activity.  For the most part though, I do not find these results to be
>> particularly concerning.  Though folks are certainly welcome to try it out on their own
>> system to see how the results might differ.
>>
>> Updates since v15: Mostly just review feed back from the previous revision.  I've
>> tracked changes below to help reviews recall the changes discussed
> 
> Hmm... so I ran fstests against this on an otherwise default V5
> filesystem, and saw three new regressions:
> 
> xfs/125 spat out this from the final repair run:
> 
> Phase 1 - find and verify superblock...
> Phase 2 - using internal log
> 	- zero log...
> 	- scan filesystem freespace and inode maps...
> 	- found root inode chunk
> Phase 3 - for each AG...
> 	- scan (but don't clear) agi unlinked lists...
> 	- process known inodes and perform inode discovery...
> 	- agno = 0
> attribute entry #32 in attr block 2, inode 134 is INCOMPLETE
> problem with attribute contents in inode 134
> would clear attr fork
> bad nblocks 8 for inode 134, would reset to 0
> bad anextents 4 for inode 134, would reset to 0
> 	- agno = 1
> 	- agno = 2
> 	- agno = 3
> 	- process newly discovered inodes...
> Phase 4 - check for duplicate blocks...
> 	- setting up duplicate extent list...
> 	- check for inodes claiming duplicate blocks...
> 	- agno = 0
> 	- agno = 1
> 	- agno = 2
> 	- agno = 3
> No modify flag set, skipping phase 5
> Phase 6 - check inode connectivity...
> 	- traversing filesystem ...
> 	- traversal finished ...
> 	- moving disconnected inodes to lost+found ...
> Phase 7 - verify link counts...
> No modify flag set, skipping filesystem flush and exiting.
> xfs_repair should not fail
> 
> And xfs/434 and xfs/436 both complained about memory leaks stemming from
> an xfs_da_state that xfs/125 didn't free correctly:
> 
> [ 1247.150683] =============================================================================
> [ 1247.151799] BUG xfs_da_state (Tainted: G    B   W        ): Objects remaining in xfs_da_state on __kmem_cache_shutdown()
> [ 1247.153246] -----------------------------------------------------------------------------
> [ 1247.153246]
> [ 1247.154528] INFO: Slab 0xffffea00002e9280 objects=17 used=11 fp=0xffff88800ba4b4a0 flags=0xfff80000010200
> [ 1247.155764] CPU: 2 PID: 50257 Comm: modprobe Tainted: G    B   W         5.12.0-rc4-djwx #rc4
> [ 1247.156849] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> [ 1247.157996] Call Trace:
> [ 1247.158330]  dump_stack+0x64/0x7c
> [ 1247.158767]  slab_err+0xb7/0xdc
> [ 1247.159196]  ? printk+0x58/0x6f
> [ 1247.159615]  __kmem_cache_shutdown.cold+0x39/0x15e
> [ 1247.160248]  kmem_cache_destroy+0x3f/0x110
> [ 1247.160779]  xfs_destroy_zones+0xbe/0xe2 [xfs]
> [ 1247.161462]  exit_xfs_fs+0x5f/0x9b4 [xfs]
> [ 1247.162065]  __do_sys_delete_module.constprop.0+0x145/0x220
> [ 1247.162740]  do_syscall_64+0x2d/0x40
> [ 1247.163197]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [ 1247.163810] RIP: 0033:0x7fd91cfe4bcb
> [ 1247.164262] Code: 73 01 c3 48 8b 0d c5 82 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 95 82 0c 00 f7 d8 64 89 01 48
> [ 1247.166352] RSP: 002b:00007fff89097038 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
> [ 1247.167217] RAX: ffffffffffffffda RBX: 0000558b8e105cc0 RCX: 00007fd91cfe4bcb
> [ 1247.167998] RDX: 0000000000000000 RSI: 0000000000000800 RDI: 0000558b8e105d28
> [ 1247.168781] RBP: 0000558b8e105cc0 R08: 0000000000000000 R09: 0000000000000000
> [ 1247.169562] R10: 00007fd91d060ac0 R11: 0000000000000206 R12: 0000558b8e105d28
> [ 1247.170351] R13: 0000000000000000 R14: 0000558b8e105d28 R15: 0000558b8e105cc0
> 
>  From a quick bisect, all of thse problem originates in the last patch.
Alrighty, I will see if I can recreate these bugs and get that figured 
out.  Thanks!

Allison
> 
> --D
> 
>> xfs: Reverse apply 72b97ea40d
>>    NEW
>>
>> xfs: Add helper xfs_attr_node_remove_step
>>    DROPPED
>>
>> xfs: Add xfs_attr_node_remove_cleanup
>>    No change
>>
>> xfs: Hoist transaction handling in xfs_attr_node_remove_step
>>    DROPPED
>>
>> xfs: Hoist xfs_attr_set_shortform
>>    No change
>>
>> xfs: Add helper xfs_attr_set_fmt
>>    Fixed helper to return error when defer_finish fails
>>
>> xfs: Separate xfs_attr_node_addname and xfs_attr_node_addname_work
>>    Renamed xfs_attr_node_addname_work to xfs_attr_node_addname_clear_incomplete
>>
>> xfs: Add helper xfs_attr_node_addname_find_attr
>>    Renamed goto out, to goto error
>>
>> xfs: Hoist xfs_attr_node_addname
>>    Removed unused retval variable
>>    Removed extra state free in xfs_attr_node_addname
>>
>> xfs: Hoist xfs_attr_leaf_addname
>>    Fixed spelling typos
>>
>> xfs: Hoist node transaction handling
>>    Added consistent braces to if/else statement
>>
>> xfs: Add delay ready attr remove routines
>>    Typo fixes
>>    Merged xfs_attr_remove_iter with xfs_attr_node_removename_iter
>>    Added state XFS_DAS_RMTBLK
>>    Flow chart updated
>>
>> xfs: Add delay ready attr set routines
>>    Rebase adjustments
>>    Typo fixes
>>
>>
>> Extended Series Changes
>> ------------------------
>> xfs: Add state machine tracepoints
>>    Rebase adjustments
>>    xfs_attr_node_remove_rmt_return removed to match earlier refactoring changes
>>    trace_xfs_attr_node_removename_iter_return becomes
>>    trace_xfs_attr_remove_iter_return to match earlier refactoring changes
>>
>> xfs: Rename __xfs_attr_rmtval_remove
>>    No change
>>
>> xfs: Handle krealloc errors in xlog_recover_add_to_cont_trans
>>    Added kmem_alloc_large fall back
>>   
>> xfs: Set up infrastructure for deferred attribute operations
>>    Typo fixes
>>    Rename xfs_trans_attr to xfs_trans_attr_finish_update
>>    Added helper function xfs_attri_validate
>>    Split patch into infrastructure and implementation patches
>>    Added XFS_ERROR_REPORT in xlog_recover_attri_commit_pass2:
>>
>> xfs: Implement for deferred attribute operations
>>    NEW
>>
>> xfs: Skip flip flags for delayed attrs
>>    Did a performance analysis
>>
>> xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred
>>    Typo fixes
>>
>> xfs: Remove unused xfs_attr_*_args
>>    Rebase adjustments
>>
>> xfs: Add delayed attributes error tag
>>    Added errortag include
>>
>> xfs: Merge xfs_delattr_context into xfs_attr_item
>>    Typo fixes
>>
>>
>> This series can be viewed on github here:
>> https://urldefense.com/v3/__https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_v16__;!!GqivPVa7Brio!Ol0JaXyUI3K5MEGrBo_eMHzcTqcuIL9p25-XSZftWgn4bmbxeX_AJf7Hl-kP6ecxUskY$
>>
>> As well as the extended delayed attribute and parent pointer series:
>> https://urldefense.com/v3/__https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_v16_extended__;!!GqivPVa7Brio!Ol0JaXyUI3K5MEGrBo_eMHzcTqcuIL9p25-XSZftWgn4bmbxeX_AJf7Hl-kP6QsZXJc5$
>>
>> And the test cases:
>> https://urldefense.com/v3/__https://github.com/allisonhenderson/xfs_work/tree/pptr_xfstestsv2__;!!GqivPVa7Brio!Ol0JaXyUI3K5MEGrBo_eMHzcTqcuIL9p25-XSZftWgn4bmbxeX_AJf7Hl-kP6fAPHdk4$
>>
>> In order to run the test cases, you will need have the corresponding xfsprogs
>> changes as well.  Which can be found here:
>> https://urldefense.com/v3/__https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_xfsprogs_v16__;!!GqivPVa7Brio!Ol0JaXyUI3K5MEGrBo_eMHzcTqcuIL9p25-XSZftWgn4bmbxeX_AJf7Hl-kP6ToIBbr7$
>> https://urldefense.com/v3/__https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_xfsprogs_v16_extended__;!!GqivPVa7Brio!Ol0JaXyUI3K5MEGrBo_eMHzcTqcuIL9p25-XSZftWgn4bmbxeX_AJf7Hl-kP6cfTeHmh$
>>
>> To run the xfs attributes tests run:
>> check -g attr
>>
>> To run as delayed attributes run:
>> export MOUNT_OPTIONS="-o delattr"
>> check -g attr
>>
>> To run parent pointer tests:
>> check -g parent
>>
>> I've also made the corresponding updates to the user space side as well, and ported anything
>> they need to seat correctly.
>>
>> Questions, comment and feedback appreciated!
>>
>> Thanks all!
>> Allison
>>
>> Allison Henderson (11):
>>    xfs: Reverse apply 72b97ea40d
>>    xfs: Add xfs_attr_node_remove_cleanup
>>    xfs: Hoist xfs_attr_set_shortform
>>    xfs: Add helper xfs_attr_set_fmt
>>    xfs: Separate xfs_attr_node_addname and
>>      xfs_attr_node_addname_clear_incomplete
>>    xfs: Add helper xfs_attr_node_addname_find_attr
>>    xfs: Hoist xfs_attr_node_addname
>>    xfs: Hoist xfs_attr_leaf_addname
>>    xfs: Hoist node transaction handling
>>    xfs: Add delay ready attr remove routines
>>    xfs: Add delay ready attr set routines
>>
>>   fs/xfs/libxfs/xfs_attr.c        | 903 ++++++++++++++++++++++++----------------
>>   fs/xfs/libxfs/xfs_attr.h        | 364 ++++++++++++++++
>>   fs/xfs/libxfs/xfs_attr_leaf.c   |   2 +-
>>   fs/xfs/libxfs/xfs_attr_remote.c | 126 ++++--
>>   fs/xfs/libxfs/xfs_attr_remote.h |   7 +-
>>   fs/xfs/xfs_attr_inactive.c      |   2 +-
>>   fs/xfs/xfs_trace.h              |   1 -
>>   7 files changed, 998 insertions(+), 407 deletions(-)
>>
>> -- 
>> 2.7.4
>>