From: Chandan Babu R <chandanrlinux@gmail.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH V2 06/12] xfs: xfs_dfork_nextents: Return extent count via an out argument
Date: Wed, 28 Jul 2021 09:51:48 +0530 [thread overview]
Message-ID: <878s1rvzj7.fsf@garuda> (raw)
In-Reply-To: <20210727222215.GP559212@magnolia>
On 28 Jul 2021 at 03:52, Darrick J. Wong wrote:
> On Mon, Jul 26, 2021 at 05:15:35PM +0530, Chandan Babu R wrote:
>> This commit changes xfs_dfork_nextents() to return an error code. The extent
>> count itself is now returned through an out argument. This facility will be
>> used by a future commit to indicate an inconsistent ondisk extent count.
>>
>> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
>> ---
>> fs/xfs/libxfs/xfs_inode_buf.c | 29 +++++++----
>> fs/xfs/libxfs/xfs_inode_buf.h | 4 +-
>> fs/xfs/libxfs/xfs_inode_fork.c | 22 ++++++--
>> fs/xfs/scrub/inode.c | 94 +++++++++++++++++++++-------------
>> fs/xfs/scrub/inode_repair.c | 34 ++++++++----
>> 5 files changed, 119 insertions(+), 64 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
>> index 6bef0757fca4..9ed04da2e2b1 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.c
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
>> @@ -345,7 +345,8 @@ xfs_dinode_verify_fork(
>> xfs_extnum_t di_nextents;
>> xfs_extnum_t max_extents;
>>
>> - di_nextents = xfs_dfork_nextents(mp, dip, whichfork);
>> + if (xfs_dfork_nextents(mp, dip, whichfork, &di_nextents))
>> + return __this_address;
>>
>> switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>> case XFS_DINODE_FMT_LOCAL:
>> @@ -377,29 +378,31 @@ xfs_dinode_verify_fork(
>> return NULL;
>> }
>>
>> -xfs_extnum_t
>> +int
>> xfs_dfork_nextents(
>> struct xfs_mount *mp,
>> struct xfs_dinode *dip,
>> - int whichfork)
>> + int whichfork,
>> + xfs_extnum_t *nextents)
>> {
>> - xfs_extnum_t nextents = 0;
>> + int error = 0;
>>
>> switch (whichfork) {
>> case XFS_DATA_FORK:
>> - nextents = be32_to_cpu(dip->di_nextents);
>> + *nextents = be32_to_cpu(dip->di_nextents);
>> break;
>>
>> case XFS_ATTR_FORK:
>> - nextents = be16_to_cpu(dip->di_anextents);
>> + *nextents = be16_to_cpu(dip->di_anextents);
>> break;
>>
>> default:
>> ASSERT(0);
>> + error = -EINVAL;
>
> -EFSCORRUPTED? We don't have a specific code for "your darn software
> screwed up, hyuck!!" but I guess this will at least get peoples'
> attention.
Ok. I will update this.
>
>> break;
>> }
>>
>> - return nextents;
>> + return error;
>> }
>>
>> static xfs_failaddr_t
>> @@ -502,6 +505,7 @@ xfs_dinode_verify(
>> uint64_t flags2;
>> uint64_t di_size;
>> xfs_extnum_t nextents;
>> + xfs_extnum_t naextents;
>> int64_t nblocks;
>>
>> if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
>> @@ -533,8 +537,13 @@ xfs_dinode_verify(
>> if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
>> return __this_address;
>>
>> - nextents = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK);
>> - nextents += xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
>> + if (xfs_dfork_nextents(mp, dip, XFS_DATA_FORK, &nextents))
>> + return __this_address;
>> +
>> + if (xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK, &naextents))
>> + return __this_address;
>> +
>> + nextents += naextents;
>> nblocks = be64_to_cpu(dip->di_nblocks);
>>
>> /* Fork checks carried over from xfs_iformat_fork */
>> @@ -595,7 +604,7 @@ xfs_dinode_verify(
>> default:
>> return __this_address;
>> }
>> - if (xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK))
>> + if (naextents)
>> return __this_address;
>> }
>>
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
>> index ea2c35091609..20f796610d46 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.h
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
>> @@ -36,8 +36,8 @@ xfs_failaddr_t xfs_inode_validate_extsize(struct xfs_mount *mp,
>> xfs_failaddr_t xfs_inode_validate_cowextsize(struct xfs_mount *mp,
>> uint32_t cowextsize, uint16_t mode, uint16_t flags,
>> uint64_t flags2);
>> -xfs_extnum_t xfs_dfork_nextents(struct xfs_mount *mp, struct xfs_dinode *dip,
>> - int whichfork);
>> +int xfs_dfork_nextents(struct xfs_mount *mp, struct xfs_dinode *dip,
>> + int whichfork, xfs_extnum_t *nextents);
>>
>> static inline uint64_t xfs_inode_encode_bigtime(struct timespec64 tv)
>> {
>> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
>> index 38dd2dfc31fa..7f7ffe29436d 100644
>> --- a/fs/xfs/libxfs/xfs_inode_fork.c
>> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
>> @@ -107,13 +107,20 @@ xfs_iformat_extents(
>> struct xfs_mount *mp = ip->i_mount;
>> struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
>> int state = xfs_bmap_fork_to_state(whichfork);
>> - xfs_extnum_t nex = xfs_dfork_nextents(mp, dip, whichfork);
>> - int size = nex * sizeof(xfs_bmbt_rec_t);
>> + xfs_extnum_t nex;
>> + int size;
>> struct xfs_iext_cursor icur;
>> struct xfs_bmbt_rec *dp;
>> struct xfs_bmbt_irec new;
>> + int error;
>> int i;
>>
>> + error = xfs_dfork_nextents(mp, dip, whichfork, &nex);
>> + if (error)
>> + return error;
>> +
>> + size = nex * sizeof(xfs_bmbt_rec_t);
>
> sizeof(struct xfs_bmbt_rec);
>
> (Please convert the old typedef usage when possible.)
Sure. I will go through the patchset and update relevant code.
>
>> +
>> /*
>> * If the number of extents is unreasonable, then something is wrong and
>> * we just bail out rather than crash in kmem_alloc() or memcpy() below.
>> @@ -235,7 +242,10 @@ xfs_iformat_data_fork(
>> * depend on it.
>> */
>> ip->i_df.if_format = dip->di_format;
>> - ip->i_df.if_nextents = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK);
>> + error = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK,
>> + &ip->i_df.if_nextents);
>> + if (error)
>> + return error;
>>
>> switch (inode->i_mode & S_IFMT) {
>> case S_IFIFO:
>> @@ -304,9 +314,11 @@ xfs_iformat_attr_fork(
>> {
>> struct xfs_mount *mp = ip->i_mount;
>> xfs_extnum_t nextents;
>> - int error = 0;
>> + int error;
>>
>> - nextents = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
>> + error = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK, &nextents);
>> + if (error)
>> + return error;
>>
>> /*
>> * Initialize the extent count early, as the per-format routines may
>> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
>> index a161dac31a6f..e9dc3749ea08 100644
>> --- a/fs/xfs/scrub/inode.c
>> +++ b/fs/xfs/scrub/inode.c
>> @@ -208,6 +208,44 @@ xchk_dinode_nsec(
>> xchk_ino_set_corrupt(sc, ino);
>> }
>>
>> +STATIC void
>> +xchk_dinode_fork_recs(
>> + struct xfs_scrub *sc,
>> + struct xfs_dinode *dip,
>> + xfs_ino_t ino,
>> + xfs_extnum_t nextents,
>> + int whichfork)
>> +{
>> + struct xfs_mount *mp = sc->mp;
>> + size_t fork_recs;
>> + unsigned char format;
>> +
>> + if (whichfork == XFS_DATA_FORK) {
>> + fork_recs = XFS_DFORK_DSIZE(dip, mp)
>> + / sizeof(struct xfs_bmbt_rec);
>> + format = dip->di_format;
>> + } else if (whichfork == XFS_ATTR_FORK) {
>> + fork_recs = XFS_DFORK_ASIZE(dip, mp)
>> + / sizeof(struct xfs_bmbt_rec);
>> + format = dip->di_aformat;
>> + }
>
> fork_recs = XFS_DFORK_SIZE(dip, mp, whichfork);
> format = XFS_DFORK_FORMAT(dip, whichfork);
>
> ?
I agree. This increases readability of code.
>
>> +
>> + switch (format) {
>> + case XFS_DINODE_FMT_EXTENTS:
>> + if (nextents > fork_recs)
>> + xchk_ino_set_corrupt(sc, ino);
>> + break;
>> + case XFS_DINODE_FMT_BTREE:
>> + if (nextents <= fork_recs)
>> + xchk_ino_set_corrupt(sc, ino);
>> + break;
>> + default:
>> + if (nextents != 0)
>> + xchk_ino_set_corrupt(sc, ino);
>> + break;
>> + }
>> +}
>> +
>> /* Scrub all the ondisk inode fields. */
>> STATIC void
>> xchk_dinode(
>> @@ -216,7 +254,6 @@ xchk_dinode(
>> xfs_ino_t ino)
>> {
>> struct xfs_mount *mp = sc->mp;
>> - size_t fork_recs;
>> unsigned long long isize;
>> uint64_t flags2;
>> xfs_extnum_t nextents;
>> @@ -224,6 +261,7 @@ xchk_dinode(
>> prid_t prid;
>> uint16_t flags;
>> uint16_t mode;
>> + int error;
>>
>> flags = be16_to_cpu(dip->di_flags);
>> if (dip->di_version >= 3)
>> @@ -379,33 +417,22 @@ xchk_dinode(
>> xchk_inode_extsize(sc, dip, ino, mode, flags);
>>
>> /* di_nextents */
>> - nextents = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK);
>> - fork_recs = XFS_DFORK_DSIZE(dip, mp) / sizeof(struct xfs_bmbt_rec);
>> - switch (dip->di_format) {
>> - case XFS_DINODE_FMT_EXTENTS:
>> - if (nextents > fork_recs)
>> - xchk_ino_set_corrupt(sc, ino);
>> - break;
>> - case XFS_DINODE_FMT_BTREE:
>> - if (nextents <= fork_recs)
>> - xchk_ino_set_corrupt(sc, ino);
>> - break;
>> - default:
>> - if (nextents != 0)
>> - xchk_ino_set_corrupt(sc, ino);
>> - break;
>> - }
>> -
>> - naextents = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
>> + error = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK, &nextents);
>> + if (error)
>> + xchk_ino_set_corrupt(sc, ino);
>> + else
>
> error = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK, &nextents);
> if (error) {
> xchk_ino_set_corrupt(sc, ino);
> return;
> }
> xchk_dinode_fork_recs(sc, dip, ino, nextents, XFS_DATA_FORK);
>
> At this point you might as well return; you have sufficient information
> to generate the corruption report for userspace.
Ok. I will update.
>
>> + xchk_dinode_fork_recs(sc, dip, ino, nextents, XFS_DATA_FORK);
>>
>> /* di_forkoff */
>> if (XFS_DFORK_APTR(dip) >= (char *)dip + mp->m_sb.sb_inodesize)
>> xchk_ino_set_corrupt(sc, ino);
>> - if (naextents != 0 && dip->di_forkoff == 0)
>> - xchk_ino_set_corrupt(sc, ino);
>> if (dip->di_forkoff == 0 && dip->di_aformat != XFS_DINODE_FMT_EXTENTS)
>> xchk_ino_set_corrupt(sc, ino);
>>
>> + error = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK, &naextents);
>> + if (error || (naextents != 0 && dip->di_forkoff == 0))
>> + xchk_ino_set_corrupt(sc, ino);
>
> Please keep these separate so that the debug tracepoints can capture
> them as separate corruption sources. Also, if xfs_dfork_nextents
> returns an error, you might as well return since we have enough data to
> generate the corruption report.
>
Ok. I will update this as well.
> (The rest of the scrub and repair code changes look good, btw.)
Thanks for the review.
--
chandan
next prev parent reply other threads:[~2021-07-28 4:21 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-26 11:45 [PATCH V2 00/12] xfs: Extend per-inode extent counters Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 01/12] xfs: Move extent count limits to xfs_format.h Chandan Babu R
2021-07-26 18:00 ` Darrick J. Wong
2021-07-27 8:07 ` Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 02/12] xfs: Rename MAXEXTNUM, MAXAEXTNUM to XFS_IFORK_EXTCNT_MAXS32, XFS_IFORK_EXTCNT_MAXS16 Chandan Babu R
2021-07-27 21:56 ` Darrick J. Wong
2021-07-27 22:03 ` Darrick J. Wong
2021-07-28 3:15 ` Chandan Babu R
2021-08-23 4:18 ` Chandan Babu R
2021-08-23 7:17 ` Chandan Babu R
2021-08-23 18:16 ` Darrick J. Wong
2021-07-26 11:45 ` [PATCH V2 03/12] xfs: Introduce xfs_iext_max() helper Chandan Babu R
2021-07-27 21:58 ` Darrick J. Wong
2021-07-28 3:17 ` Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 04/12] xfs: Use xfs_extnum_t instead of basic data types Chandan Babu R
2021-07-27 21:59 ` Darrick J. Wong
2021-07-28 3:38 ` Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 05/12] xfs: Introduce xfs_dfork_nextents() helper Chandan Babu R
2021-07-27 22:10 ` Darrick J. Wong
2021-07-28 4:06 ` Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 06/12] xfs: xfs_dfork_nextents: Return extent count via an out argument Chandan Babu R
2021-07-27 22:22 ` Darrick J. Wong
2021-07-28 4:21 ` Chandan Babu R [this message]
2021-07-26 11:45 ` [PATCH V2 07/12] xfs: Rename inode's extent counter fields based on their width Chandan Babu R
2021-07-27 22:50 ` Darrick J. Wong
2021-07-28 5:48 ` Chandan Babu R
2021-07-28 19:04 ` Darrick J. Wong
2021-07-26 11:45 ` [PATCH V2 08/12] xfs: Promote xfs_extnum_t and xfs_aextnum_t to 64 and 32-bits respectively Chandan Babu R
2021-07-27 22:29 ` Darrick J. Wong
2021-07-26 11:45 ` [PATCH V2 09/12] xfs: Rename XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5 Chandan Babu R
2021-07-27 22:54 ` Darrick J. Wong
2021-07-27 23:00 ` Darrick J. Wong
2021-07-27 23:17 ` Dave Chinner
2021-07-28 6:56 ` Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 10/12] xfs: Enable bulkstat ioctl to support 64-bit extent counters Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 11/12] xfs: Extend per-inode extent counter widths Chandan Babu R
2021-07-27 23:09 ` Darrick J. Wong
2021-07-28 7:17 ` Chandan Babu R
2021-07-26 11:45 ` [PATCH V2 12/12] xfs: Error tag to test if v5 bulkstat skips inodes with large extent count Chandan Babu R
2021-07-27 23:10 ` Darrick J. Wong
2021-07-28 7:23 ` Chandan Babu R
2021-07-28 7:38 ` Chandan Babu R
2021-07-28 19:06 ` Darrick J. Wong
2021-07-28 21:27 ` [PATCH V2 00/12] xfs: Extend per-inode extent counters Darrick J. Wong
2021-07-29 6:40 ` 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=878s1rvzj7.fsf@garuda \
--to=chandanrlinux@gmail.com \
--cc=djwong@kernel.org \
--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.