| Message ID | 20200818133015.25398-1-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xfs: more unlinked inode list optimization v4 | expand |
On Tue, Aug 18, 2020 at 09:30:12PM +0800, Gao Xiang wrote: > Hi forks, > > This is RFC v4 version which is based on Dave's latest patchset: > https://lore.kernel.org/r/20200812092556.2567285-1-david@fromorbit.com As we already discussed on IRC, please send new revisions of patchsets as a separate thread from the old submission. > I didn't send out v3 because it was based on Dave's previous RFC > patchset, but I'm still not quite sure to drop RFC tag since this > version is different from the previous versions... Hm, this cover letter could use some tidying up, since it took me a bit of digging to figure out that yes, this is the successor of the old series that tried to get the AGI buffer lock out of the way if we're adding a newly unlinked inode to the end of the unlinked list. > Changes since v2: > - rebase on new patchset, and omit the original first patch > "xfs: arrange all unlinked inodes into one list" since it now > has better form in the base patchset; > > - a tail xfs_inode pointer is no longer needed since the original > patchset introduced list_head iunlink infrastructure and it can > be used to get the tail inode; > > - take pag_iunlink_mutex lock until all iunlink log items are > committed. Otherwise, xfs_iunlink_log() order would not be equal > to the trans commit order so it can mis-reorder and cause metadata > corruption I mentioned in v2. > > In order to archive that, some recursive count is introduced since > there could be several iunlink operations in one transaction, > and introduce some per-AG fields as well since these operations > in the transaction may not operate inodes in the same AG. we may > also need to take AGI buffer lock in advance (e.g. whiteout rename > path) due to iunlink operations and locking order constraint. > For more details, see related inlined comments as well... > > - "xfs: get rid of unused pagi_unlinked_hash" would be better folded > into original patchset since pagi_unlinked_hash is no longer needed. > > ============ > > [Original text] > > This RFC patchset mainly addresses the thoughts [*] and [**] from Dave's > original patchset, > https://lore.kernel.org/r/20200623095015.1934171-1-david@fromorbit.com > > In short, it focues on the following ideas mentioned by Dave: > - use bucket 0 instead of multiple buckets since in-memory double > linked list finally works; > > - avoid taking AGI buffer and unnecessary AGI update if possible, so > 1) add a new lock and keep proper locking order to avoid deadlock; > 2) insert a new unlinked inode from the tail instead of head; > > In addition, it's worth noticing 3 things: > - xfs_iunlink_remove() should support old multiple buckets in order > to keep old inode unlinked list (old image) working when recovering. > > - (but) OTOH, the old kernel recovery _shouldn't_ work with new image > since the bucket_index from old xfs_iunlink_remove() is generated > by the old formula (rather than keep in xfs_inode), which is now > fixed as 0. So this feature is not forward compatible without some > extra backport patches; Oh? These seem like serious limitations, are they still true? --D > - a tail xfs_inode pointer is also added in the perag, which keeps > track of the tail of bucket 0 since it's mainly used for xfs_iunlink(). > > > The git tree is also available at > git://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git tags/xfs/iunlink_opt_v4 > > Gitweb: > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/log/?h=xfs/iunlink_opt_v4 > > > Some preliminary tests are done (including fstests, but there seems > some pre-exist failures and I haven't looked into yet). And I confirmed > there was no previous metadata corruption mentioned in RFC v2 anymore. > > To confirm that I'm in the right direction, I post the latest version > now since it haven't been updated for a while. > > Comments and directions are welcomed. :) > > Thanks, > Gao Xiang > > Gao Xiang (3): > xfs: get rid of unused pagi_unlinked_hash > xfs: introduce perag iunlink lock > xfs: insert unlinked inodes from tail > > fs/xfs/xfs_inode.c | 194 ++++++++++++++++++++++++++++++++------ > fs/xfs/xfs_inode.h | 1 + > fs/xfs/xfs_iunlink_item.c | 16 ++++ > fs/xfs/xfs_mount.c | 4 + > fs/xfs/xfs_mount.h | 14 +-- > 5 files changed, 193 insertions(+), 36 deletions(-) > > -- > 2.18.1 >
Hi Darrick, On Tue, Aug 18, 2020 at 05:53:34PM -0700, Darrick J. Wong wrote: > On Tue, Aug 18, 2020 at 09:30:12PM +0800, Gao Xiang wrote: > > Hi forks, > > > > This is RFC v4 version which is based on Dave's latest patchset: > > https://lore.kernel.org/r/20200812092556.2567285-1-david@fromorbit.com > > As we already discussed on IRC, please send new revisions of patchsets > as a separate thread from the old submission. Okay, will definitely do later. > > > I didn't send out v3 because it was based on Dave's previous RFC > > patchset, but I'm still not quite sure to drop RFC tag since this > > version is different from the previous versions... > > Hm, this cover letter could use some tidying up, since it took me a bit > of digging to figure out that yes, this is the successor of the old > series that tried to get the AGI buffer lock out of the way if we're > adding a newly unlinked inode to the end of the unlinked list. I'm trying to sort out in the next revision if something shows unclear in the code. I talked with Dave person-to-person weeks ago about these constraints on IRC, but I'm not sure I can write out some fluent formal words... I think there are 2 independent things: 1) avoiding taking AGI buffer lock if AGI buffer is untouched; 2) adding a newly unlinked inode to the end of the unlinked list. So, 2) can be achieved as well without 1) since AGI buffer lock is a powerful lock and can be recursively taken, but if we'd like to add a new per-AG iunlink lock, there are some new constraints (locking order and deadlock concerns) than the current approach. In summary, due to many exist paths (e.g. tmpfile path), we need to take lock in the following order: AGI buffer lock -> per-AG iunlink lock. Otherwise it could cause dead lock. And we cannot release per-AG iunlink lock before all iunlink operations in this trans are commited, or it could cause iunlink fs corruption... > > > Changes since v2: > > - rebase on new patchset, and omit the original first patch > > "xfs: arrange all unlinked inodes into one list" since it now > > has better form in the base patchset; > > > > - a tail xfs_inode pointer is no longer needed since the original > > patchset introduced list_head iunlink infrastructure and it can > > be used to get the tail inode; > > > > - take pag_iunlink_mutex lock until all iunlink log items are > > committed. Otherwise, xfs_iunlink_log() order would not be equal > > to the trans commit order so it can mis-reorder and cause metadata > > corruption I mentioned in v2. > > > > In order to archive that, some recursive count is introduced since > > there could be several iunlink operations in one transaction, > > and introduce some per-AG fields as well since these operations > > in the transaction may not operate inodes in the same AG. we may > > also need to take AGI buffer lock in advance (e.g. whiteout rename > > path) due to iunlink operations and locking order constraint. > > For more details, see related inlined comments as well... > > > > - "xfs: get rid of unused pagi_unlinked_hash" would be better folded > > into original patchset since pagi_unlinked_hash is no longer needed. > > > > ============ > > > > [Original text] > > > > This RFC patchset mainly addresses the thoughts [*] and [**] from Dave's > > original patchset, > > https://lore.kernel.org/r/20200623095015.1934171-1-david@fromorbit.com > > > > In short, it focues on the following ideas mentioned by Dave: > > - use bucket 0 instead of multiple buckets since in-memory double > > linked list finally works; > > > > - avoid taking AGI buffer and unnecessary AGI update if possible, so > > 1) add a new lock and keep proper locking order to avoid deadlock; > > 2) insert a new unlinked inode from the tail instead of head; > > > > In addition, it's worth noticing 3 things: > > - xfs_iunlink_remove() should support old multiple buckets in order > > to keep old inode unlinked list (old image) working when recovering. > > > > - (but) OTOH, the old kernel recovery _shouldn't_ work with new image > > since the bucket_index from old xfs_iunlink_remove() is generated > > by the old formula (rather than keep in xfs_inode), which is now > > fixed as 0. So this feature is not forward compatible without some > > extra backport patches; > > Oh? These seem like serious limitations, are they still true? Yeah, I think that's still true (I tested on my VM before). Thanks, Gao Xiang > > --D > > > - a tail xfs_inode pointer is also added in the perag, which keeps > > track of the tail of bucket 0 since it's mainly used for xfs_iunlink(). > > > > > > The git tree is also available at > > git://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git tags/xfs/iunlink_opt_v4 > > > > Gitweb: > > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/log/?h=xfs/iunlink_opt_v4 > > > > > > Some preliminary tests are done (including fstests, but there seems > > some pre-exist failures and I haven't looked into yet). And I confirmed > > there was no previous metadata corruption mentioned in RFC v2 anymore. > > > > To confirm that I'm in the right direction, I post the latest version > > now since it haven't been updated for a while. > > > > Comments and directions are welcomed. :) > > > > Thanks, > > Gao Xiang > > > > Gao Xiang (3): > > xfs: get rid of unused pagi_unlinked_hash > > xfs: introduce perag iunlink lock > > xfs: insert unlinked inodes from tail > > > > fs/xfs/xfs_inode.c | 194 ++++++++++++++++++++++++++++++++------ > > fs/xfs/xfs_inode.h | 1 + > > fs/xfs/xfs_iunlink_item.c | 16 ++++ > > fs/xfs/xfs_mount.c | 4 + > > fs/xfs/xfs_mount.h | 14 +-- > > 5 files changed, 193 insertions(+), 36 deletions(-) > > > > -- > > 2.18.1 > > >
Hm. I saw the following warning from lockdep when running generic/078 with:
MKFS_OPTIONS -- -m reflink=1,rmapbt=1 -i sparse=1
MOUNT_OPTIONS -- -o usrquota,grpquota,prjquota,
CHECK_OPTIONS -- -g auto
XFS_MKFS_OPTIONS -- -bsize=4096
The test kernel is 5.9-rc1 + inobtcount + y2038 + dave's iunlink series + yours
+[ 516.166575] run fstests generic/078 at 2020-08-19 15:35:28
+[ 516.659584] XFS (sda): Mounting V5 Filesystem
+[ 516.846982] XFS (sda): Ending clean mount
+[ 516.849669] xfs filesystem being mounted at /mnt supports timestamps until 2038 (0x7fffffff)
+
+[ 517.341920] ============================================
+[ 517.342849] WARNING: possible recursive locking detected
+[ 517.343832] 5.9.0-rc1-djw #rc1 Tainted: G W
+[ 517.344862] --------------------------------------------
+[ 517.345886] renameat2/107505 is trying to acquire lock:
+[ 517.346830] ffff88803ca48468 (&pag->pag_iunlink_mutex){+.+.}-{3:3}, at: xfs_iunlink+0xb8/0x3f0 [xfs]
+[ 517.348460]
+ but task is already holding lock:
+[ 517.349107] ffff88803ca4bc68 (&pag->pag_iunlink_mutex){+.+.}-{3:3}, at: xfs_iunlink_remove+0x239/0x3f0 [xfs]
+[ 517.350240]
+ other info that might help us debug this:
+[ 517.351166] Possible unsafe locking scenario:
+
+[ 517.351902] CPU0
+[ 517.352399] ----
+[ 517.352895] lock(&pag->pag_iunlink_mutex);
+[ 517.353572] lock(&pag->pag_iunlink_mutex);
+[ 517.354387]
+ *** DEADLOCK ***
+
+[ 517.355394] May be due to missing lock nesting notation
+
+[ 517.356132] 9 locks held by renameat2/107505:
+[ 517.356615] #0: ffff888032b78468 (sb_writers#12){.+.+}-{0:0}, at: mnt_want_write+0x24/0x60
+[ 517.357561] #1: ffff88803d00bfe8 (&inode->i_sb->s_type->i_mutex_dir_key/1){+.+.}-{3:3}, at: lock_rename+0xf5/0x100
+[ 517.358912] #2: ffff88803d00fbe8 (&inode->i_sb->s_type->i_mutex_dir_key){++++}-{3:3}, at: vfs_rename+0x17d/0x950
+[ 517.360011] #3: ffff888032b78688 (sb_internal){.+.+}-{0:0}, at: xfs_trans_alloc+0x18b/0x250 [xfs]
+[ 517.361018] #4: ffff88803d00f8f0 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_ilock+0xcf/0x2a0 [xfs]
+[ 517.362063] #5: ffff88803d00bcf0 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_ilock_nowait+0xcf/0x340 [xfs]
+[ 517.363173] #6: ffff88803bf82df0 (&xfs_nondir_ilock_class){++++}-{3:3}, at: xfs_ilock_nowait+0xcf/0x340 [xfs]
+[ 517.364685] #7: ffff88803d00cbf0 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_ilock_nowait+0xcf/0x340 [xfs]
+[ 517.366414] #8: ffff88803ca4bc68 (&pag->pag_iunlink_mutex){+.+.}-{3:3}, at: xfs_iunlink_remove+0x239/0x3f0 [xfs]
+[ 517.367787]
+ stack backtrace:
+[ 517.368285] CPU: 1 PID: 107505 Comm: renameat2 Tainted: G W 5.9.0-rc1-djw #rc1
+[ 517.369196] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1 04/01/2014
+[ 517.370401] Call Trace:
+[ 517.370703] dump_stack+0x7c/0xac
+[ 517.371091] __lock_acquire.cold+0x168/0x2ab
+[ 517.371571] lock_acquire+0xa2/0x370
+[ 517.372025] ? xfs_iunlink+0xb8/0x3f0 [xfs]
+[ 517.372505] __mutex_lock+0xa1/0xa00
+[ 517.372955] ? xfs_iunlink+0xb8/0x3f0 [xfs]
+[ 517.373455] ? kvm_clock_read+0x14/0x30
+[ 517.373898] ? kvm_sched_clock_read+0x9/0x20
+[ 517.374379] ? sched_clock_cpu+0x14/0xe0
+[ 517.374897] ? xfs_iunlink+0xb8/0x3f0 [xfs]
+[ 517.375405] ? xfs_iunlink+0x94/0x3f0 [xfs]
+[ 517.376185] ? rcu_read_lock_sched_held+0x56/0x80
+[ 517.377055] ? xfs_iunlink+0xb8/0x3f0 [xfs]
+[ 517.377896] xfs_iunlink+0xb8/0x3f0 [xfs]
+[ 517.378703] xfs_rename+0xdb8/0x1030 [xfs]
+[ 517.379261] xfs_vn_rename+0xd5/0x140 [xfs]
+[ 517.379740] vfs_rename+0x1bc/0x950
+[ 517.380146] ? lookup_dcache+0x18/0x60
+[ 517.380572] ? do_renameat2+0x343/0x4d0
+[ 517.381013] do_renameat2+0x343/0x4d0
+[ 517.381672] __x64_sys_renameat2+0x25/0x30
+[ 517.382145] do_syscall_64+0x31/0x40
+[ 517.382554] entry_SYSCALL_64_after_hwframe+0x44/0xa9
+[ 517.383330] RIP: 0033:0x7fb9be244083
+[ 517.384005] Code: 64 89 02 b8 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 f3 0f 1e fa 49 89 ca 45 85 c0 74 44 b8 3c 01 00 00 0f 05 <48> 3d 00 f0 ff ff 77 3d 41 89 c0 83 f8 ff 74 0d 44 89 c0 c3 66 0f
+[ 517.387257] RSP: 002b:00007ffc56416ed8 EFLAGS: 00000202 ORIG_RAX: 000000000000013c
+[ 517.388148] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fb9be244083
+[ 517.389183] RDX: 00000000ffffff9c RSI: 00007ffc5641968c RDI: 00000000ffffff9c
+[ 517.390190] RBP: 0000000000000000 R08: 0000000000000004 R09: 00007ffc56416fd8
+[ 517.391440] R10: 00007ffc5641969c R11: 0000000000000202 R12: 000056494b1d6220
+[ 517.392747] R13: 00007ffc56416fd0 R14: 0000000000000000 R15: 0000000000000000
+[ 517.831548] XFS (sda): Unmounting Filesystem
+[ 518.026539] XFS (sda): Mounting V5 Filesystem
+[ 518.158574] XFS (sda): Ending clean mount
+[ 518.160484] xfs filesystem being mounted at /mnt supports timestamps until 2038 (0x7fffffff)
Dunno what this is about, but I'll have a look in the morning...
--D
Hi Darrick, On Wed, Aug 19, 2020 at 07:46:18PM -0700, Darrick J. Wong wrote: > Hm. I saw the following warning from lockdep when running generic/078 with: > > MKFS_OPTIONS -- -m reflink=1,rmapbt=1 -i sparse=1 > MOUNT_OPTIONS -- -o usrquota,grpquota,prjquota, > CHECK_OPTIONS -- -g auto > XFS_MKFS_OPTIONS -- -bsize=4096 > > The test kernel is 5.9-rc1 + inobtcount + y2038 + dave's iunlink series + yours Thanks for the report. I didn't enable lockdep before, will confirm it soon. Thanks! Gao Xiang > > +[ 516.166575] run fstests generic/078 at 2020-08-19 15:35:28 > +[ 516.659584] XFS (sda): Mounting V5 Filesystem > +[ 516.846982] XFS (sda): Ending clean mount > +[ 516.849669] xfs filesystem being mounted at /mnt supports timestamps until 2038 (0x7fffffff) > + > +[ 517.341920] ============================================ > +[ 517.342849] WARNING: possible recursive locking detected > +[ 517.343832] 5.9.0-rc1-djw #rc1 Tainted: G W > +[ 517.344862] -------------------------------------------- > +[ 517.345886] renameat2/107505 is trying to acquire lock: > +[ 517.346830] ffff88803ca48468 (&pag->pag_iunlink_mutex){+.+.}-{3:3}, at: xfs_iunlink+0xb8/0x3f0 [xfs] > +[ 517.348460] > + but task is already holding lock: > +[ 517.349107] ffff88803ca4bc68 (&pag->pag_iunlink_mutex){+.+.}-{3:3}, at: xfs_iunlink_remove+0x239/0x3f0 [xfs] > +[ 517.350240] > + other info that might help us debug this: > +[ 517.351166] Possible unsafe locking scenario: > + > +[ 517.351902] CPU0 > +[ 517.352399] ---- > +[ 517.352895] lock(&pag->pag_iunlink_mutex); > +[ 517.353572] lock(&pag->pag_iunlink_mutex); > +[ 517.354387] > + *** DEADLOCK *** > + > +[ 517.355394] May be due to missing lock nesting notation > + > +[ 517.356132] 9 locks held by renameat2/107505: > +[ 517.356615] #0: ffff888032b78468 (sb_writers#12){.+.+}-{0:0}, at: mnt_want_write+0x24/0x60 > +[ 517.357561] #1: ffff88803d00bfe8 (&inode->i_sb->s_type->i_mutex_dir_key/1){+.+.}-{3:3}, at: lock_rename+0xf5/0x100 > +[ 517.358912] #2: ffff88803d00fbe8 (&inode->i_sb->s_type->i_mutex_dir_key){++++}-{3:3}, at: vfs_rename+0x17d/0x950 > +[ 517.360011] #3: ffff888032b78688 (sb_internal){.+.+}-{0:0}, at: xfs_trans_alloc+0x18b/0x250 [xfs] > +[ 517.361018] #4: ffff88803d00f8f0 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_ilock+0xcf/0x2a0 [xfs] > +[ 517.362063] #5: ffff88803d00bcf0 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_ilock_nowait+0xcf/0x340 [xfs] > +[ 517.363173] #6: ffff88803bf82df0 (&xfs_nondir_ilock_class){++++}-{3:3}, at: xfs_ilock_nowait+0xcf/0x340 [xfs] > +[ 517.364685] #7: ffff88803d00cbf0 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_ilock_nowait+0xcf/0x340 [xfs] > +[ 517.366414] #8: ffff88803ca4bc68 (&pag->pag_iunlink_mutex){+.+.}-{3:3}, at: xfs_iunlink_remove+0x239/0x3f0 [xfs] > +[ 517.367787] > + stack backtrace: > +[ 517.368285] CPU: 1 PID: 107505 Comm: renameat2 Tainted: G W 5.9.0-rc1-djw #rc1 > +[ 517.369196] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1 04/01/2014 > +[ 517.370401] Call Trace: > +[ 517.370703] dump_stack+0x7c/0xac > +[ 517.371091] __lock_acquire.cold+0x168/0x2ab > +[ 517.371571] lock_acquire+0xa2/0x370 > +[ 517.372025] ? xfs_iunlink+0xb8/0x3f0 [xfs] > +[ 517.372505] __mutex_lock+0xa1/0xa00 > +[ 517.372955] ? xfs_iunlink+0xb8/0x3f0 [xfs] > +[ 517.373455] ? kvm_clock_read+0x14/0x30 > +[ 517.373898] ? kvm_sched_clock_read+0x9/0x20 > +[ 517.374379] ? sched_clock_cpu+0x14/0xe0 > +[ 517.374897] ? xfs_iunlink+0xb8/0x3f0 [xfs] > +[ 517.375405] ? xfs_iunlink+0x94/0x3f0 [xfs] > +[ 517.376185] ? rcu_read_lock_sched_held+0x56/0x80 > +[ 517.377055] ? xfs_iunlink+0xb8/0x3f0 [xfs] > +[ 517.377896] xfs_iunlink+0xb8/0x3f0 [xfs] > +[ 517.378703] xfs_rename+0xdb8/0x1030 [xfs] > +[ 517.379261] xfs_vn_rename+0xd5/0x140 [xfs] > +[ 517.379740] vfs_rename+0x1bc/0x950 > +[ 517.380146] ? lookup_dcache+0x18/0x60 > +[ 517.380572] ? do_renameat2+0x343/0x4d0 > +[ 517.381013] do_renameat2+0x343/0x4d0 > +[ 517.381672] __x64_sys_renameat2+0x25/0x30 > +[ 517.382145] do_syscall_64+0x31/0x40 > +[ 517.382554] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > +[ 517.383330] RIP: 0033:0x7fb9be244083 > +[ 517.384005] Code: 64 89 02 b8 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 f3 0f 1e fa 49 89 ca 45 85 c0 74 44 b8 3c 01 00 00 0f 05 <48> 3d 00 f0 ff ff 77 3d 41 89 c0 83 f8 ff 74 0d 44 89 c0 c3 66 0f > +[ 517.387257] RSP: 002b:00007ffc56416ed8 EFLAGS: 00000202 ORIG_RAX: 000000000000013c > +[ 517.388148] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fb9be244083 > +[ 517.389183] RDX: 00000000ffffff9c RSI: 00007ffc5641968c RDI: 00000000ffffff9c > +[ 517.390190] RBP: 0000000000000000 R08: 0000000000000004 R09: 00007ffc56416fd8 > +[ 517.391440] R10: 00007ffc5641969c R11: 0000000000000202 R12: 000056494b1d6220 > +[ 517.392747] R13: 00007ffc56416fd0 R14: 0000000000000000 R15: 0000000000000000 > +[ 517.831548] XFS (sda): Unmounting Filesystem > +[ 518.026539] XFS (sda): Mounting V5 Filesystem > +[ 518.158574] XFS (sda): Ending clean mount > +[ 518.160484] xfs filesystem being mounted at /mnt supports timestamps until 2038 (0x7fffffff) > > Dunno what this is about, but I'll have a look in the morning... > > --D >