Re: [PATCH 3/7] xfs: Compute maximum height of directory BMBT separately

[Chandan Babu R <chandanrlinux@gmail.com>]

On Wednesday 10 June 2020 12:10:02 AM IST Darrick J. Wong wrote:
> On Tue, Jun 09, 2020 at 07:53:55PM +0530, Chandan Babu R wrote:
> > On Tuesday 9 June 2020 2:29:22 AM IST Darrick J. Wong wrote:
> > > On Sat, Jun 06, 2020 at 01:57:41PM +0530, Chandan Babu R wrote:
> > > > xfs/306 causes the following call trace when using a data fork with a
> > > > maximum extent count of 2^47,
> > > > 
> > > >  XFS (loop0): Mounting V5 Filesystem
> > > >  XFS (loop0): Log size 8906 blocks too small, minimum size is 9075 blocks
> > > >  XFS (loop0): AAIEEE! Log failed size checks. Abort!
> > > >  XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 711
> > > 
> > > Uh... won't applying the corresponding MAXEXTNUM changes and whatnot to
> > > xfsprogs result in mkfs formatting a log with 9075 blocks?  Is there
> > > some other mistake in the minimum log size computations?
> > 
> > The call trace given below shows up when using 2^47 as the maximum extent
> > count for both Dir and Non-dir inodes.
> > 
> > However, using 2^27 as the maximum
> > extent count for directories would reduce the log reservation value for
> > "rename" operation (which has the maximum sized log reservation when using the
> > below mentioned FS geometry).
> > 
> > "Rename" log reservation is a function of the maximum directory BMBT height
> > which in turn is a function of the maximum number of extents that can be
> > occupied by a directory.
> > 
> > Hence when moving the MAXEXTNUM changes to xfsprogs, the corresponding
> > "maximum directory extent count" changes must also be moved as a
> > dependency.
> > 
> > With this patchset applied (i.e. With 2^27 as the maximum extent count for
> > directory inodes and 2^47 as the maximum extent count for non-directory
> > inodes), xfs_log_calc_minimum_size() in kernel returns 8691 blocks.
> 
> Hmm, 8691, you say?  Ok, that's a helpful clue...
> 
> MAXEXTNUM	min log blocks
> 2^47		9,075
> 2^32		8,906
> 2^27		8,691
> 
> ...and now I think I finally understand the goal here.  The existing
> xfs_bmap_compute_maxlevels computes the max bmbt height from MAXEXTNUM
> (2^32).  The file rename reservation computation uses this max bmbt
> height, which works out to a min log size of 8,906 blocks.  Once you
> change MAXEXTNUM to 2^47, this computation turns into 9,075 blocks.
> 
> This means that if you use mkfs.xfs 5.6.0 to create a small, vanilla V5
> filesystem, it won't mount on your development kernel due to the minimum
> log size checks, even if you didn't enable the larger extent counters.
> 
> Therefore, you're introducing m_bm_dir_maxlevel to store the max bmbt
> height for a directory, using that to compute the rename reservation,
> and lo and behold the min log size never goes above the old limit.
> 
> This is problematic... (scroll down, please)
> 
> > > 
> > > >  ------------[ cut here ]------------
> > > >  WARNING: CPU: 0 PID: 12821 at fs/xfs/xfs_message.c:112 assfail+0x25/0x28
> > > >  Modules linked in:
> > > >  CPU: 0 PID: 12821 Comm: mount Tainted: G        W         5.6.0-rc6-next-20200320-chandan-00003-g071c2af3f4de #1
> > > >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> > > >  RIP: 0010:assfail+0x25/0x28
> > > >  Code: ff ff 0f 0b c3 0f 1f 44 00 00 41 89 c8 48 89 d1 48 89 f2 48 c7 c6 40 b7 4b b3 e8 82 f9 ff ff 80 3d 83 d6 64 01 00 74 02 0f $
> > > >  RSP: 0018:ffffb05b414cbd78 EFLAGS: 00010246
> > > >  RAX: 0000000000000000 RBX: ffff9d9d501d5000 RCX: 0000000000000000
> > > >  RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffffb346dc65
> > > >  RBP: ffff9da444b49a80 R08: 0000000000000000 R09: 0000000000000000
> > > >  R10: 000000000000000a R11: f000000000000000 R12: 00000000ffffffea
> > > >  R13: 000000000000000e R14: 0000000000004594 R15: ffff9d9d501d5628
> > > >  FS:  00007fd6c5d17c80(0000) GS:ffff9da44d800000(0000) knlGS:0000000000000000
> > > >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > >  CR2: 0000000000000002 CR3: 00000008a48c0000 CR4: 00000000000006f0
> > > >  Call Trace:
> > > >   xfs_log_mount+0xf8/0x300
> > > >   xfs_mountfs+0x46e/0x950
> > > >   xfs_fc_fill_super+0x318/0x510
> > > >   ? xfs_mount_free+0x30/0x30
> > > >   get_tree_bdev+0x15c/0x250
> > > >   vfs_get_tree+0x25/0xb0
> > > >   do_mount+0x740/0x9b0
> > > >   ? memdup_user+0x41/0x80
> > > >   __x64_sys_mount+0x8e/0xd0
> > > >   do_syscall_64+0x48/0x110
> > > >   entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > >  RIP: 0033:0x7fd6c5f2ccda
> > > >  Code: 48 8b 0d b9 e1 0b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f $
> > > >  RSP: 002b:00007ffe00dfb9f8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
> > > >  RAX: ffffffffffffffda RBX: 0000560c1aaa92c0 RCX: 00007fd6c5f2ccda
> > > >  RDX: 0000560c1aaae110 RSI: 0000560c1aaad040 RDI: 0000560c1aaa94d0
> > > >  RBP: 00007fd6c607d204 R08: 0000000000000000 R09: 0000560c1aaadde0
> > > >  R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> > > >  R13: 0000000000000000 R14: 0000560c1aaa94d0 R15: 0000560c1aaae110
> > > >  ---[ end trace 6436391b468bc652 ]---
> > > >  XFS (loop0): log mount failed
> > > > 
> > > > The corresponding filesystem was created using mkfs options
> > > > "-m rmapbt=1,reflink=1 -b size=1k -d size=20m -n size=64k".
> > > > 
> > > > i.e. We have a filesystem of size 20MiB, data block size of 1KiB and
> > > > directory block size of 64KiB. Filesystems of size < 1GiB can have less
> > > > than 10MiB on-disk log (Please refer to calculate_log_size() in
> > > > xfsprogs).
> > > 
> > > Hm.  You don't seem to be setting either of the big extent count feature
> > > flags here.
> > > 
> > > Is this something that happens after a filesystem gets *upgraded* to
> > > support extent counts > 2^32?  If it's this second case, then I think
> > > the function that upgrades the filesystem has to reject the change if it
> > > would cause the minimum log size checks to fail.
> > 
> > This happens when having 2^47 as the value of MAXEXTNUM irrespective of
> > whether the filesystem's superblock has the big extent count feature flag set
> > i.e. this patchset
> > 
> > Using 2^47 as the value of MAXEXTNUM causes the height of the data fork BMBT
> > tree to increase when compared to the height of the tree when using 2^32
> > MAXEXTNUM (In the case of the fs geometry that caused the above call trace,
> > the height increased by 1). The call xfs_bmap_compute_maxlevels(mp,
> > XFS_DATA_FORK) (invoked as part of FS mount operation) uses MAXEXTNUM as input
> > to calculate the maximum height of the data fork BMBT and the result is stored
> > in mp->m_bm_maxlevels[XFS_DATA_FORK]. This value is then used when calculating
> > log reservations for various fs operations. Hence the log reservations of fs
> > operations now change regardless of whether the "big extent count" feature
> > flag is set or not.
> 
> "...or not."
> 
> Urrrk, no.  The log reservation calculations for existing filesystems
> must not change, because (at best) this will cause subtle log behavior
> changes due to the fluctuating reservation sizes; and (at worst) it can
> cause the same log minimum size mounting problems you observed above.
> 
> If you disturb the log reservations for existing filesystems such
> that the minimum log size goes up, this means that small filesystems
> created with an old mkfs will now fail to mount with the new kernel.
> This is never acceptable.
> 
> If you disturb the log reservations such that the minimum log size goes
> down, this means that when those changes get pulled in by the xfsprogs
> maintainer, a new mkfs will produce small filesystems that won't mount
> on older kernels.  The only way this is acceptable is if the changes
> only affect filesystems with a feature flag set that would cause all
> of those older kernels to warn about the feature being EXPERIMENTAL.

