From: "Darrick J. Wong" <djwong@kernel.org>
To: Chandan Babu R <chandanrlinux@gmail.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH V2 07/12] xfs: Rename inode's extent counter fields based on their width
Date: Tue, 27 Jul 2021 15:50:06 -0700 [thread overview]
Message-ID: <20210727225006.GR559212@magnolia> (raw)
In-Reply-To: <20210726114541.24898-8-chandanrlinux@gmail.com>
On Mon, Jul 26, 2021 at 05:15:36PM +0530, Chandan Babu R wrote:
> This commit renames extent counter fields in "struct xfs_dinode" and "struct
> xfs_log_dinode" based on the width of the fields. As of this commit, the
> 32-bit field will be used to count data fork extents and the 16-bit field will
> be used to count attr fork extents.
I totally had the preconceived notion that you were going to make the
existing fields the 'lo' bits and then add six bytes of 'hi' field to
the ondisk inode for the space you need.
Instead, I see that in the new scheme, the the space where di_anextents
is becomes unused, the space where di_nextents is now becomes the attr
fork extent count, and you allocate another 8 bytes at the end of the
ondisk inode for the data fork extent count.
Hm. That /is/ clever in that we don't have to split bits between
fields, but the downside is that if you want to upgrade existing
filesystems, you'd either have to rewrite every inode in the entire
filesystem, or introduce a di_flags2 bit to signal that this inode
actually has the extended counters. It also uses 8 bytes at the end of
the ondisk inode structure.
I think if we adjust the design a little bit we can enable the upgrade
use case and reuse existing empty space in the ondisk inode. Notice
that there are six bytes of di_pad available in the middle of the inode
record? What do you think about putting the upper fields there? The
middle of the struct then looks like this:
__be32 di_nextentshi; /* upper 32-bits of di_nextents */
__be16 di_naextentshi; /* upper 16-bits of di_naextents */
__be16 di_flushiter; /* incremented on flush */
xfs_timestamp_t di_atime; /* time last accessed */
xfs_timestamp_t di_mtime; /* time last modified */
xfs_timestamp_t di_ctime; /* time created/inode modified */
__be64 di_size; /* number of bytes in file */
__be64 di_nblocks; /* # of direct & btree blocks used */
__be32 di_extsize; /* basic/minimum extent size for file */
__be32 di_nextentslo; /* number of extents in data fork */
__be16 di_anextentslo; /* number of extents in attribute fork*/
And your xfs_dfork_extents function looks like:
case XFS_DATA_FORK:
return (xfs_extnum_t)be32_to_cpu(dip->di_nextentshi) << 32 |
be32_to_cpu(dip->di_nextentslo);
case XFS_ATTR_FORK:
return (xfs_extnum_t)be16_to_cpu(dip->di_naextentshi) << 16 |
be16_to_cpu(dip->di_naextentslo);
The pad fields are supposed to be zero, and upgrading now is no more
effort than tapping into the existing xfs_repair upgrader code to add
the EXTCOUNT64 feature flag.
--D
>
> This change is done to enable a future commit to introduce a new 64-bit extent
> counter field.
>
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
> fs/xfs/libxfs/xfs_format.h | 4 ++--
> fs/xfs/libxfs/xfs_inode_buf.c | 8 ++++----
> fs/xfs/libxfs/xfs_log_format.h | 4 ++--
> fs/xfs/scrub/inode_repair.c | 4 ++--
> fs/xfs/scrub/trace.h | 14 +++++++-------
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_inode_item_recover.c | 8 ++++----
> 7 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 001a4077a7c6..2362cc005cc6 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1039,8 +1039,8 @@ typedef struct xfs_dinode {
> __be64 di_size; /* number of bytes in file */
> __be64 di_nblocks; /* # of direct & btree blocks used */
> __be32 di_extsize; /* basic/minimum extent size for file */
> - __be32 di_nextents; /* number of extents in data fork */
> - __be16 di_anextents; /* number of extents in attribute fork*/
> + __be32 di_nextents32; /* number of extents in data fork */
> + __be16 di_nextents16; /* number of extents in attribute fork*/
> __u8 di_forkoff; /* attr fork offs, <<3 for 64b align */
> __s8 di_aformat; /* format of attr fork's data */
> __be32 di_dmevmask; /* DMIG event mask */
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 9ed04da2e2b1..65d753e16007 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -313,8 +313,8 @@ xfs_inode_to_disk(
> to->di_size = cpu_to_be64(ip->i_disk_size);
> to->di_nblocks = cpu_to_be64(ip->i_nblocks);
> to->di_extsize = cpu_to_be32(ip->i_extsize);
> - to->di_nextents = cpu_to_be32(xfs_ifork_nextents(&ip->i_df));
> - to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp));
> + to->di_nextents32 = cpu_to_be32(xfs_ifork_nextents(&ip->i_df));
> + to->di_nextents16 = cpu_to_be16(xfs_ifork_nextents(ip->i_afp));
/me wonders if these should get their own static inline conversion
helpers to set the appropriate fields, like I did for timestamps?
> to->di_forkoff = ip->i_forkoff;
> to->di_aformat = xfs_ifork_format(ip->i_afp);
> to->di_flags = cpu_to_be16(ip->i_diflags);
> @@ -389,11 +389,11 @@ xfs_dfork_nextents(
>
> switch (whichfork) {
> case XFS_DATA_FORK:
> - *nextents = be32_to_cpu(dip->di_nextents);
> + *nextents = be32_to_cpu(dip->di_nextents32);
> break;
>
> case XFS_ATTR_FORK:
> - *nextents = be16_to_cpu(dip->di_anextents);
> + *nextents = be16_to_cpu(dip->di_nextents16);
> break;
>
> default:
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 0c888f92184e..ca8e4ad8312a 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -402,8 +402,8 @@ struct xfs_log_dinode {
> xfs_fsize_t di_size; /* number of bytes in file */
> xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
> xfs_extlen_t di_extsize; /* basic/minimum extent size for file */
> - xfs_extnum_t di_nextents; /* number of extents in data fork */
> - xfs_aextnum_t di_anextents; /* number of extents in attribute fork*/
> + uint32_t di_nextents32; /* number of extents in data fork */
> + uint16_t di_nextents16; /* number of extents in attribute fork*/
> uint8_t di_forkoff; /* attr fork offs, <<3 for 64b align */
> int8_t di_aformat; /* format of attr fork's data */
> uint32_t di_dmevmask; /* DMIG event mask */
> diff --git a/fs/xfs/scrub/inode_repair.c b/fs/xfs/scrub/inode_repair.c
> index 521c8df00990..4d773a16f886 100644
> --- a/fs/xfs/scrub/inode_repair.c
> +++ b/fs/xfs/scrub/inode_repair.c
> @@ -736,7 +736,7 @@ xrep_dinode_zap_dfork(
> {
> trace_xrep_dinode_zap_dfork(sc, dip);
>
> - dip->di_nextents = 0;
> + dip->di_nextents32 = 0;
>
> /* Special files always get reset to DEV */
> switch (mode & S_IFMT) {
> @@ -823,7 +823,7 @@ xrep_dinode_zap_afork(
> trace_xrep_dinode_zap_afork(sc, dip);
>
> dip->di_aformat = XFS_DINODE_FMT_EXTENTS;
> - dip->di_anextents = 0;
> + dip->di_nextents16 = 0;
>
> dip->di_forkoff = 0;
> dip->di_mode = cpu_to_be16(mode & ~0777);
> diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> index fd03685b1f6b..a0303f692e52 100644
> --- a/fs/xfs/scrub/trace.h
> +++ b/fs/xfs/scrub/trace.h
> @@ -1209,8 +1209,8 @@ DECLARE_EVENT_CLASS(xrep_dinode_class,
> __field(uint64_t, size)
> __field(uint64_t, nblocks)
> __field(uint32_t, extsize)
> - __field(uint32_t, nextents)
> - __field(uint16_t, anextents)
> + __field(uint32_t, nextents32)
> + __field(uint16_t, nextents16)
> __field(uint8_t, forkoff)
> __field(uint8_t, aformat)
> __field(uint16_t, flags)
> @@ -1229,8 +1229,8 @@ DECLARE_EVENT_CLASS(xrep_dinode_class,
> __entry->size = be64_to_cpu(dip->di_size);
> __entry->nblocks = be64_to_cpu(dip->di_nblocks);
> __entry->extsize = be32_to_cpu(dip->di_extsize);
> - __entry->nextents = be32_to_cpu(dip->di_nextents);
> - __entry->anextents = be16_to_cpu(dip->di_anextents);
> + __entry->nextents32 = be32_to_cpu(dip->di_nextents32);
> + __entry->nextents16 = be16_to_cpu(dip->di_nextents16);
> __entry->forkoff = dip->di_forkoff;
> __entry->aformat = dip->di_aformat;
> __entry->flags = be16_to_cpu(dip->di_flags);
> @@ -1238,7 +1238,7 @@ DECLARE_EVENT_CLASS(xrep_dinode_class,
> __entry->flags2 = be64_to_cpu(dip->di_flags2);
> __entry->cowextsize = be32_to_cpu(dip->di_cowextsize);
> ),
> - TP_printk("dev %d:%d ino 0x%llx mode 0x%x version %u format %u uid %u gid %u size %llu nblocks %llu extsize %u nextents %u anextents %u forkoff %u aformat %u flags 0x%x gen 0x%x flags2 0x%llx cowextsize %u",
> + TP_printk("dev %d:%d ino 0x%llx mode 0x%x version %u format %u uid %u gid %u size %llu nblocks %llu extsize %u nextents32 %u nextents16 %u forkoff %u aformat %u flags 0x%x gen 0x%x flags2 0x%llx cowextsize %u",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> __entry->ino,
> __entry->mode,
> @@ -1249,8 +1249,8 @@ DECLARE_EVENT_CLASS(xrep_dinode_class,
> __entry->size,
> __entry->nblocks,
> __entry->extsize,
> - __entry->nextents,
> - __entry->anextents,
> + __entry->nextents32,
> + __entry->nextents16,
> __entry->forkoff,
> __entry->aformat,
> __entry->flags,
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 35de30849fcc..f54ce7468ba1 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -385,8 +385,8 @@ xfs_inode_to_log_dinode(
> to->di_size = ip->i_disk_size;
> to->di_nblocks = ip->i_nblocks;
> to->di_extsize = ip->i_extsize;
> - to->di_nextents = xfs_ifork_nextents(&ip->i_df);
> - to->di_anextents = xfs_ifork_nextents(ip->i_afp);
> + to->di_nextents32 = xfs_ifork_nextents(&ip->i_df);
> + to->di_nextents16 = xfs_ifork_nextents(ip->i_afp);
> to->di_forkoff = ip->i_forkoff;
> to->di_aformat = xfs_ifork_format(ip->i_afp);
> to->di_flags = ip->i_diflags;
> diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
> index 7b79518b6c20..40af9d1265c7 100644
> --- a/fs/xfs/xfs_inode_item_recover.c
> +++ b/fs/xfs/xfs_inode_item_recover.c
> @@ -166,8 +166,8 @@ xfs_log_dinode_to_disk(
> to->di_size = cpu_to_be64(from->di_size);
> to->di_nblocks = cpu_to_be64(from->di_nblocks);
> to->di_extsize = cpu_to_be32(from->di_extsize);
> - to->di_nextents = cpu_to_be32(from->di_nextents);
> - to->di_anextents = cpu_to_be16(from->di_anextents);
> + to->di_nextents32 = cpu_to_be32(from->di_nextents32);
> + to->di_nextents16 = cpu_to_be16(from->di_nextents16);
> to->di_forkoff = from->di_forkoff;
> to->di_aformat = from->di_aformat;
> to->di_dmevmask = cpu_to_be32(from->di_dmevmask);
> @@ -332,7 +332,7 @@ xlog_recover_inode_commit_pass2(
> goto out_release;
> }
> }
> - if (unlikely(ldip->di_nextents + ldip->di_anextents > ldip->di_nblocks)){
> + if (unlikely(ldip->di_nextents32 + ldip->di_nextents16 > ldip->di_nblocks)) {
> XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(5)",
> XFS_ERRLEVEL_LOW, mp, ldip,
> sizeof(*ldip));
> @@ -340,7 +340,7 @@ xlog_recover_inode_commit_pass2(
> "%s: Bad inode log record, rec ptr "PTR_FMT", dino ptr "PTR_FMT", "
> "dino bp "PTR_FMT", ino %Ld, total extents = %d, nblocks = %Ld",
> __func__, item, dip, bp, in_f->ilf_ino,
> - ldip->di_nextents + ldip->di_anextents,
> + ldip->di_nextents32 + ldip->di_nextents16,
> ldip->di_nblocks);
> error = -EFSCORRUPTED;
> goto out_release;
> --
> 2.30.2
>
next prev parent reply other threads:[~2021-07-27 22:50 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
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 [this message]
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=20210727225006.GR559212@magnolia \
--to=djwong@kernel.org \
--cc=chandanrlinux@gmail.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.