From: Chandan Babu R <chandanrlinux@gmail.com>
To: linux-xfs@vger.kernel.org
Cc: Chandan Babu R <chandanrlinux@gmail.com>,
darrick.wong@oracle.com, david@fromorbit.com
Subject: [PATCH V6 00/11] Bail out if transaction can cause extent count to overflow
Date: Mon, 12 Oct 2020 14:59:27 +0530 [thread overview]
Message-ID: <20201012092938.50946-1-chandanrlinux@gmail.com> (raw)
XFS does not check for possible overflow of per-inode extent counter
fields when adding extents to either data or attr fork.
For e.g.
1. Insert 5 million xattrs (each having a value size of 255 bytes) and
then delete 50% of them in an alternating manner.
2. On a 4k block sized XFS filesystem instance, the above causes 98511
extents to be created in the attr fork of the inode.
xfsaild/loop0 2008 [003] 1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131
3. The incore inode fork extent counter is a signed 32-bit
quantity. However, the on-disk extent counter is an unsigned 16-bit
quantity and hence cannot hold 98511 extents.
4. The following incorrect value is stored in the xattr extent counter,
# xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
core.naextents = -32561
This patchset adds a new helper function
(i.e. xfs_iext_count_may_overflow()) to check for overflow of the
per-inode data and xattr extent counters and invokes it before
starting an fs operation (e.g. creating a new directory entry). With
this patchset applied, XFS detects counter overflows and returns with
an error rather than causing a silent corruption.
The patchset has been tested by executing xfstests with the following
mkfs.xfs options,
1. -m crc=0 -b size=1k
2. -m crc=0 -b size=4k
3. -m crc=0 -b size=512
4. -m rmapbt=1,reflink=1 -b size=1k
5. -m rmapbt=1,reflink=1 -b size=4k
The patches can also be obtained from
https://github.com/chandanr/linux.git at branch xfs-reserve-extent-count-v6.
I have two patches that define the newly introduced error injection
tags in xfsprogs
(https://github.com/chandanr/xfsprogs-dev/commit/7fd7aeef1cefbcc9abd6dd5887e710c80e48079d
and
https://github.com/chandanr/xfsprogs-dev/commit/3cbe12f6fdf306de06c4096eb50641fa2d834dc5).
I have also written tests
(https://github.com/chandanr/check-iext-overflow/blob/master/check-iext-overflow.sh/)
for verifying the checks introduced in the kernel. The tests have to
be edited to make them suitable for merging with xfstests. But they
also depend on error tags introduced in these patches . Hence, I am
planning to post the changes for xfsprogs and xfstests if other
developers are fine with the changes made in this patchset.
Changelog:
V5 -> V6:
1. Rebased the patchset on xfs-linux/for-next branch.
2. Drop "xfs: Set tp->t_firstblock only once during a transaction's
lifetime" patch from the patchset.
3. Add a comment to xfs_bmap_btalloc() describing why it was chosen
to start "free space extent search" from AG 0 when
XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT is enabled and when the
transaction is allocating its first extent.
4. Fix review comments associated with coding style.
V4 -> V5:
1. Introduce new error tag XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT to
let user space programs to be able to guarantee that free space
requests for files are satisfied by allocating minlen sized
extents.
2. Change xfs_bmap_btalloc() and xfs_alloc_vextent() to allocate
minlen sized extents when XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT is
enabled.
3. Introduce a new patch that causes tp->t_firstblock to be assigned
to a value only when its previous value is NULLFSBLOCK.
4. Replace the previously introduced MAXERRTAGEXTNUM (maximum inode
fork extent count) with the hardcoded value of 10.
5. xfs_bui_item_recover(): Use XFS_IEXT_ADD_NOSPLIT_CNT when mapping
an extent.
6. xfs_swap_extent_rmap(): Use xfs_bmap_is_real_extent() instead of
xfs_bmap_is_update_needed() to assess if the extent really needs
to be swapped.
V3 -> V4:
1. Introduce new patch which lets userspace programs to test "extent
count overflow detection" by injecting an error tag. The new
error tag reduces the maximum allowed extent count to 10.
2. Injecting the newly defined error tag prevents
xfs_bmap_add_extent_hole_real() from merging a new extent with
its neighbours to allow writing deterministic tests for testing
extent count overflow for Directories, Xattr and growing realtime
devices. This is required because the new extent being allocated
can be contiguous with its neighbours (w.r.t both file and disk
offsets).
3. Injecting the newly defined error tag forces block sized extents
to be allocated for summary/bitmap files when growing a realtime
device. This is required because xfs_growfs_rt_alloc() allocates
as large an extent as possible for summary/bitmap files and hence
it would be impossible to write deterministic tests.
4. Rename XFS_IEXT_REMOVE_CNT to XFS_IEXT_PUNCH_HOLE_CNT to reflect
the actual meaning of the fs operation.
5. Fold XFS_IEXT_INSERT_HOLE_CNT code into that associated with
XFS_IEXT_PUNCH_HOLE_CNT since both perform the same job.
6. xfs_swap_extent_rmap(): Check for extent overflow should be made
on the source file only if the donor file extent has a valid
on-disk mapping and vice versa.
V2 -> V3:
1. Move the definition of xfs_iext_count_may_overflow() from
libxfs/xfs_trans_resv.c to libxfs/xfs_inode_fork.c. Also, I tried
to make xfs_iext_count_may_overflow() an inline function by
placing the definition in libxfs/xfs_inode_fork.h. However this
required that the definition of 'struct xfs_inode' be available,
since xfs_iext_count_may_overflow() uses a 'struct xfs_inode *'
type variable.
2. Handle XFS_COW_FORK within xfs_iext_count_may_overflow() by
returning a success value.
3. Rename XFS_IEXT_ADD_CNT to XFS_IEXT_ADD_NOSPLIT_CNT. Thanks to
Darrick for the suggesting the new name.
4. Expand comments to make use of 80 columns.
V1 -> V2:
1. Rename helper function from xfs_trans_resv_ext_cnt() to
xfs_iext_count_may_overflow().
2. Define and use macros to represent fs operations and the
corresponding increase in extent count.
3. Split the patches based on the fs operation being performed.
Chandan Babu R (11):
xfs: Add helper for checking per-inode extent count overflow
xfs: Check for extent overflow when trivally adding a new extent
xfs: Check for extent overflow when punching a hole
xfs: Check for extent overflow when adding/removing xattrs
xfs: Check for extent overflow when adding/removing dir entries
xfs: Check for extent overflow when writing to unwritten extent
xfs: Check for extent overflow when moving extent from cow to data
fork
xfs: Check for extent overflow when remapping an extent
xfs: Check for extent overflow when swapping extents
xfs: Introduce error injection to reduce maximum inode fork extent
count
xfs: Introduce error injection to allocate only minlen size extents
for files
fs/xfs/libxfs/xfs_alloc.c | 46 ++++++++++++++++++++
fs/xfs/libxfs/xfs_alloc.h | 1 +
fs/xfs/libxfs/xfs_attr.c | 13 ++++++
fs/xfs/libxfs/xfs_bmap.c | 40 ++++++++++++++----
fs/xfs/libxfs/xfs_errortag.h | 6 ++-
fs/xfs/libxfs/xfs_inode_fork.c | 27 ++++++++++++
fs/xfs/libxfs/xfs_inode_fork.h | 77 ++++++++++++++++++++++++++++++++++
fs/xfs/xfs_bmap_item.c | 10 +++++
fs/xfs/xfs_bmap_util.c | 31 ++++++++++++++
fs/xfs/xfs_dquot.c | 8 +++-
fs/xfs/xfs_error.c | 6 +++
fs/xfs/xfs_inode.c | 27 ++++++++++++
fs/xfs/xfs_iomap.c | 10 +++++
fs/xfs/xfs_reflink.c | 10 +++++
fs/xfs/xfs_rtalloc.c | 5 +++
fs/xfs/xfs_symlink.c | 5 +++
16 files changed, 313 insertions(+), 9 deletions(-)
--
2.28.0
next reply other threads:[~2020-10-12 9:30 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-12 9:29 Chandan Babu R [this message]
2020-10-12 9:29 ` [PATCH V6 01/11] xfs: Add helper for checking per-inode extent count overflow Chandan Babu R
2020-10-15 8:34 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 02/11] xfs: Check for extent overflow when trivally adding a new extent Chandan Babu R
2020-10-15 8:34 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 03/11] xfs: Check for extent overflow when punching a hole Chandan Babu R
2020-10-15 8:35 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 04/11] xfs: Check for extent overflow when adding/removing xattrs Chandan Babu R
2020-10-15 8:36 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 05/11] xfs: Check for extent overflow when adding/removing dir entries Chandan Babu R
2020-10-15 8:36 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 06/11] xfs: Check for extent overflow when writing to unwritten extent Chandan Babu R
2020-10-15 8:36 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 07/11] xfs: Check for extent overflow when moving extent from cow to data fork Chandan Babu R
2020-10-15 8:37 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 08/11] xfs: Check for extent overflow when remapping an extent Chandan Babu R
2020-10-15 8:39 ` Christoph Hellwig
2020-10-15 10:01 ` Chandan Babu R
2020-10-15 18:45 ` Darrick J. Wong
2020-10-16 4:27 ` Chandan Babu R
2020-10-16 7:04 ` Christoph Hellwig
2020-10-16 11:28 ` Chandan Babu R
2020-10-16 15:29 ` Darrick J. Wong
2020-10-17 2:55 ` Chandan Babu R
2020-10-12 9:29 ` [PATCH V6 09/11] xfs: Check for extent overflow when swapping extents Chandan Babu R
2020-10-12 9:29 ` [PATCH V6 10/11] xfs: Introduce error injection to reduce maximum inode fork extent count Chandan Babu R
2020-10-15 8:40 ` Christoph Hellwig
2020-10-12 9:29 ` [PATCH V6 11/11] xfs: Introduce error injection to allocate only minlen size extents for files Chandan Babu R
2020-10-15 8:41 ` Christoph Hellwig
2020-10-15 10:02 ` Chandan Babu R
2020-10-15 18:41 ` Darrick J. Wong
2020-10-16 11:31 ` Chandan Babu R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201012092938.50946-1-chandanrlinux@gmail.com \
--to=chandanrlinux@gmail.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.