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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).