- 27 May, 2020 3 commits
-
-
Dave Chinner authored
Seeing massive cpu usage from xfs_agino_range() on one machine; instruction level profiles look similar to another machine running the same workload, only one machine is consuming 10x as much CPU as the other and going much slower. The only real difference between the two machines is core count per socket. Both are running identical 16p/16GB virtual machine configurations Machine A: 25.83% [k] xfs_agino_range 12.68% [k] __xfs_dir3_data_check 6.95% [k] xfs_verify_ino 6.78% [k] xfs_dir2_data_entry_tag_p 3.56% [k] xfs_buf_find 2.31% [k] xfs_verify_dir_ino 2.02% [k] xfs_dabuf_map.constprop.0 1.65% [k] xfs_ag_block_count And takes around 13 minutes to remove 50 million inodes. Machine B: 13.90% [k] __pv_queued_spin_lock_slowpath 3.76% [k] do_raw_spin_lock 2.83% [k] xfs_dir3_leaf_check_int 2.75% [k] xfs_agino_range 2.51% [k] __raw_callee_save___pv_queued_spin_unlock 2.18% [k] __xfs_dir3_data_check 2.02% [k] xfs_log_commit_cil And takes around 5m30s to remove 50 million inodes. Suspect is cacheline contention on m_sectbb_log which is used in one of the macros in xfs_agino_range. This is a read-only variable but shares a cacheline with m_active_trans which is a global atomic that gets bounced all around the machine. The workload is trying to run hundreds of thousands of transactions per second and hence cacheline contention will be occurring on this atomic counter. Hence xfs_agino_range() is likely just be an innocent bystander as the cache coherency protocol fights over the cacheline between CPU cores and sockets. On machine A, this rearrangement of the struct xfs_mount results in the profile changing to: 9.77% [kernel] [k] xfs_agino_range 6.27% [kernel] [k] __xfs_dir3_data_check 5.31% [kernel] [k] __pv_queued_spin_lock_slowpath 4.54% [kernel] [k] xfs_buf_find 3.79% [kernel] [k] do_raw_spin_lock 3.39% [kernel] [k] xfs_verify_ino 2.73% [kernel] [k] __raw_callee_save___pv_queued_spin_unlock Vastly less CPU usage in xfs_agino_range(), but still 3x the amount of machine B and still runs substantially slower than it should. Current rm -rf of 50 million files: vanilla patched machine A 13m20s 6m42s machine B 5m30s 5m02s It's an improvement, hence indicating that separation and further optimisation of read-only global filesystem data is worthwhile, but it clearly isn't the underlying issue causing this specific performance degradation. Signed-off-by:
Dave Chinner <dchinner@redhat.com> Reviewed-by:
Christoph Hellwig <hch@lst.de> Reviewed-by:
Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by:
-
Dave Chinner authored
Shaokun Zhang reported that XFS was using substantial CPU time in percpu_count_sum() when running a single threaded benchmark on a high CPU count (128p) machine from xfs_mod_ifree(). The issue is that the filesystem is empty when the benchmark runs, so inode allocation is running with a very low inode free count. With the percpu counter batching, this means comparisons when the counter is less that 128 * 256 = 32768 use the slow path of adding up all the counters across the CPUs, and this is expensive on high CPU count machines. The summing in xfs_mod_ifree() is only used to fire an assert if an underrun occurs. The error is ignored by the higher level code. Hence this is really just debug code and we don't need to run it on production kernels, nor do we need such debug checks to return error values just to trigger an assert. Finally, xfs_mod_icount/xfs_mod_ifree are only called from xfs_trans_unreserve_and_mod_sb(), so get rid of them and just directly call the percpu_counter_add/percpu_counter_compare functions. The compare functions are now run only on debug builds as they are internal to ASSERT() checks and so only compiled in when ASSERTs are active (CONFIG_XFS_DEBUG=y or CONFIG_XFS_WARN=y). Reported-by:
Shaokun Zhang <zhangshaokun@hisilicon.com> Signed-off-by:
-
Dave Chinner authored
xfs: gut error handling in xfs_trans_unreserve_and_mod_sb() From: Dave Chinner <dchinner@redhat.com> The error handling in xfs_trans_unreserve_and_mod_sb() is largely incorrect - rolling back the changes in the transaction if only one counter underruns makes all the other counters incorrect. We still allow the change to proceed and committing the transaction, except now we have multiple incorrect counters instead of a single underflow. Further, we don't actually report the error to the caller, so this is completely silent except on debug kernels that will assert on failure before we even get to the rollback code. Hence this error handling is broken, untested, and largely unnecessary complexity. Just remove it. Signed-off-by:
-
- 19 May, 2020 22 commits
-
-
Christoph Hellwig authored
Move freeing the dynamically allocated attr and COW fork, as well as zeroing the pointers where actually needed into the callers, and just pass the xfs_ifork structure to xfs_idestroy_fork. Also simplify the kmem_free calls by not checking for NULL first. Signed-off-by:
Chandan Babu R <chandanrlinux@gmail.com> Reviewed-by:
Brian Foster <bfoster@redhat.com> Reviewed-by:
-
Christoph Hellwig authored
Both the data and attr fork have a format that is stored in the legacy idinode. Move it into the xfs_ifork structure instead, where it uses up padding. Signed-off-by:
-
Christoph Hellwig authored
There are there are three extents counters per inode, one for each of the forks. Two are in the legacy icdinode and one is directly in struct xfs_inode. Switch to a single counter in the xfs_ifork structure where it uses up padding at the end of the structure. This simplifies various bits of code that just wants the number of extents counter and can now directly dereference it. Signed-off-by:
-
Christoph Hellwig authored
xfs_ifree only need to free inline data in the data fork, as we've already taken care of the attr fork before (and in fact freed the fork structure). Just open code the freeing of the inline data. Signed-off-by:
-
Christoph Hellwig authored
Just checking di_forkoff directly is a little easier to follow. Signed-off-by:
-
Darrick J. Wong authored
XFS_IFORK_Q is supposed to be a predicate, not a function returning a value. Its usage is in xchk_bmap_check_rmaps is incorrect, but that function only cares about whether or not the "size" of the data is zero or not. Convert that logic to use a proper boolean, and teach the caller to skip the call entirely if the end result would be that we'd do nothing anyway. This avoids a crash later in this series. Signed-off-by:
-
Christoph Hellwig authored
Now that we fully verify the inode forks before they are added to the inode cache, the crash reported in https://bugzilla.kernel.org/show_bug.cgi?id=204031 can't happen anymore, as we'll never let an inode that has inconsistent nextents counts vs the presence of an in-core attr fork leak into the inactivate code path. So remove the work around to try to handle the case, and just return an error and warn if the fork is not present. Signed-off-by:
-
Christoph Hellwig authored
We don't call xfs_bmapi_read for the COW fork anymore, so remove the special casing. Signed-off-by:
-
Christoph Hellwig authored
Call the data/attr local fork verifiers as soon as we are ready for them. This keeps them close to the code setting up the forks, and avoids a few branches later on. Also open code xfs_inode_verify_forks in the only remaining caller. Signed-off-by:
-
Christoph Hellwig authored
The split between xfs_inode_verify_forks and the two helpers implementing the actual functionality is a little strange. Reshuffle it so that xfs_inode_verify_forks verifies if the data and attr forks are actually in local format and only call the low-level helpers if that is the case. Handle the actual error reporting in the low-level handlers to streamline the caller. Signed-off-by:
-
Christoph Hellwig authored
xfs_ifork_ops add up to two indirect calls per inode read and flush, despite just having a single instance in the kernel. In xfsprogs phase6 in xfs_repair overrides the verify_dir method to deal with inodes that do not have a valid parent, but that can be fixed pretty easily by ensuring they always have a valid looking parent. Signed-off-by:
-
Christoph Hellwig authored
There is not much point in the xfs_iread function, as it has a single caller and not a whole lot of code. Move it into the only caller, and trim down the overdocumentation to just documenting the important "why" instead of a lot of redundant "what". Signed-off-by:
-
Christoph Hellwig authored
i_delayed_blks is set to 0 in xfs_inode_alloc and can't have anything assigned to it until the inode is visible to the VFS. Signed-off-by:
-
Christoph Hellwig authored
Keep the code dealing with the dinode together, and also ensure we verify the dinode in the owner change log recovery case as well. Signed-off-by:
-
Christoph Hellwig authored
Handle inodes with a 0 di_mode in xfs_inode_from_disk, instead of partially duplicating inode reading in xfs_iread. Signed-off-by:
-
Christoph Hellwig authored
xfs_iformat_fork is a weird catchall. Split it into one helper for the data fork and one for the attr fork, and then call both helper as well as the COW fork initialization from xfs_inode_from_disk. Order the COW fork initialization after the attr fork initialization given that it can't fail to simplify the error handling. Note that the newly split helpers are moved down the file in xfs_inode_fork.c to avoid the need for forward declarations. Signed-off-by:
-
Christoph Hellwig authored
We always need to fill out the fork structures when reading the inode, so call xfs_iformat_fork from the tail of xfs_inode_from_disk. Signed-off-by:
-
Christoph Hellwig authored
The last argument to xfs_bmapi_raad contains XFS_BMAPI_* flags, not the fork. Given that XFS_DATA_FORK evaluates to 0 no real harm is done, but let's fix this anyway. Signed-off-by:
-
Kaixu Xia authored
Fix this error message to complain about project and group quota flag bits instead of "PUOTA" and "QUOTA". Signed-off-by:
Kaixu Xia <kaixuxia@tencent.com> Reviewed-by:
-
Darrick J. Wong authored
Since the old SWAPEXT ioctl doesn't know how to adjust quota ids, bail out of the ids don't match and quotas are enabled. Signed-off-by:
Eric Sandeen <sandeen@redhat.com> Reviewed-by:
-
Darrick J. Wong authored
While QAing the new xfs_repair quotacheck code, I uncovered a quota corruption bug resulting from a bad interaction between dquot buffer initialization and quotacheck. The bug can be reproduced with the following sequence: # mkfs.xfs -f /dev/sdf # mount /dev/sdf /opt -o usrquota # su nobody -s /bin/bash -c 'touch /opt/barf' # sync # xfs_quota -x -c 'report -ahi' /opt User quota on /opt (/dev/sdf) Inodes User ID Used Soft Hard Warn/Grace ---------- --------------------------------- root 3 0 0 00 [------] nobody 1 0 0 00 [------] # xfs_io -x -c 'shutdown' /opt # umount /opt # mount /dev/sdf /opt -o usrquota # touch /opt/man2 # xfs_quota -x -c 'report -ahi' /opt User quota on /opt (/dev/sdf) Inodes User ID Used Soft Hard Warn/Grace ---------- --------------------------------- root 1 0 0 00 [------] nobody 1 0 0 00 [------] # umount /opt Notice how the initial quotacheck set the root dquot icount to 3 (rootino, rbmino, rsumino), but after shutdown -> remount -> recovery, xfs_quota reports that the root dquot has only 1 icount. We haven't deleted anything from the filesystem, which means that quota is now under-counting. This behavior is not limited to icount or the root dquot, but this is the shortest reproducer. I traced the cause of this discrepancy to the way that we handle ondisk dquot updates during quotacheck vs. regular fs activity. Normally, when we allocate a disk block for a dquot, we log the buffer as a regular (dquot) buffer. Subsequent updates to the dquots backed by that block are done via separate dquot log item updates, which means that they depend on the logged buffer update being written to disk before the dquot items. Because individual dquots have their own LSN fields, that initial dquot buffer must always be recovered. However, the story changes for quotacheck, which can cause dquot block allocations but persists the final dquot counter values via a delwri list. Because recovery doesn't gate dquot buffer replay on an LSN, this means that the initial dquot buffer can be replayed over the (newer) contents that were delwritten at the end of quotacheck. In effect, this re-initializes the dquot counters after they've been updated. If the log does not contain any other dquot items to recover, the obsolete dquot contents will not be corrected by log recovery. Because quotacheck uses a transaction to log the setting of the CHKD flags in the superblock, we skip quotacheck during the second mount call, which allows the incorrect icount to remain. Fix this by changing the ondisk dquot initialization function to use ordered buffers to write out fresh dquot blocks if it detects that we're running quotacheck. If the system goes down before quotacheck can complete, the CHKD flags will not be set in the superblock and the next mount will run quotacheck again, which can fix uninitialized dquot buffers. This requires amending the defer code to maintaine ordered buffer state across defer rolls for the sake of the dquot allocation code. For regular operations we preserve the current behavior since the dquot items require properly initialized ondisk dquot records. Signed-off-by: -
Brian Foster authored
The attr fork can transition from shortform to leaf format while empty if the first xattr doesn't fit in shortform. While this empty leaf block state is intended to be transient, it is technically not due to the transactional implementation of the xattr set operation. We historically have a couple of bandaids to work around this problem. The first is to hold the buffer after the format conversion to prevent premature writeback of the empty leaf buffer and the second is to bypass the xattr count check in the verifier during recovery. The latter assumes that the xattr set is also in the log and will be recovered into the buffer soon after the empty leaf buffer is reconstructed. This is not guaranteed, however. If the filesystem crashes after the format conversion but before the xattr set that induced it, only the format conversion may exist in the log. When recovered, this creates a latent corrupted state on the inode as any subsequent attempts to read the buffer fail due to verifier failure. This includes further attempts to set xattrs on the inode or attempts to destroy the attr fork, which prevents the inode from ever being removed from the unlinked list. To avoid this condition, accept that an empty attr leaf block is a valid state and remove the count check from the verifier. This means that on rare occasions an attr fork might exist in an unexpected state, but is otherwise consistent and functional. Note that we retain the logic to avoid racing with metadata writeback to reduce the window where this can occur. Signed-off-by:
-
- 13 May, 2020 3 commits
-
-
Nishad Kamdar authored
This patch corrects the SPDX License Identifier style in header files related to XFS File System support. For C header files Documentation/process/license-rules.rst mandates C-like comments. (opposed to C source files where C++ style should be used). Changes made by using a script provided by Joe Perches here: https://lkml.org/lkml/2019/2/7/46. Suggested-by:
Joe Perches <joe@perches.com> Signed-off-by:
Nishad Kamdar <nishadkamdar@gmail.com> Reviewed-by:
-
Gustavo A. R. Silva authored
The current codebase makes use of the zero-length array language extension to the C90 standard, but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99: struct foo { int stuff; struct boo array[]; }; By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on. Also, notice that, dynamic memory allocations won't be affected by this change: "Flexible array members have incomplete type, and so the sizeof operator may not be applied. As a quirk of the original implementation of zero-length arrays, sizeof evaluates to zero."[1] sizeof(flexible-array-member) triggers a warning because flexible array members have incomplete type[1]. There are some instances of code in which the sizeof operator is being incorrectly/erroneously applied to zero-length arrays and the result is zero. Such instances may be hiding some bugs. So, this work (flexible-array member conversions) will also help to get completely rid of those sorts of issues. This issue was found with the help of Coccinelle. [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html [2] https://github.com/KSPP/linux/issues/21 [3] commit 76497732 ("cxgb3/l2t: Fix undefined behaviour") Signed-off-by:Gustavo A. R. Silva <gustavoars@kernel.org> Reviewed-by:
-
Zheng Bin authored
Construct an img like this: dd if=/dev/zero of=xfs.img bs=1M count=20 mkfs.xfs -d agcount=1 xfs.img xfs_db -x xfs.img sb 0 write fdblocks 0 agf 0 write freeblks 0 write longest 0 quit mount it, df -h /mnt(xfs mount point), will show this: Filesystem Size Used Avail Use% Mounted on /dev/loop0 17M -64Z -32K 100% /mnt Reported-by:
Hulk Robot <hulkci@huawei.com> Signed-off-by:
Zheng Bin <zhengbin13@huawei.com> Reviewed-by:
-
- 08 May, 2020 12 commits
-
-
Chen Zhou authored
Remove duplicate headers which are included twice. Signed-off-by:
Chen Zhou <chenzhou10@huawei.com> Reviewed-by:
-
Brian Foster authored
The random buffer write failure errortag patch introduced a local mount pointer variable for the test macro, but the macro is compiled out on !DEBUG kernels. This results in an unused variable warning. Access the mount structure through the buffer pointer and remove the local mount pointer to address the warning. Fixes: 7376d745 ("xfs: random buffer write failure errortag") Signed-off-by:
-
Darrick J. Wong authored
Remove unnecessary includes from the log recovery code. Suggested-by:
Christoph Hellwig <hch@infradead.org> Signed-off-by:
-
Darrick J. Wong authored
Move the helpers that handle incore buffer cancellation records to xfs_buf_item_recover.c since they're not directly related to the main log recovery machinery. No functional changes. Signed-off-by:
-
Darrick J. Wong authored
The only purpose of XFS_LI_RECOVERED is to prevent log recovery from trying to replay recovered intents more than once. Therefore, we can move the bit setting up to the ->iop_recover caller. Signed-off-by:
-
Darrick J. Wong authored
Now that we've made the recovered item tests all the same, we can hoist the test and the ail locking code to the ->iop_recover caller and call the recovery function directly. Signed-off-by:
-
Darrick J. Wong authored
Rename XFS_{EFI,BUI,RUI,CUI}_RECOVERED to XFS_LI_RECOVERED so that we track recovery status in the log item, then get rid of the now unused flags fields in each of those log item types. Signed-off-by: -
Darrick J. Wong authored
During recovery, every intent that we recover from the log has to be added to the AIL. Replace the open-coded addition with a helper. Signed-off-by:
-
Darrick J. Wong authored
Replace the open-coded AIL item walking with a proper helper when we're trying to release an intent item that has been finished. We add a new ->iop_match method to decide if an intent item matches a supplied ID. Signed-off-by:
-
Darrick J. Wong authored
Now that we've finished converting all types of log intent items to provide an ->iop_recover function, we can convert the "is this an intent item?" predicate to look for a non-null iop_recover pointer. Move the predicate closer to the functions that use it. Signed-off-by:
-
Darrick J. Wong authored
Move the code that processes the log items created from the recovered log items into the per-item source code files and use dispatch functions to call them. No functional changes. Signed-off-by:
-
Darrick J. Wong authored
Move the code that processes the log items created from the recovered log items into the per-item source code files and use dispatch functions to call them. No functional changes. Signed-off-by:
-