So the reduction of the rename log reservation size (by using 2^32 as the
maximum directory extent count) must be accompanied with setting of a feature
flag other than bigfork feature flag right? I say that because the bigfork
feature flag is currently set at runtime when we are about to overflow signed
16-bit attrs or signed 32-bit data extent counters. Log reservation values are
pre-calculated during filesystem mount and cannot be changed during runtime.

This also means that the patch "xfs: Fix log reservation calculation for xattr
insert operation" also needs to be handled specially since it,
- Replaces two reservations (mount and runtime) with just one static
  reservation.
- Reduces the value of "xattr set operation" reservation.
Hence older kernels may not be able to mount filesystems created with mkfs.xfs
containing this patch.

> 
> Either way, users end up broken.
> 
> > > 
> > > Granted, I don't understand the need (in the next patch) to special case
> > > bmbt maxlevels for directory data forks.  That's probably muddying up
> > > my ability to figure all this out.  Yes I did read this series
> > > backwards. :)
> > 
> > Using a separate maximum extent count for directory data fork was required to
> > reduce the increased log reservations described above. To be precise, rename
> > operation invokes XFS_DIR_OP_LOG_COUNT() which indirectly uses
> > mp->m_bm_maxlevels[XFS_DATA_FORK] for its calculations. When using a modified
> > kernel which had 2^47 as the value for MAXEXTNUM resulted in a taller data
> > fork BMBT tree. Hence log reservation space for rename operation became larger.
> > 
> > The idea of special handling of "maximum extents for directory data fork" came
> > up later when trying to find a way to reduce the log reservation for the
> > rename operation.
> 
> I think a better way to handle the directory operation reservations is:
> 
> 1. Introduce XFS_MAXDIREXTNUM == 2^32-1, and use that to compute
>    m_bm_dir_maxlevel for directories.
> 
> 2. Use m_bm_dir_maxlevel to compute the rename reservations, like you do
>    here.
> 
> 3. As a cleanup, split XFS_NEXTENTADD_SPACE_RES into three separate
>    helpers: one for attr forks (a), one for regular file data forks (b),
>    and one for !S_ISREG() data forks(c).  The DAENTER macros can switch
>    between (a) and (c).  Anything that knows it's being run against a
>    regular file can use (b).  Symlinks and rtbitmaps can use (c).
> 
>    We then add a separate helper taking an xfs_inode and whichfork to
>    compute the correct value for the the callers that have non-variable
>    arguments.

Ok. I will take a shot at implementing the helpers. Thanks for the
suggestions.

> 
> This means that the log reservations will stay the same, regardless of
> whether the bigfork feature is enabled.

On a filesystem which would have already used,
1. 2^47 max data extent count
2. 2^28 max directory extent count
3. 2^32 max xattr count
for computing the log reservations during mount time, an upgrade to bigfork
feature would not affect the pre-calculated log reservation values.

> I think this will be safe for
> the attr extent count expansion, since we aren't letting the attr fork
> expand beyond 2^32 extents, which means the max bmbt height there will
> never be larger than anything we've ever seen before.
> 
> In my head I've convinced myself that this will keep the code simpler in
> the long run, but maybe the rest of you have other ideas or flames? :D
> 

-- 
chandan