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@xxxxxxxxx>
>> ---
>> 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
