| Message ID | 20210916100647.176018-8-chandan.babu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Extend per-inode extent counters | expand |
On Thu, Sep 16, 2021 at 03:36:42PM +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. > > This change is done to enable a future commit to introduce a new 64-bit extent > counter field. > > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> > --- > fs/xfs/libxfs/xfs_format.h | 8 ++++---- > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- > 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 dba868f2c3e3..87c927d912f6 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -802,8 +802,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*/ Hmmm. Having the same field in the inode hold the extent count for different inode forks based on a bit in the superblock means the on-disk inode format is not self describing. i.e. we can't decode the on-disk contents of an inode correctly without knowing whether a specific feature bit is set in the superblock or not. Right now we don't have use external configs to decode the inode. Feature level conditional fields are determined by inode version, not superblock bits. Optional feature fields easy to deal with - zero if the feature is not in use, otherwise we assume it is in use and can validity check it appropriately. IOWs, we don't need to look at sb feature bits to decode and validate inode fields. This change means that we can't determine if the extent counts are correct just by looking at the on-disk inode. If we just have di_nextents32 set to a non-zero value, does that mean we should have data fork extents or attribute fork extents present? Just looking at whether the attr fork is initialised is not sufficient - it can be initialised with zero attr fork extents present. We can't look at the literal area contents, either, because we don't zero that when we shrink it. We can't look at di_nblocks, because that counts both attr and data for blocks. We can't look at di_size, because we can have data extents beyond EOF and hence a size of zero doesn't mean the data fork is empty. So if both forks are in extent format, they could be either both empty, both contain extents or only one fork contains extents but we can't tell which state is the correct one. Hence if di_nextents64 if zero, we don't know if di_nextents32 is a count of attribute extents or data extents without first looking at the superblock feature bit to determine if di_nextents64 is in use or not. The inode format is not self describing anymore. When XFS introduced 32 bit link counts, the inode version was bumped from v1 to v2 because it redefined fields in the inode structure similar to this proposal[1]. The verison number was then used to determine if the inode was in old or new format - it was a self describing format change. Hence If we are going to redefine di_nextents to be able to hold either data fork extent count (old format) or attr fork extent count (new format) we really need to bump the inode version so that we can discriminate between the two inode formats just by looking at the inode itself. If we don't want to bump the version, then we need to do something like: - __be32 di_nextents; /* number of extents in data fork */ - __be16 di_anextents; /* number of extents in attribute fork*/ + __be32 di_nextents_old;/* old number of extents in data fork */ + __be16 di_anextents_old;/* old number of extents in attribute fork*/ ..... - __u8 di_pad2[12]; + __be64 di_nextents; /* number of extents in data fork */ + __be32 di_anextents; /* number of extents in attribute fork*/ + __u8 di_pad2[4]; So that there is no ambiguity in the on-disk format between the two formats - if one set is non-zero, the other set must be zero in this sort of setup. However, I think that redefining the fields and bumping the inode version is the better long term strategy, as it allows future reuse of the di_anextents_old field, and it uses less of the small amount of unused padding we have remaining in the on-disk inode core. At which point, the feature bit in the superblock becomes "has v4 inodes", not "has big extent counts". We then use v4 inode format in memory for everything (i.e. 64 bit extent counts) and convert to/from the ondisk format at IO time like we do with v1/v2 inodes. Thoughts? -Dave. [1] The change to v2 inodes back in 1995 removed the filesystem UUID from the inode and was replaced with a 32 bit link counter, a project ID value and padding: @@ -36,10 +38,12 @@ typedef struct xfs_dinode_core __uint16_t di_mode; /* mode and type of file */ __int8_t di_version; /* inode version */ __int8_t di_format; /* format of di_c data */ - __uint16_t di_nlink; /* number of links to file */ + __uint16_t di_onlink; /* old number of links to file */ __uint32_t di_uid; /* owner's user id */ __uint32_t di_gid; /* owner's group id */ - uuid_t di_uuid; /* file unique id */ + __uint32_t di_nlink; /* number of links to file */ + __uint16_t di_projid; /* owner's project id */ + __uint8_t di_pad[10]; /* unused, zeroed space */ 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 */ @@ -81,7 +85,13 @@ typedef struct xfs_dinode it was the redefinition of the di_uuid variable space that required the bumping of the inode version...
On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote: > On Thu, Sep 16, 2021 at 03:36:42PM +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. > > > > This change is done to enable a future commit to introduce a new 64-bit extent > > counter field. > > > > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> > > --- > > fs/xfs/libxfs/xfs_format.h | 8 ++++---- > > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- > > 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 dba868f2c3e3..87c927d912f6 100644 > > --- a/fs/xfs/libxfs/xfs_format.h > > +++ b/fs/xfs/libxfs/xfs_format.h > > @@ -802,8 +802,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*/ > > > Hmmm. Having the same field in the inode hold the extent count > for different inode forks based on a bit in the superblock means the > on-disk inode format is not self describing. i.e. we can't decode > the on-disk contents of an inode correctly without knowing whether a > specific feature bit is set in the superblock or not. Hmmmm - I just realised that there is an inode flag that indicates the format is different. It's jsut that most of the code doing conditional behaviour is using the superblock flag, not the inode flag as the conditional. So it is self describing, but I still don't like the way the same field is used for the different forks. It just feels like we are placing a landmine that we are going to forget about and step on in the future.... Cheers, Dave.
On 28 Sep 2021 at 05:16, Dave Chinner wrote: > On Thu, Sep 16, 2021 at 03:36:42PM +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. >> >> This change is done to enable a future commit to introduce a new 64-bit extent >> counter field. >> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> >> --- >> fs/xfs/libxfs/xfs_format.h | 8 ++++---- >> fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- >> 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 dba868f2c3e3..87c927d912f6 100644 >> --- a/fs/xfs/libxfs/xfs_format.h >> +++ b/fs/xfs/libxfs/xfs_format.h >> @@ -802,8 +802,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*/ > > > Hmmm. Having the same field in the inode hold the extent count > for different inode forks based on a bit in the superblock means the > on-disk inode format is not self describing. i.e. we can't decode > the on-disk contents of an inode correctly without knowing whether a > specific feature bit is set in the superblock or not. > > Right now we don't have use external configs to decode the inode. > Feature level conditional fields are determined by inode version, > not superblock bits. Optional feature fields easy to deal with - > zero if the feature is not in use, otherwise we assume it is in use > and can validity check it appropriately. IOWs, we don't need > to look at sb feature bits to decode and validate inode fields. > > This change means that we can't determine if the extent counts are > correct just by looking at the on-disk inode. If we just have > di_nextents32 set to a non-zero value, does that mean we should have > data fork extents or attribute fork extents present? > > Just looking at whether the attr fork is initialised is not > sufficient - it can be initialised with zero attr fork extents > present. We can't look at the literal area contents, either, > because we don't zero that when we shrink it. We can't look at > di_nblocks, because that counts both attr and data for blocks. We > can't look at di_size, because we can have data extents beyond EOF > and hence a size of zero doesn't mean the data fork is empty. > > So if both forks are in extent format, they could be either both > empty, both contain extents or only one fork contains extents but we > can't tell which state is the correct one. Hence > if di_nextents64 if zero, we don't know if di_nextents32 is a count > of attribute extents or data extents without first looking at the > superblock feature bit to determine if di_nextents64 is in use or > not. The inode format is not self describing anymore. > > When XFS introduced 32 bit link counts, the inode version was bumped > from v1 to v2 because it redefined fields in the inode structure > similar to this proposal[1]. The verison number was then used to > determine if the inode was in old or new format - it was a self > describing format change. Hence If we are going to redefine > di_nextents to be able to hold either data fork extent count (old > format) or attr fork extent count (new format) we really need to > bump the inode version so that we can discriminate between the two > inode formats just by looking at the inode itself. > > If we don't want to bump the version, then we need to do something > like: > > - __be32 di_nextents; /* number of extents in data fork */ > - __be16 di_anextents; /* number of extents in attribute fork*/ > + __be32 di_nextents_old;/* old number of extents in data fork */ > + __be16 di_anextents_old;/* old number of extents in attribute fork*/ > ..... > - __u8 di_pad2[12]; > + __be64 di_nextents; /* number of extents in data fork */ > + __be32 di_anextents; /* number of extents in attribute fork*/ > + __u8 di_pad2[4]; > > So that there is no ambiguity in the on-disk format between the two > formats - if one set is non-zero, the other set must be zero in this > sort of setup. > > However, I think that redefining the fields and bumping the inode > version is the better long term strategy, as it allows future reuse > of the di_anextents_old field, and it uses less of the small amount > of unused padding we have remaining in the on-disk inode core. > > At which point, the feature bit in the superblock becomes "has v4 > inodes", not "has big extent counts". We then use v4 inode format in > memory for everything (i.e. 64 bit extent counts) and convert > to/from the ondisk format at IO time like we do with v1/v2 inodes. > > Thoughts? The patch "xfs: Extend per-inode extent counter widths" (which appears later in the series) adds the new per-inode flag XFS_DIFLAG2_NREXT64. This flag is set on inodes which use 64-bit data fork extent counter and 32-bit attr fork extent counter fields. Verifiers can check for the presence/absence of this flag to determine which extent counter fields to use for verification of an xfs_dinode structure. Hence, XFS_DIFLAG2_NREXT64 flag should be sufficient for maintaining the self describing nature of XFS inodes right? > > -Dave. > > [1] The change to v2 inodes back in 1995 removed the filesystem UUID > from the inode and was replaced with a 32 bit link counter, a project ID > value and padding: > > @@ -36,10 +38,12 @@ typedef struct xfs_dinode_core > __uint16_t di_mode; /* mode and type of file */ > __int8_t di_version; /* inode version */ > __int8_t di_format; /* format of di_c data */ > - __uint16_t di_nlink; /* number of links to file */ > + __uint16_t di_onlink; /* old number of links to file */ > __uint32_t di_uid; /* owner's user id */ > __uint32_t di_gid; /* owner's group id */ > - uuid_t di_uuid; /* file unique id */ > + __uint32_t di_nlink; /* number of links to file */ > + __uint16_t di_projid; /* owner's project id */ > + __uint8_t di_pad[10]; /* unused, zeroed space */ > 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 */ > @@ -81,7 +85,13 @@ typedef struct xfs_dinode > > it was the redefinition of the di_uuid variable space that required > the bumping of the inode version...
On 28 Sep 2021 at 09:34, Dave Chinner wrote: > On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote: >> On Thu, Sep 16, 2021 at 03:36:42PM +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. >> > >> > This change is done to enable a future commit to introduce a new 64-bit extent >> > counter field. >> > >> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> >> > --- >> > fs/xfs/libxfs/xfs_format.h | 8 ++++---- >> > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- >> > 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 dba868f2c3e3..87c927d912f6 100644 >> > --- a/fs/xfs/libxfs/xfs_format.h >> > +++ b/fs/xfs/libxfs/xfs_format.h >> > @@ -802,8 +802,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*/ >> >> >> Hmmm. Having the same field in the inode hold the extent count >> for different inode forks based on a bit in the superblock means the >> on-disk inode format is not self describing. i.e. we can't decode >> the on-disk contents of an inode correctly without knowing whether a >> specific feature bit is set in the superblock or not. > > Hmmmm - I just realised that there is an inode flag that indicates > the format is different. It's jsut that most of the code doing > conditional behaviour is using the superblock flag, not the inode > flag as the conditional. > > So it is self describing, but I still don't like the way the same > field is used for the different forks. It just feels like we are > placing a landmine that we are going to forget about and step > on in the future.... > Sorry, I missed this response from you. I agree with your suggestion. I will use the inode version number to help in deciding which extent counter fields are valid for a specific inode.
On Wed, Sep 29, 2021 at 10:33:23PM +0530, Chandan Babu R wrote: > On 28 Sep 2021 at 09:34, Dave Chinner wrote: > > On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote: > >> On Thu, Sep 16, 2021 at 03:36:42PM +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. > >> > > >> > This change is done to enable a future commit to introduce a new 64-bit extent > >> > counter field. > >> > > >> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> > >> > --- > >> > fs/xfs/libxfs/xfs_format.h | 8 ++++---- > >> > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- > >> > 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 dba868f2c3e3..87c927d912f6 100644 > >> > --- a/fs/xfs/libxfs/xfs_format.h > >> > +++ b/fs/xfs/libxfs/xfs_format.h > >> > @@ -802,8 +802,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*/ > >> > >> > >> Hmmm. Having the same field in the inode hold the extent count > >> for different inode forks based on a bit in the superblock means the > >> on-disk inode format is not self describing. i.e. we can't decode > >> the on-disk contents of an inode correctly without knowing whether a > >> specific feature bit is set in the superblock or not. > > > > Hmmmm - I just realised that there is an inode flag that indicates > > the format is different. It's jsut that most of the code doing > > conditional behaviour is using the superblock flag, not the inode > > flag as the conditional. > > > > So it is self describing, but I still don't like the way the same > > field is used for the different forks. It just feels like we are > > placing a landmine that we are going to forget about and step > > on in the future.... > > > > Sorry, I missed this response from you. > > I agree with your suggestion. I will use the inode version number to help in > deciding which extent counter fields are valid for a specific inode. No, don't do something I suggested with a flawed understanding of the code. Just because *I* suggest something, it means you have to make that change. That is reacting to *who* said something, not *what was said*. So, I may have reservations about the way the storage definitions are being redefined, but if I had a valid, technical argument I could give right now I would have said so directly. I can't put my finger on why this worries me in this case but didn't for something like, say, the BIGTIME feature which redefined the contents of various fields in the inode. IOWs, I haven't really had time to think and go back over the rest of the patchset since I realised my mistake and determine if that changes what I think about this, so don't go turning the patchset upside just because *I suggested something*. Think critically about what is said and respond to that, not look at who said it and respond based on their reputation. Cheers, Dave.
On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: > On Wed, Sep 29, 2021 at 10:33:23PM +0530, Chandan Babu R wrote: > > On 28 Sep 2021 at 09:34, Dave Chinner wrote: > > > On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote: > > >> On Thu, Sep 16, 2021 at 03:36:42PM +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. > > >> > > > >> > This change is done to enable a future commit to introduce a new 64-bit extent > > >> > counter field. > > >> > > > >> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> > > >> > --- > > >> > fs/xfs/libxfs/xfs_format.h | 8 ++++---- > > >> > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- > > >> > 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 dba868f2c3e3..87c927d912f6 100644 > > >> > --- a/fs/xfs/libxfs/xfs_format.h > > >> > +++ b/fs/xfs/libxfs/xfs_format.h > > >> > @@ -802,8 +802,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*/ > > >> > > >> > > >> Hmmm. Having the same field in the inode hold the extent count > > >> for different inode forks based on a bit in the superblock means the > > >> on-disk inode format is not self describing. i.e. we can't decode > > >> the on-disk contents of an inode correctly without knowing whether a > > >> specific feature bit is set in the superblock or not. > > > > > > Hmmmm - I just realised that there is an inode flag that indicates > > > the format is different. It's jsut that most of the code doing > > > conditional behaviour is using the superblock flag, not the inode > > > flag as the conditional. > > > > > > So it is self describing, but I still don't like the way the same > > > field is used for the different forks. It just feels like we are > > > placing a landmine that we are going to forget about and step > > > on in the future.... > > > > > > > Sorry, I missed this response from you. > > > > I agree with your suggestion. I will use the inode version number to help in > > deciding which extent counter fields are valid for a specific inode. > > No, don't do something I suggested with a flawed understanding of > the code. > > Just because *I* suggest something, it means you have to make that > change. That is reacting to *who* said something, not *what was > said*. > > So, I may have reservations about the way the storage definitions > are being redefined, but if I had a valid, technical argument I > could give right now I would have said so directly. I can't put my > finger on why this worries me in this case but didn't for something > like, say, the BIGTIME feature which redefined the contents of > various fields in the inode. > > IOWs, I haven't really had time to think and go back over the rest > of the patchset since I realised my mistake and determine if that > changes what I think about this, so don't go turning the patchset > upside just because *I suggested something*. So, looking over the patchset more, I think I understand my feeling a bit better. Inconsistency is a big part of it. The in-memory extent counts are held in the struct xfs_inode_fork and not the inode. The type is a xfs_extcnt_t - it's not a size dependent type. Indeed, there are actually no users of the xfs_aextcnt_t variable in XFS at all any more. It should be removed. What this means is that in-memory inode extent counting just doesn't discriminate between inode fork types. They are all 64 bit counters, and all the limits applied to them should be 64 bit types. Even the checks for overflow are abstracted away by xfs_iext_count_may_overflow(), so none of the extent manipulation code has any idea there are different types and limits in the on-disk format. That's good. The only place the actual type matters is when looking at the raw disk inode and, unfortunately, that's where it gets messy. Anything accessing the on-disk inode directly has to look at inode version number, and an inode feature flag to interpret the inode format correctly. That format is then reflected in an in-memory inode feature flag, and then there's the superblock feature flag on top of that to indicate that there are NREXT64 format inodes in the filesystem. Then there's implied dynamic upgrades of the on-disk inode format. We see that being implied in xfs_inode_to_disk_iext_counters() and xfs_trans_log_inode() but the filesystem format can't be changed dynamically. i.e. we can't create new NREXT64 inodes if the superblock flag is not set, so there is no code in this patchset that I can see that provides a trigger for a dynamic upgrade to start. IOWs, the filesystem has to be taken offline to change the superblock feature bit, and the setup of the default NREXT64 inode flag at mount time re-inforces this. With this in mind, I started to see inconsistent use of inode feature flag vs superblock feature flag to determine on-disk inode extent count limits. e.g. look at xfs_iext_count_may_overflow() and xfs_iext_max_nextents(). Both of these are determining the maximum number of extents that are valid for an inode, and they look at the -superblock feature bit- to determine the limits. This only works if all inodes in the filesystem have the same format, which is not true if we are doing dynamic upgrades of the inode features. The most obvious case here is that scrub needs to determine the layout and limits based on the current feature bits in the inode, not the superblock feature bit. Then we have to look at how the upgrade is performed - by changing the in-memory inode flag during xfs_trans_log_inode() when the inode is dirtied. When we are modifying the inode for extent allocation, we check the extent count limits on the inode *before* we dirty the inode. Hence the only way an "upgrade at overflow thresholds" can actually work is if we don't use the inode flag for determining limits but instead use the sueprblock feature bit limits. But as I've already pointed out, that leads to other problems. When we are converting an inode format, we currently do it when the inode is first brought into memory and read from disk (i.e. xfs_inode_from_disk()). We do the full conversion at this point in time, such that if the inode is dirtied in memory all the correct behaviour for the new format occurs and the writeback is done in the new format. This would allow xfs_iext_count_may_overflow/xfs_iext_max_nextents to actually return the correct limits for the inode as it is being modified and not have to rely on superblock feature bits. If the inode is not being modified, then the in-memory format changes are discarded when the inode is reclaimed from memory and nothing changes on disk. This means that once we've read the inode in from disk and set up ip->i_diflags2 according to the superblock feature bit, we can use the in-memory inode flag -everywhere- we need to find and/or check limits during modifications. Yes, I know that the BIGTIME upgrade path does this, but that doesn't have limits that prevent modifications from taking place before we can log the inode and set the BIGTIME flag.... So, yeah, I think the biggest problem I've been having is that the way the inode flags, the limits and the on-disk format is juggled has resulted in me taking some time to understand where the problems lie. Cleaning up the initialisation, conversion and consistency in using the inode flags rather thant he superblock flag will go a long way to addressing my concerns --- FWIW, I also think doing something like this would help make the code be easier to read and confirm that it is obviously correct when reading it: __be32 di_gid; /* owner's group id */ __be32 di_nlink; /* number of links to file */ __be16 di_projid_lo; /* lower part of owner's project id */ __be16 di_projid_hi; /* higher part owner's project id */ union { __be64 di_big_dextcnt; /* NREXT64 data extents */ __u8 di_v3_pad[8]; /* !NREXT64 V3 inode zeroed space */ struct { __u8 di_v2_pad[6]; /* V2 inode zeroed space */ __be16 di_flushiter; /* V2 inode 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 */ union { struct { __be32 di_big_aextcnt; /* NREXT64 attr extents */ __be16 di_nrext64_pad; /* NREXT64 unused, zero */ }; struct { __be32 di_nextents; /* !NREXT64 data extents */ __be16 di_anextents; /* !NREXT64 attr extents */ } } __u8 di_forkoff; /* attr fork offs, <<3 for 64b align */ __s8 di_aformat; /* format of attr fork's data */ ... Then we get something like: static inline void xfs_inode_to_disk_iext_counters( struct xfs_inode *ip, struct xfs_dinode *to) { if (xfs_inode_has_nrext64(ip)) { to->di_big_dextent_cnt = cpu_to_be64(xfs_ifork_nextents(&ip->i_df)); to->di_big_anextents = cpu_to_be32(xfs_ifork_nextents(ip->i_afp)); to->di_nrext64_pad = 0; } else { to->di_nextents = cpu_to_be32(xfs_ifork_nextents(&ip->i_df)); to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp)); } } This is now obvious that we are writing to the correct fields in the inode for the feature bits that are set, and we don't need to zero the di_big_dextcnt field because that's been taken care of by the existing di_v2_pad/flushiter zeroing. That bit could probably be improved by unwinding and open coding this in xfs_inode_to_disk(), but I think what I'm proposing should be obvious now... Cheers, Dave.
On 30 Sep 2021 at 10:01, Dave Chinner wrote: > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: >> On Wed, Sep 29, 2021 at 10:33:23PM +0530, Chandan Babu R wrote: >> > On 28 Sep 2021 at 09:34, Dave Chinner wrote: >> > > On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote: >> > >> On Thu, Sep 16, 2021 at 03:36:42PM +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. >> > >> > >> > >> > This change is done to enable a future commit to introduce a new 64-bit extent >> > >> > counter field. >> > >> > >> > >> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> >> > >> > --- >> > >> > fs/xfs/libxfs/xfs_format.h | 8 ++++---- >> > >> > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- >> > >> > 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 dba868f2c3e3..87c927d912f6 100644 >> > >> > --- a/fs/xfs/libxfs/xfs_format.h >> > >> > +++ b/fs/xfs/libxfs/xfs_format.h >> > >> > @@ -802,8 +802,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*/ >> > >> >> > >> >> > >> Hmmm. Having the same field in the inode hold the extent count >> > >> for different inode forks based on a bit in the superblock means the >> > >> on-disk inode format is not self describing. i.e. we can't decode >> > >> the on-disk contents of an inode correctly without knowing whether a >> > >> specific feature bit is set in the superblock or not. >> > > >> > > Hmmmm - I just realised that there is an inode flag that indicates >> > > the format is different. It's jsut that most of the code doing >> > > conditional behaviour is using the superblock flag, not the inode >> > > flag as the conditional. >> > > >> > > So it is self describing, but I still don't like the way the same >> > > field is used for the different forks. It just feels like we are >> > > placing a landmine that we are going to forget about and step >> > > on in the future.... >> > > >> > >> > Sorry, I missed this response from you. >> > >> > I agree with your suggestion. I will use the inode version number to help in >> > deciding which extent counter fields are valid for a specific inode. >> >> No, don't do something I suggested with a flawed understanding of >> the code. >> >> Just because *I* suggest something, it means you have to make that >> change. That is reacting to *who* said something, not *what was >> said*. >> >> So, I may have reservations about the way the storage definitions >> are being redefined, but if I had a valid, technical argument I >> could give right now I would have said so directly. I can't put my >> finger on why this worries me in this case but didn't for something >> like, say, the BIGTIME feature which redefined the contents of >> various fields in the inode. >> >> IOWs, I haven't really had time to think and go back over the rest >> of the patchset since I realised my mistake and determine if that >> changes what I think about this, so don't go turning the patchset >> upside just because *I suggested something*. > > So, looking over the patchset more, I think I understand my feeling > a bit better. Inconsistency is a big part of it. > > The in-memory extent counts are held in the struct xfs_inode_fork > and not the inode. The type is a xfs_extcnt_t - it's not a size > dependent type. Indeed, there are actually no users of the > xfs_aextcnt_t variable in XFS at all any more. It should be removed. > > What this means is that in-memory inode extent counting just doesn't > discriminate between inode fork types. They are all 64 bit counters, > and all the limits applied to them should be 64 bit types. Even the > checks for overflow are abstracted away by > xfs_iext_count_may_overflow(), so none of the extent manipulation > code has any idea there are different types and limits in the > on-disk format. > > That's good. > > The only place the actual type matters is when looking at the raw > disk inode and, unfortunately, that's where it gets messy. Anything > accessing the on-disk inode directly has to look at inode version > number, and an inode feature flag to interpret the inode format > correctly. That format is then reflected in an in-memory inode > feature flag, and then there's the superblock feature flag on top of > that to indicate that there are NREXT64 format inodes in the > filesystem. > > Then there's implied dynamic upgrades of the on-disk inode format. > We see that being implied in xfs_inode_to_disk_iext_counters() and > xfs_trans_log_inode() but the filesystem format can't be changed > dynamically. i.e. we can't create new NREXT64 inodes if the > superblock flag is not set, so there is no code in this patchset > that I can see that provides a trigger for a dynamic upgrade to > start. IOWs, the filesystem has to be taken offline to change the > superblock feature bit, and the setup of the default NREXT64 inode > flag at mount time re-inforces this. > > With this in mind, I started to see inconsistent use of inode > feature flag vs superblock feature flag to determine on-disk inode > extent count limits. e.g. look at xfs_iext_count_may_overflow() and > xfs_iext_max_nextents(). Both of these are determining the maximum > number of extents that are valid for an inode, and they look at the > -superblock feature bit- to determine the limits. > > This only works if all inodes in the filesystem have the same > format, which is not true if we are doing dynamic upgrades of the > inode features. The most obvious case here is that scrub needs to > determine the layout and limits based on the current feature bits in > the inode, not the superblock feature bit. > > Then we have to look at how the upgrade is performed - by changing > the in-memory inode flag during xfs_trans_log_inode() when the inode > is dirtied. When we are modifying the inode for extent allocation, > we check the extent count limits on the inode *before* we dirty the > inode. Hence the only way an "upgrade at overflow thresholds" can > actually work is if we don't use the inode flag for determining > limits but instead use the sueprblock feature bit limits. But as > I've already pointed out, that leads to other problems. > > When we are converting an inode format, we currently do it when the > inode is first brought into memory and read from disk (i.e. > xfs_inode_from_disk()). We do the full conversion at this point in > time, such that if the inode is dirtied in memory all the correct > behaviour for the new format occurs and the writeback is done in the > new format. > > This would allow xfs_iext_count_may_overflow/xfs_iext_max_nextents > to actually return the correct limits for the inode as it is being > modified and not have to rely on superblock feature bits. If the > inode is not being modified, then the in-memory format changes are > discarded when the inode is reclaimed from memory and nothing > changes on disk. > > This means that once we've read the inode in from disk and set up > ip->i_diflags2 according to the superblock feature bit, we can use > the in-memory inode flag -everywhere- we need to find and/or check > limits during modifications. Yes, I know that the BIGTIME upgrade > path does this, but that doesn't have limits that prevent > modifications from taking place before we can log the inode and set > the BIGTIME flag.... > Ok. The above solution looks logically correct. I haven't been able to come up with a scenario where the solution wouldn't work. I will implement it and see if anything breaks. > So, yeah, I think the biggest problem I've been having is that the > way the inode flags, the limits and the on-disk format is juggled > has resulted in me taking some time to understand where the problems > lie. Cleaning up the initialisation, conversion and consistency in > using the inode flags rather thant he superblock flag will go a long > way to addressing my concerns > > --- > > FWIW, I also think doing something like this would help make the > code be easier to read and confirm that it is obviously correct when > reading it: > > __be32 di_gid; /* owner's group id */ > __be32 di_nlink; /* number of links to file */ > __be16 di_projid_lo; /* lower part of owner's project id */ > __be16 di_projid_hi; /* higher part owner's project id */ > union { > __be64 di_big_dextcnt; /* NREXT64 data extents */ > __u8 di_v3_pad[8]; /* !NREXT64 V3 inode zeroed space */ > struct { > __u8 di_v2_pad[6]; /* V2 inode zeroed space */ > __be16 di_flushiter; /* V2 inode 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 */ > union { > struct { > __be32 di_big_aextcnt; /* NREXT64 attr extents */ > __be16 di_nrext64_pad; /* NREXT64 unused, zero */ > }; > struct { > __be32 di_nextents; /* !NREXT64 data extents */ > __be16 di_anextents; /* !NREXT64 attr extents */ > } > } > __u8 di_forkoff; /* attr fork offs, <<3 for 64b align */ > __s8 di_aformat; /* format of attr fork's data */ > ... > > Then we get something like: > > static inline void > xfs_inode_to_disk_iext_counters( > struct xfs_inode *ip, > struct xfs_dinode *to) > { > if (xfs_inode_has_nrext64(ip)) { > to->di_big_dextent_cnt = cpu_to_be64(xfs_ifork_nextents(&ip->i_df)); > to->di_big_anextents = cpu_to_be32(xfs_ifork_nextents(ip->i_afp)); > to->di_nrext64_pad = 0; > } else { > to->di_nextents = cpu_to_be32(xfs_ifork_nextents(&ip->i_df)); > to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp)); > } > } > > This is now obvious that we are writing to the correct fields > in the inode for the feature bits that are set, and we don't need > to zero the di_big_dextcnt field because that's been taken care of > by the existing di_v2_pad/flushiter zeroing. That bit could probably > be improved by unwinding and open coding this in xfs_inode_to_disk(), > but I think what I'm proposing should be obvious now... > Yes, the explaination provided by you is very clear. I will implement these suggestions.
On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote: > On 30 Sep 2021 at 10:01, Dave Chinner wrote: > > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: > >> On Wed, Sep 29, 2021 at 10:33:23PM +0530, Chandan Babu R wrote: > >> > On 28 Sep 2021 at 09:34, Dave Chinner wrote: > >> > > On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote: > >> > >> On Thu, Sep 16, 2021 at 03:36:42PM +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. > >> > >> > > >> > >> > This change is done to enable a future commit to introduce a new 64-bit extent > >> > >> > counter field. > >> > >> > > >> > >> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> > >> > >> > --- > >> > >> > fs/xfs/libxfs/xfs_format.h | 8 ++++---- > >> > >> > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- > >> > >> > 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 dba868f2c3e3..87c927d912f6 100644 > >> > >> > --- a/fs/xfs/libxfs/xfs_format.h > >> > >> > +++ b/fs/xfs/libxfs/xfs_format.h > >> > >> > @@ -802,8 +802,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*/ > >> > >> > >> > >> > >> > >> Hmmm. Having the same field in the inode hold the extent count > >> > >> for different inode forks based on a bit in the superblock means the > >> > >> on-disk inode format is not self describing. i.e. we can't decode > >> > >> the on-disk contents of an inode correctly without knowing whether a > >> > >> specific feature bit is set in the superblock or not. > >> > > > >> > > Hmmmm - I just realised that there is an inode flag that indicates > >> > > the format is different. It's jsut that most of the code doing > >> > > conditional behaviour is using the superblock flag, not the inode > >> > > flag as the conditional. > >> > > > >> > > So it is self describing, but I still don't like the way the same > >> > > field is used for the different forks. It just feels like we are > >> > > placing a landmine that we are going to forget about and step > >> > > on in the future.... > >> > > > >> > > >> > Sorry, I missed this response from you. > >> > > >> > I agree with your suggestion. I will use the inode version number to help in > >> > deciding which extent counter fields are valid for a specific inode. > >> > >> No, don't do something I suggested with a flawed understanding of > >> the code. > >> > >> Just because *I* suggest something, it means you have to make that > >> change. That is reacting to *who* said something, not *what was > >> said*. > >> > >> So, I may have reservations about the way the storage definitions > >> are being redefined, but if I had a valid, technical argument I > >> could give right now I would have said so directly. I can't put my > >> finger on why this worries me in this case but didn't for something > >> like, say, the BIGTIME feature which redefined the contents of > >> various fields in the inode. > >> > >> IOWs, I haven't really had time to think and go back over the rest > >> of the patchset since I realised my mistake and determine if that > >> changes what I think about this, so don't go turning the patchset > >> upside just because *I suggested something*. > > > > So, looking over the patchset more, I think I understand my feeling > > a bit better. Inconsistency is a big part of it. > > > > The in-memory extent counts are held in the struct xfs_inode_fork > > and not the inode. The type is a xfs_extcnt_t - it's not a size > > dependent type. Indeed, there are actually no users of the > > xfs_aextcnt_t variable in XFS at all any more. It should be removed. > > > > What this means is that in-memory inode extent counting just doesn't > > discriminate between inode fork types. They are all 64 bit counters, > > and all the limits applied to them should be 64 bit types. Even the > > checks for overflow are abstracted away by > > xfs_iext_count_may_overflow(), so none of the extent manipulation > > code has any idea there are different types and limits in the > > on-disk format. > > > > That's good. > > > > The only place the actual type matters is when looking at the raw > > disk inode and, unfortunately, that's where it gets messy. Anything > > accessing the on-disk inode directly has to look at inode version > > number, and an inode feature flag to interpret the inode format > > correctly. That format is then reflected in an in-memory inode > > feature flag, and then there's the superblock feature flag on top of > > that to indicate that there are NREXT64 format inodes in the > > filesystem. > > > > Then there's implied dynamic upgrades of the on-disk inode format. > > We see that being implied in xfs_inode_to_disk_iext_counters() and > > xfs_trans_log_inode() but the filesystem format can't be changed > > dynamically. i.e. we can't create new NREXT64 inodes if the > > superblock flag is not set, so there is no code in this patchset > > that I can see that provides a trigger for a dynamic upgrade to > > start. IOWs, the filesystem has to be taken offline to change the > > superblock feature bit, and the setup of the default NREXT64 inode > > flag at mount time re-inforces this. > > > > With this in mind, I started to see inconsistent use of inode > > feature flag vs superblock feature flag to determine on-disk inode > > extent count limits. e.g. look at xfs_iext_count_may_overflow() and > > xfs_iext_max_nextents(). Both of these are determining the maximum > > number of extents that are valid for an inode, and they look at the > > -superblock feature bit- to determine the limits. > > > > This only works if all inodes in the filesystem have the same > > format, which is not true if we are doing dynamic upgrades of the > > inode features. The most obvious case here is that scrub needs to > > determine the layout and limits based on the current feature bits in > > the inode, not the superblock feature bit. > > > > Then we have to look at how the upgrade is performed - by changing > > the in-memory inode flag during xfs_trans_log_inode() when the inode > > is dirtied. When we are modifying the inode for extent allocation, > > we check the extent count limits on the inode *before* we dirty the > > inode. Hence the only way an "upgrade at overflow thresholds" can > > actually work is if we don't use the inode flag for determining > > limits but instead use the sueprblock feature bit limits. But as > > I've already pointed out, that leads to other problems. > > > > When we are converting an inode format, we currently do it when the > > inode is first brought into memory and read from disk (i.e. > > xfs_inode_from_disk()). We do the full conversion at this point in > > time, such that if the inode is dirtied in memory all the correct > > behaviour for the new format occurs and the writeback is done in the > > new format. > > > > This would allow xfs_iext_count_may_overflow/xfs_iext_max_nextents > > to actually return the correct limits for the inode as it is being > > modified and not have to rely on superblock feature bits. If the > > inode is not being modified, then the in-memory format changes are > > discarded when the inode is reclaimed from memory and nothing > > changes on disk. > > > > This means that once we've read the inode in from disk and set up > > ip->i_diflags2 according to the superblock feature bit, we can use > > the in-memory inode flag -everywhere- we need to find and/or check > > limits during modifications. Yes, I know that the BIGTIME upgrade > > path does this, but that doesn't have limits that prevent > > modifications from taking place before we can log the inode and set > > the BIGTIME flag.... > > > > Ok. The above solution looks logically correct. I haven't been able to come up > with a scenario where the solution wouldn't work. I will implement it and see > if anything breaks. I think I can poke one hole in it - I missed the fact that if we upgrade and inode read time, and then we modify the inode without modifying the inode core (can we even do that - metadata mods should at least change timestamps right?) then we don't log the format change or the NREXT64 inode flag change and they only appear in the on-disk inode at writeback. Log recovery needs to be checked for correct behaviour here. I think that if the inode is in NREXT64 format when read in and the log inode core is not, then the on disk LSN must be more recent than what is being recovered from the log and should be skipped. If NREXT64 is present in the log inode, then we logged the core properly and we just don't care what format is on disk because we replay it into NREXT64 format and write that back. SO I *think* we're ok here, but it needs closer inspection to determine behaviour is actually safe. If it is safe, then maybe in future we can do the same thing for BIGTIME and get that upgrade out of xfs_trans_log_inode() as well.... > > --- > > > > FWIW, I also think doing something like this would help make the > > code be easier to read and confirm that it is obviously correct when > > reading it: > > > > __be32 di_gid; /* owner's group id */ > > __be32 di_nlink; /* number of links to file */ > > __be16 di_projid_lo; /* lower part of owner's project id */ > > __be16 di_projid_hi; /* higher part owner's project id */ > > union { > > __be64 di_big_dextcnt; /* NREXT64 data extents */ > > __u8 di_v3_pad[8]; /* !NREXT64 V3 inode zeroed space */ > > struct { > > __u8 di_v2_pad[6]; /* V2 inode zeroed space */ > > __be16 di_flushiter; /* V2 inode 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 */ > > union { > > struct { > > __be32 di_big_aextcnt; /* NREXT64 attr extents */ > > __be16 di_nrext64_pad; /* NREXT64 unused, zero */ > > }; > > struct { > > __be32 di_nextents; /* !NREXT64 data extents */ > > __be16 di_anextents; /* !NREXT64 attr extents */ > > } > > } > > __u8 di_forkoff; /* attr fork offs, <<3 for 64b align */ > > __s8 di_aformat; /* format of attr fork's data */ > > ... > > > > Then we get something like: > > > > static inline void > > xfs_inode_to_disk_iext_counters( > > struct xfs_inode *ip, > > struct xfs_dinode *to) > > { > > if (xfs_inode_has_nrext64(ip)) { > > to->di_big_dextent_cnt = cpu_to_be64(xfs_ifork_nextents(&ip->i_df)); > > to->di_big_anextents = cpu_to_be32(xfs_ifork_nextents(ip->i_afp)); > > to->di_nrext64_pad = 0; > > } else { > > to->di_nextents = cpu_to_be32(xfs_ifork_nextents(&ip->i_df)); > > to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp)); > > } > > } > > > > This is now obvious that we are writing to the correct fields > > in the inode for the feature bits that are set, and we don't need > > to zero the di_big_dextcnt field because that's been taken care of > > by the existing di_v2_pad/flushiter zeroing. That bit could probably > > be improved by unwinding and open coding this in xfs_inode_to_disk(), > > but I think what I'm proposing should be obvious now... > > > > Yes, the explaination provided by you is very clear. I will implement these > suggestions. Don't forget to try to poke holes in it and look for complexity that can be removed before you try to implement or optimise anything. FWIW, the code design concept I'm basing this on is that complexity should be contained within the structures that store the data, rather than be directly exposed to the code that manipulates the data. Cheers, Dave.
On 01 Oct 2021 at 04:25, Dave Chinner wrote: > On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote: >> On 30 Sep 2021 at 10:01, Dave Chinner wrote: >> > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: >> >> On Wed, Sep 29, 2021 at 10:33:23PM +0530, Chandan Babu R wrote: >> >> > On 28 Sep 2021 at 09:34, Dave Chinner wrote: >> >> > > On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote: >> >> > >> On Thu, Sep 16, 2021 at 03:36:42PM +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. >> >> > >> > >> >> > >> > This change is done to enable a future commit to introduce a new 64-bit extent >> >> > >> > counter field. >> >> > >> > >> >> > >> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> >> >> > >> > --- >> >> > >> > fs/xfs/libxfs/xfs_format.h | 8 ++++---- >> >> > >> > fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- >> >> > >> > 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 dba868f2c3e3..87c927d912f6 100644 >> >> > >> > --- a/fs/xfs/libxfs/xfs_format.h >> >> > >> > +++ b/fs/xfs/libxfs/xfs_format.h >> >> > >> > @@ -802,8 +802,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*/ >> >> > >> >> >> > >> >> >> > >> Hmmm. Having the same field in the inode hold the extent count >> >> > >> for different inode forks based on a bit in the superblock means the >> >> > >> on-disk inode format is not self describing. i.e. we can't decode >> >> > >> the on-disk contents of an inode correctly without knowing whether a >> >> > >> specific feature bit is set in the superblock or not. >> >> > > >> >> > > Hmmmm - I just realised that there is an inode flag that indicates >> >> > > the format is different. It's jsut that most of the code doing >> >> > > conditional behaviour is using the superblock flag, not the inode >> >> > > flag as the conditional. >> >> > > >> >> > > So it is self describing, but I still don't like the way the same >> >> > > field is used for the different forks. It just feels like we are >> >> > > placing a landmine that we are going to forget about and step >> >> > > on in the future.... >> >> > > >> >> > >> >> > Sorry, I missed this response from you. >> >> > >> >> > I agree with your suggestion. I will use the inode version number to help in >> >> > deciding which extent counter fields are valid for a specific inode. >> >> >> >> No, don't do something I suggested with a flawed understanding of >> >> the code. >> >> >> >> Just because *I* suggest something, it means you have to make that >> >> change. That is reacting to *who* said something, not *what was >> >> said*. >> >> >> >> So, I may have reservations about the way the storage definitions >> >> are being redefined, but if I had a valid, technical argument I >> >> could give right now I would have said so directly. I can't put my >> >> finger on why this worries me in this case but didn't for something >> >> like, say, the BIGTIME feature which redefined the contents of >> >> various fields in the inode. >> >> >> >> IOWs, I haven't really had time to think and go back over the rest >> >> of the patchset since I realised my mistake and determine if that >> >> changes what I think about this, so don't go turning the patchset >> >> upside just because *I suggested something*. >> > >> > So, looking over the patchset more, I think I understand my feeling >> > a bit better. Inconsistency is a big part of it. >> > >> > The in-memory extent counts are held in the struct xfs_inode_fork >> > and not the inode. The type is a xfs_extcnt_t - it's not a size >> > dependent type. Indeed, there are actually no users of the >> > xfs_aextcnt_t variable in XFS at all any more. It should be removed. >> > >> > What this means is that in-memory inode extent counting just doesn't >> > discriminate between inode fork types. They are all 64 bit counters, >> > and all the limits applied to them should be 64 bit types. Even the >> > checks for overflow are abstracted away by >> > xfs_iext_count_may_overflow(), so none of the extent manipulation >> > code has any idea there are different types and limits in the >> > on-disk format. >> > >> > That's good. >> > >> > The only place the actual type matters is when looking at the raw >> > disk inode and, unfortunately, that's where it gets messy. Anything >> > accessing the on-disk inode directly has to look at inode version >> > number, and an inode feature flag to interpret the inode format >> > correctly. That format is then reflected in an in-memory inode >> > feature flag, and then there's the superblock feature flag on top of >> > that to indicate that there are NREXT64 format inodes in the >> > filesystem. >> > >> > Then there's implied dynamic upgrades of the on-disk inode format. >> > We see that being implied in xfs_inode_to_disk_iext_counters() and >> > xfs_trans_log_inode() but the filesystem format can't be changed >> > dynamically. i.e. we can't create new NREXT64 inodes if the >> > superblock flag is not set, so there is no code in this patchset >> > that I can see that provides a trigger for a dynamic upgrade to >> > start. IOWs, the filesystem has to be taken offline to change the >> > superblock feature bit, and the setup of the default NREXT64 inode >> > flag at mount time re-inforces this. >> > >> > With this in mind, I started to see inconsistent use of inode >> > feature flag vs superblock feature flag to determine on-disk inode >> > extent count limits. e.g. look at xfs_iext_count_may_overflow() and >> > xfs_iext_max_nextents(). Both of these are determining the maximum >> > number of extents that are valid for an inode, and they look at the >> > -superblock feature bit- to determine the limits. >> > >> > This only works if all inodes in the filesystem have the same >> > format, which is not true if we are doing dynamic upgrades of the >> > inode features. The most obvious case here is that scrub needs to >> > determine the layout and limits based on the current feature bits in >> > the inode, not the superblock feature bit. >> > >> > Then we have to look at how the upgrade is performed - by changing >> > the in-memory inode flag during xfs_trans_log_inode() when the inode >> > is dirtied. When we are modifying the inode for extent allocation, >> > we check the extent count limits on the inode *before* we dirty the >> > inode. Hence the only way an "upgrade at overflow thresholds" can >> > actually work is if we don't use the inode flag for determining >> > limits but instead use the sueprblock feature bit limits. But as >> > I've already pointed out, that leads to other problems. >> > >> > When we are converting an inode format, we currently do it when the >> > inode is first brought into memory and read from disk (i.e. >> > xfs_inode_from_disk()). We do the full conversion at this point in >> > time, such that if the inode is dirtied in memory all the correct >> > behaviour for the new format occurs and the writeback is done in the >> > new format. >> > >> > This would allow xfs_iext_count_may_overflow/xfs_iext_max_nextents >> > to actually return the correct limits for the inode as it is being >> > modified and not have to rely on superblock feature bits. If the >> > inode is not being modified, then the in-memory format changes are >> > discarded when the inode is reclaimed from memory and nothing >> > changes on disk. >> > >> > This means that once we've read the inode in from disk and set up >> > ip->i_diflags2 according to the superblock feature bit, we can use >> > the in-memory inode flag -everywhere- we need to find and/or check >> > limits during modifications. Yes, I know that the BIGTIME upgrade >> > path does this, but that doesn't have limits that prevent >> > modifications from taking place before we can log the inode and set >> > the BIGTIME flag.... >> > >> >> Ok. The above solution looks logically correct. I haven't been able to come up >> with a scenario where the solution wouldn't work. I will implement it and see >> if anything breaks. > > I think I can poke one hole in it - I missed the fact that if we > upgrade and inode read time, and then we modify the inode without > modifying the inode core (can we even do that - metadata mods should > at least change timestamps right?) then we don't log the format > change or the NREXT64 inode flag change and they only appear in the > on-disk inode at writeback. > > Log recovery needs to be checked for correct behaviour here. I think > that if the inode is in NREXT64 format when read in and the log > inode core is not, then the on disk LSN must be more recent than > what is being recovered from the log and should be skipped. If > NREXT64 is present in the log inode, then we logged the core > properly and we just don't care what format is on disk because we > replay it into NREXT64 format and write that back. xfs_inode_item_format() logs the inode core regardless of whether XFS_ILOG_CORE flag is set in xfs_inode_log_item->ili_fields. Hence, setting the NREXT64 bit in xfs_dinode->di_flags2 just after reading an inode from disk should not result in a scenario where the corresponding xfs_log_dinode->di_flags2 will not have NREXT64 bit set. If log recovery comes across a log inode with NREXT64 bit set in its di_flags2 field, then we can safely conclude that the ondisk inode has to be updated to reflect this change i.e. there is no need to compare LSNs of the checkpoint transaction being replayed and that of the disk inode. > > SO I *think* we're ok here, but it needs closer inspection to > determine behaviour is actually safe. If it is safe, then maybe in > future we can do the same thing for BIGTIME and get that upgrade out > of xfs_trans_log_inode() as well.... > >> > --- >> > >> > FWIW, I also think doing something like this would help make the >> > code be easier to read and confirm that it is obviously correct when >> > reading it: >> > >> > __be32 di_gid; /* owner's group id */ >> > __be32 di_nlink; /* number of links to file */ >> > __be16 di_projid_lo; /* lower part of owner's project id */ >> > __be16 di_projid_hi; /* higher part owner's project id */ >> > union { >> > __be64 di_big_dextcnt; /* NREXT64 data extents */ >> > __u8 di_v3_pad[8]; /* !NREXT64 V3 inode zeroed space */ >> > struct { >> > __u8 di_v2_pad[6]; /* V2 inode zeroed space */ >> > __be16 di_flushiter; /* V2 inode 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 */ >> > union { >> > struct { >> > __be32 di_big_aextcnt; /* NREXT64 attr extents */ >> > __be16 di_nrext64_pad; /* NREXT64 unused, zero */ >> > }; >> > struct { >> > __be32 di_nextents; /* !NREXT64 data extents */ >> > __be16 di_anextents; /* !NREXT64 attr extents */ >> > } >> > } The two structures above result in padding and hence result in a hole being introduced. The entire union above can be replaced with the following, union { __be32 di_big_aextcnt; /* NREXT64 attr extents */ __be32 di_nextents; /* !NREXT64 data extents */ }; union { __be16 di_nrext64_pad; /* NREXT64 unused, zero */ __be16 di_anextents; /* !NREXT64 attr extents */ }; >> > __u8 di_forkoff; /* attr fork offs, <<3 for 64b align */ >> > __s8 di_aformat; /* format of attr fork's data */ >> > ... >> > >> > Then we get something like: >> > >> > static inline void >> > xfs_inode_to_disk_iext_counters( >> > struct xfs_inode *ip, >> > struct xfs_dinode *to) >> > { >> > if (xfs_inode_has_nrext64(ip)) { >> > to->di_big_dextent_cnt = cpu_to_be64(xfs_ifork_nextents(&ip->i_df)); >> > to->di_big_anextents = cpu_to_be32(xfs_ifork_nextents(ip->i_afp)); >> > to->di_nrext64_pad = 0; >> > } else { >> > to->di_nextents = cpu_to_be32(xfs_ifork_nextents(&ip->i_df)); >> > to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp)); >> > } >> > } >> > >> > This is now obvious that we are writing to the correct fields >> > in the inode for the feature bits that are set, and we don't need >> > to zero the di_big_dextcnt field because that's been taken care of >> > by the existing di_v2_pad/flushiter zeroing. That bit could probably >> > be improved by unwinding and open coding this in xfs_inode_to_disk(), >> > but I think what I'm proposing should be obvious now... >> > >> >> Yes, the explaination provided by you is very clear. I will implement these >> suggestions. > > Don't forget to try to poke holes in it and look for complexity that > can be removed before you try to implement or optimise anything. > > FWIW, the code design concept I'm basing this on is that complexity > should be contained within the structures that store the data, > rather than be directly exposed to the code that manipulates the > data. > To summarize the design, - We need both the per-inode flag (for satisfying the requirement of self-describing metadata) and superblock flag (since an older kernel should not be allowed to mount an fs containing inodes with large extent counters). - When an allocated inode is read from disk, the incore inode's NREXT64 bit in di_flags2 field should be set if the superblock has NREXT64 feature enabled. - Any modification to an inode is guaranteed to cause logging of its di_flags2 field. Hence xfs_iext_max_nextents() can depend on an inode's di_flags2 field's NREXT64 bit to determine the maximum extent count. - Newly allocated inodes will have NREXT64 bit set in di_flags2 field by default due to xfs_ino_geometry->new_diflags2 having XFS_DIFLAG2_NREXT64 bit set. Apart from the regular fs operations, the on-disk format changes introduced above seems to work well with Log replay, Scrub and xfs_repair.
On Thu, Oct 07, 2021 at 04:22:25PM +0530, Chandan Babu R wrote: > On 01 Oct 2021 at 04:25, Dave Chinner wrote: > > On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote: > >> On 30 Sep 2021 at 10:01, Dave Chinner wrote: > >> > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: > >> > > >> > >> Ok. The above solution looks logically correct. I haven't been able to come up > >> with a scenario where the solution wouldn't work. I will implement it and see > >> if anything breaks. > > > > I think I can poke one hole in it - I missed the fact that if we > > upgrade and inode read time, and then we modify the inode without > > modifying the inode core (can we even do that - metadata mods should > > at least change timestamps right?) then we don't log the format > > change or the NREXT64 inode flag change and they only appear in the > > on-disk inode at writeback. > > > > Log recovery needs to be checked for correct behaviour here. I think > > that if the inode is in NREXT64 format when read in and the log > > inode core is not, then the on disk LSN must be more recent than > > what is being recovered from the log and should be skipped. If > > NREXT64 is present in the log inode, then we logged the core > > properly and we just don't care what format is on disk because we > > replay it into NREXT64 format and write that back. > > xfs_inode_item_format() logs the inode core regardless of whether > XFS_ILOG_CORE flag is set in xfs_inode_log_item->ili_fields. Hence, setting > the NREXT64 bit in xfs_dinode->di_flags2 just after reading an inode from disk > should not result in a scenario where the corresponding > xfs_log_dinode->di_flags2 will not have NREXT64 bit set. Except that log recovery might be replaying lots of indoe changes such as: log inode commit A log inode commit B log inode set NREXT64 commit C writeback inode <crash before log tail moves> Recovery will then replay commit A, B and C, in which case we *must not recover the log inode* in commit A or B because the LSN in the on-disk inode points at commit C. Hence replaying A or B will result in the on-disk inode going backwards in time and hence resulting in an inconsistent state on disk until commit C is recovered. > i.e. there is no need to compare LSNs of the checkpoint > transaction being replayed and that of the disk inode. Inncorrect: we -always- have to do this, regardless of the change being made. > If log recovery comes across a log inode with NREXT64 bit set in its di_flags2 > field, then we can safely conclude that the ondisk inode has to be updated to > reflect this change We can't assume that. This makes an assumption that NREXT64 is only ever a one-way transition. There's nothing in the disk format that prevents us from -removing- NREXT64 for inodes that don't need large extent counts. Yes, the -current implementation- does not allow going back to small extent counts, but the on-disk format design still needs to allow for such things to be done as we may need such functionality and flexibility in the on-disk format in the future. Hence we have to ensure that log recovery handles both set and reset transistions from the start. If we don't ensure that log recovery handles reset conditions when we first add the feature bit, then we are going to have to add a log incompat or another feature bit to stop older kernels from trying to recover reset operations. IOWs, the only determining factor as to whether we should replay an inode is the LSN of the on-disk inode vs the LSN of the transaction being replayed. Feature bits in either the on-disk ior log inode are not reliable indicators of whether a dynamically set feature is active or not at the time the inode item is being replayed... > >> > FWIW, I also think doing something like this would help make the > >> > code be easier to read and confirm that it is obviously correct when > >> > reading it: > >> > > >> > __be32 di_gid; /* owner's group id */ > >> > __be32 di_nlink; /* number of links to file */ > >> > __be16 di_projid_lo; /* lower part of owner's project id */ > >> > __be16 di_projid_hi; /* higher part owner's project id */ > >> > union { > >> > __be64 di_big_dextcnt; /* NREXT64 data extents */ > >> > __u8 di_v3_pad[8]; /* !NREXT64 V3 inode zeroed space */ > >> > struct { > >> > __u8 di_v2_pad[6]; /* V2 inode zeroed space */ > >> > __be16 di_flushiter; /* V2 inode 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 */ > >> > union { > >> > struct { > >> > __be32 di_big_aextcnt; /* NREXT64 attr extents */ > >> > __be16 di_nrext64_pad; /* NREXT64 unused, zero */ > >> > }; > >> > struct { > >> > __be32 di_nextents; /* !NREXT64 data extents */ > >> > __be16 di_anextents; /* !NREXT64 attr extents */ > >> > } > >> > } > > The two structures above result in padding and hence result in a hole being > introduced. The entire union above can be replaced with the following, > > union { > __be32 di_big_aextcnt; /* NREXT64 attr extents */ > __be32 di_nextents; /* !NREXT64 data extents */ > }; > union { > __be16 di_nrext64_pad; /* NREXT64 unused, zero */ > __be16 di_anextents; /* !NREXT64 attr extents */ > }; I don't think this makes sense. This groups by field rather than by feature layout. It doesn't make it clear at all that these varaibles both change definition at the same time - they are either {di_nexts, di_anexts} pair or a {di_big_aexts, pad} pair. That's the whole point of using anonymous structs here - it defines and documents the relationship between the layouts when certain features are set rather than relying on people to parse the comments correctly to determine the relationship.... Cheers, Dave.
On 11 Oct 2021 at 03:19, Dave Chinner wrote: > On Thu, Oct 07, 2021 at 04:22:25PM +0530, Chandan Babu R wrote: >> On 01 Oct 2021 at 04:25, Dave Chinner wrote: >> > On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote: >> >> On 30 Sep 2021 at 10:01, Dave Chinner wrote: >> >> > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: >> >> > >> >> >> >> Ok. The above solution looks logically correct. I haven't been able to come up >> >> with a scenario where the solution wouldn't work. I will implement it and see >> >> if anything breaks. >> > >> > I think I can poke one hole in it - I missed the fact that if we >> > upgrade and inode read time, and then we modify the inode without >> > modifying the inode core (can we even do that - metadata mods should >> > at least change timestamps right?) then we don't log the format >> > change or the NREXT64 inode flag change and they only appear in the >> > on-disk inode at writeback. >> > >> > Log recovery needs to be checked for correct behaviour here. I think >> > that if the inode is in NREXT64 format when read in and the log >> > inode core is not, then the on disk LSN must be more recent than >> > what is being recovered from the log and should be skipped. If >> > NREXT64 is present in the log inode, then we logged the core >> > properly and we just don't care what format is on disk because we >> > replay it into NREXT64 format and write that back. >> >> xfs_inode_item_format() logs the inode core regardless of whether >> XFS_ILOG_CORE flag is set in xfs_inode_log_item->ili_fields. Hence, setting >> the NREXT64 bit in xfs_dinode->di_flags2 just after reading an inode from disk >> should not result in a scenario where the corresponding >> xfs_log_dinode->di_flags2 will not have NREXT64 bit set. > > Except that log recovery might be replaying lots of indoe changes > such as: > > log inode > commit A > log inode > commit B > log inode > set NREXT64 > commit C > writeback inode > <crash before log tail moves> > > Recovery will then replay commit A, B and C, in which case we *must > not recover the log inode* in commit A or B because the LSN in the > on-disk inode points at commit C. Hence replaying A or B will result > in the on-disk inode going backwards in time and hence resulting in > an inconsistent state on disk until commit C is recovered. > >> i.e. there is no need to compare LSNs of the checkpoint >> transaction being replayed and that of the disk inode. > > Inncorrect: we -always- have to do this, regardless of the change > being made. > >> If log recovery comes across a log inode with NREXT64 bit set in its di_flags2 >> field, then we can safely conclude that the ondisk inode has to be updated to >> reflect this change > > We can't assume that. This makes an assumption that NREXT64 is > only ever a one-way transition. There's nothing in the disk format that > prevents us from -removing- NREXT64 for inodes that don't need large > extent counts. > > Yes, the -current implementation- does not allow going back to small > extent counts, but the on-disk format design still needs to allow > for such things to be done as we may need such functionality and > flexibility in the on-disk format in the future. > > Hence we have to ensure that log recovery handles both set and reset > transistions from the start. If we don't ensure that log recovery > handles reset conditions when we first add the feature bit, then > we are going to have to add a log incompat or another feature bit > to stop older kernels from trying to recover reset operations. > Ok. I had never considered the possibility of transitioning an inode back into 32-bit data fork extent count format. With this new requirement, I now understand the reasoning behind comparing ondisk inode's LSN and checkpoint transaction's LSN. As you have mentioned earlier, comparing LSNs is required not only for the change introduced in this patch, but also for any other change in value of any of the inode's fields. Without such a comparison, the inode can temporarily end up being in an inconsistent state during log replay. To that end, The following code snippet from xlog_recover_inode_commit_pass2() skips playing back xfs_log_dinode entries when ondisk inode's LSN is greater than checkpoint transaction's LSN, if (dip->di_version >= 3) { xfs_lsn_t lsn = be64_to_cpu(dip->di_lsn); if (lsn && lsn != -1 && XFS_LSN_CMP(lsn, current_lsn) > 0) { trace_xfs_log_recover_inode_skip(log, in_f); error = 0; goto out_owner_change; } } However, if the commits in the sequence below belong to three different checkpoint transactions having the same LSN, log inode commit A log inode commit B set NREXT64 log inode commit C writeback inode <crash before log tail moves> Then the above code snippet won't prevent an inode from becoming temporarily inconsistent due to commits A and B being replayed. To handle this, we should probably go with the additional rule of "Replay log inode if both the log inode and the ondisk inode have the same value for NREXT64 bit". With that additional rule in place, the following sequence will result in a consistent inode state even if all the three checkpoint transactions have the same LSN, log inode commit A set NREXT64 log inode commit B clear NREXT64 log inode commit C writeback inode <crash before log tail moves> i.e. Commit B won't be replayed. Please let me know if my understanding is incorrect. > IOWs, the only determining factor as to whether we should replay an > inode is the LSN of the on-disk inode vs the LSN of the transaction > being replayed. Feature bits in either the on-disk ior log inode are > not reliable indicators of whether a dynamically set feature is > active or not at the time the inode item is being replayed... > >> >> > FWIW, I also think doing something like this would help make the >> >> > code be easier to read and confirm that it is obviously correct when >> >> > reading it: >> >> > >> >> > __be32 di_gid; /* owner's group id */ >> >> > __be32 di_nlink; /* number of links to file */ >> >> > __be16 di_projid_lo; /* lower part of owner's project id */ >> >> > __be16 di_projid_hi; /* higher part owner's project id */ >> >> > union { >> >> > __be64 di_big_dextcnt; /* NREXT64 data extents */ >> >> > __u8 di_v3_pad[8]; /* !NREXT64 V3 inode zeroed space */ >> >> > struct { >> >> > __u8 di_v2_pad[6]; /* V2 inode zeroed space */ >> >> > __be16 di_flushiter; /* V2 inode 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 */ >> >> > union { >> >> > struct { >> >> > __be32 di_big_aextcnt; /* NREXT64 attr extents */ >> >> > __be16 di_nrext64_pad; /* NREXT64 unused, zero */ >> >> > }; >> >> > struct { >> >> > __be32 di_nextents; /* !NREXT64 data extents */ >> >> > __be16 di_anextents; /* !NREXT64 attr extents */ >> >> > } >> >> > } >> >> The two structures above result in padding and hence result in a hole being >> introduced. The entire union above can be replaced with the following, >> >> union { >> __be32 di_big_aextcnt; /* NREXT64 attr extents */ >> __be32 di_nextents; /* !NREXT64 data extents */ >> }; >> union { >> __be16 di_nrext64_pad; /* NREXT64 unused, zero */ >> __be16 di_anextents; /* !NREXT64 attr extents */ >> }; > > I don't think this makes sense. This groups by field rather than > by feature layout. It doesn't make it clear at all that these > varaibles both change definition at the same time - they are either > {di_nexts, di_anexts} pair or a {di_big_aexts, pad} pair. That's the > whole point of using anonymous structs here - it defines and > documents the relationship between the layouts when certain features > are set rather than relying on people to parse the comments > correctly to determine the relationship.... Ok. I will need to check if there are alternative ways of arranging the fields to accomplish the goal stated above. I will think about this and get back as soon as possible.
On Wed, Oct 13, 2021 at 08:14:01PM +0530, Chandan Babu R wrote: > On 11 Oct 2021 at 03:19, Dave Chinner wrote: > > On Thu, Oct 07, 2021 at 04:22:25PM +0530, Chandan Babu R wrote: > >> On 01 Oct 2021 at 04:25, Dave Chinner wrote: > >> > On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote: > >> >> On 30 Sep 2021 at 10:01, Dave Chinner wrote: > >> >> > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: > >> >> > > >> >> > >> >> Ok. The above solution looks logically correct. I haven't been able to come up > >> >> with a scenario where the solution wouldn't work. I will implement it and see > >> >> if anything breaks. > >> > > >> > I think I can poke one hole in it - I missed the fact that if we > >> > upgrade and inode read time, and then we modify the inode without > >> > modifying the inode core (can we even do that - metadata mods should > >> > at least change timestamps right?) then we don't log the format > >> > change or the NREXT64 inode flag change and they only appear in the > >> > on-disk inode at writeback. > >> > > >> > Log recovery needs to be checked for correct behaviour here. I think > >> > that if the inode is in NREXT64 format when read in and the log > >> > inode core is not, then the on disk LSN must be more recent than > >> > what is being recovered from the log and should be skipped. If > >> > NREXT64 is present in the log inode, then we logged the core > >> > properly and we just don't care what format is on disk because we > >> > replay it into NREXT64 format and write that back. > >> > >> xfs_inode_item_format() logs the inode core regardless of whether > >> XFS_ILOG_CORE flag is set in xfs_inode_log_item->ili_fields. Hence, setting > >> the NREXT64 bit in xfs_dinode->di_flags2 just after reading an inode from disk > >> should not result in a scenario where the corresponding > >> xfs_log_dinode->di_flags2 will not have NREXT64 bit set. > > > > Except that log recovery might be replaying lots of indoe changes > > such as: > > > > log inode > > commit A > > log inode > > commit B > > log inode > > set NREXT64 > > commit C > > writeback inode > > <crash before log tail moves> > > > > Recovery will then replay commit A, B and C, in which case we *must > > not recover the log inode* in commit A or B because the LSN in the > > on-disk inode points at commit C. Hence replaying A or B will result > > in the on-disk inode going backwards in time and hence resulting in > > an inconsistent state on disk until commit C is recovered. > > > >> i.e. there is no need to compare LSNs of the checkpoint > >> transaction being replayed and that of the disk inode. > > > > Inncorrect: we -always- have to do this, regardless of the change > > being made. > > > >> If log recovery comes across a log inode with NREXT64 bit set in its di_flags2 > >> field, then we can safely conclude that the ondisk inode has to be updated to > >> reflect this change > > > > We can't assume that. This makes an assumption that NREXT64 is > > only ever a one-way transition. There's nothing in the disk format that > > prevents us from -removing- NREXT64 for inodes that don't need large > > extent counts. > > > > Yes, the -current implementation- does not allow going back to small > > extent counts, but the on-disk format design still needs to allow > > for such things to be done as we may need such functionality and > > flexibility in the on-disk format in the future. > > > > Hence we have to ensure that log recovery handles both set and reset > > transistions from the start. If we don't ensure that log recovery > > handles reset conditions when we first add the feature bit, then > > we are going to have to add a log incompat or another feature bit > > to stop older kernels from trying to recover reset operations. > > > > Ok. I had never considered the possibility of transitioning an inode back into > 32-bit data fork extent count format. With this new requirement, I now > understand the reasoning behind comparing ondisk inode's LSN and checkpoint > transaction's LSN. > > As you have mentioned earlier, comparing LSNs is required not only for the > change introduced in this patch, but also for any other change in value of any > of the inode's fields. Without such a comparison, the inode can temporarily > end up being in an inconsistent state during log replay. > > To that end, The following code snippet from xlog_recover_inode_commit_pass2() > skips playing back xfs_log_dinode entries when ondisk inode's LSN is greater > than checkpoint transaction's LSN, > > if (dip->di_version >= 3) { > xfs_lsn_t lsn = be64_to_cpu(dip->di_lsn); > > if (lsn && lsn != -1 && XFS_LSN_CMP(lsn, current_lsn) > 0) { > trace_xfs_log_recover_inode_skip(log, in_f); > error = 0; > goto out_owner_change; > } > } > > > However, if the commits in the sequence below belong to three different > checkpoint transactions having the same LSN, > > log inode > commit A > log inode > commit B > set NREXT64 > log inode > commit C > writeback inode > <crash before log tail moves> > > Then the above code snippet won't prevent an inode from becoming temporarily > inconsistent due to commits A and B being replayed. Ah, this is a very special corner case. You snipped out the most important part of the comment above that code: /* * If the inode has an LSN in it, recover the inode only if the on-disk * inode's LSN is older than the lsn of the transaction we are * replaying. We can have multiple checkpoints with the same start LSN, * so the current LSN being equal to the on-disk LSN doesn't necessarily * mean that the on-disk inode is more recent than the change being * replayed. .... This is exactly the situation you are asking about here - what happens in recovery when the LSNs are the same and there are multiple checkpoints with the same LSN. The first thing to understand here is "how do we get checkpoints with the same LSN" and then understand what it implies. We get checkpoints with the same start/commit LSNs when multiple checkpoints are written in the same iclog. The start/commit LSNs are determined by the LSN of the iclog they are written in, and hence if they are the same they were written to the journal in a single "atomic" IO. I say "atomic" because it's not an atomic IO at the hardware level. It's atomic in that the entire iclog is protected by a CRC and hence if the CRC check for the iclog passes at recovery, then the iclog write has been recovered intact. If the write was torn, misdirected or some other physical media failure occurred, then we don't recovery the iclog at all. IOWs, none of the changes in the iclog are recovered. IOWs, we have atomic "all or nothing" iclog recovery semantics. Next, the fact that the inode has been written back and is up to date on disk means that the iclog is entirely on stable storage. The inode isn't unpinned until the flush/FUA associtate with the iclog was completed, which happens before the iclog IO is completed and the callbacks to unpin the inode are run. Hence ordering tells us the entire iclog is on disk and should be recovered. What this really means is that we cannot possibly see the intermediate commit A or commit B states on disk at runtime or before recovery is run. The metadata is not unpinned until the iclog that also contains commit C is written to the journal. Hence from the POV of the on-disk inode, we go from the original version to commit C in one step and we never, ever see A or B as intermediate states. IOWs, the iclog contents defines old -> C as an atomic on-disk modification, even though the contents are spread across multiple checkpoints.[1] Hence in this specific case, we have 3 individual modifications to the inode and it's related metadata sitting in the journal waiting for log recovery to replay them as an atomic unit. They will all get recovered, and each change that is replayed will be internally consistent. Therefore, after replaying commit A, the inode and it's metadata will be reverted to whatever was in that commit and it will be consistent in that context. Then replay of commit B and commit C bring it back up to being up to date on disk and providing the step change from old -> C as the runtime code would have also done. Hence at the end of replay, the inode and all it's related metadata will be consistent with commit C and so so this special transient corner case should resolve itself correctly (at least, as far as my poor dumb brain can reason about it being correct). > To handle this, we should > probably go with the additional rule of "Replay log inode if both the log > inode and the ondisk inode have the same value for NREXT64 bit". No, we do not want case specific logic in recovery code like this because inode core updates are simply overwrites. As long as the overwrites are all replayed from A to C, we end up with the correct result of an "atomic" step change from old to C on disk... Cheers, Dave. [1] There's more really subtle, complex details around start LSN vs commit LSN ordering with AIL, iclog and recovery LSNs and how to treat same start/different commit LSNs, different start/same commit LSNs, etc, but that's way beyond the scope of what is needed to be understood here. These play into why we replay all the changes at the same LSN as per above rather than skip them. Commit 32baa63d82ee ("xfs: logging the on disk inode LSN can make it go backwards") might give you some more insight into the complexities here.
On 14 Oct 2021 at 07:30, Dave Chinner wrote: > On Wed, Oct 13, 2021 at 08:14:01PM +0530, Chandan Babu R wrote: >> On 11 Oct 2021 at 03:19, Dave Chinner wrote: >> > On Thu, Oct 07, 2021 at 04:22:25PM +0530, Chandan Babu R wrote: >> >> On 01 Oct 2021 at 04:25, Dave Chinner wrote: >> >> > On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote: >> >> >> On 30 Sep 2021 at 10:01, Dave Chinner wrote: >> >> >> > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: >> >> >> > >> >> >> >> >> >> Ok. The above solution looks logically correct. I haven't been able to come up >> >> >> with a scenario where the solution wouldn't work. I will implement it and see >> >> >> if anything breaks. >> >> > >> >> > I think I can poke one hole in it - I missed the fact that if we >> >> > upgrade and inode read time, and then we modify the inode without >> >> > modifying the inode core (can we even do that - metadata mods should >> >> > at least change timestamps right?) then we don't log the format >> >> > change or the NREXT64 inode flag change and they only appear in the >> >> > on-disk inode at writeback. >> >> > >> >> > Log recovery needs to be checked for correct behaviour here. I think >> >> > that if the inode is in NREXT64 format when read in and the log >> >> > inode core is not, then the on disk LSN must be more recent than >> >> > what is being recovered from the log and should be skipped. If >> >> > NREXT64 is present in the log inode, then we logged the core >> >> > properly and we just don't care what format is on disk because we >> >> > replay it into NREXT64 format and write that back. >> >> >> >> xfs_inode_item_format() logs the inode core regardless of whether >> >> XFS_ILOG_CORE flag is set in xfs_inode_log_item->ili_fields. Hence, setting >> >> the NREXT64 bit in xfs_dinode->di_flags2 just after reading an inode from disk >> >> should not result in a scenario where the corresponding >> >> xfs_log_dinode->di_flags2 will not have NREXT64 bit set. >> > >> > Except that log recovery might be replaying lots of indoe changes >> > such as: >> > >> > log inode >> > commit A >> > log inode >> > commit B >> > log inode >> > set NREXT64 >> > commit C >> > writeback inode >> > <crash before log tail moves> >> > >> > Recovery will then replay commit A, B and C, in which case we *must >> > not recover the log inode* in commit A or B because the LSN in the >> > on-disk inode points at commit C. Hence replaying A or B will result >> > in the on-disk inode going backwards in time and hence resulting in >> > an inconsistent state on disk until commit C is recovered. >> > >> >> i.e. there is no need to compare LSNs of the checkpoint >> >> transaction being replayed and that of the disk inode. >> > >> > Inncorrect: we -always- have to do this, regardless of the change >> > being made. >> > >> >> If log recovery comes across a log inode with NREXT64 bit set in its di_flags2 >> >> field, then we can safely conclude that the ondisk inode has to be updated to >> >> reflect this change >> > >> > We can't assume that. This makes an assumption that NREXT64 is >> > only ever a one-way transition. There's nothing in the disk format that >> > prevents us from -removing- NREXT64 for inodes that don't need large >> > extent counts. >> > >> > Yes, the -current implementation- does not allow going back to small >> > extent counts, but the on-disk format design still needs to allow >> > for such things to be done as we may need such functionality and >> > flexibility in the on-disk format in the future. >> > >> > Hence we have to ensure that log recovery handles both set and reset >> > transistions from the start. If we don't ensure that log recovery >> > handles reset conditions when we first add the feature bit, then >> > we are going to have to add a log incompat or another feature bit >> > to stop older kernels from trying to recover reset operations. >> > >> >> Ok. I had never considered the possibility of transitioning an inode back into >> 32-bit data fork extent count format. With this new requirement, I now >> understand the reasoning behind comparing ondisk inode's LSN and checkpoint >> transaction's LSN. >> >> As you have mentioned earlier, comparing LSNs is required not only for the >> change introduced in this patch, but also for any other change in value of any >> of the inode's fields. Without such a comparison, the inode can temporarily >> end up being in an inconsistent state during log replay. >> >> To that end, The following code snippet from xlog_recover_inode_commit_pass2() >> skips playing back xfs_log_dinode entries when ondisk inode's LSN is greater >> than checkpoint transaction's LSN, >> >> if (dip->di_version >= 3) { >> xfs_lsn_t lsn = be64_to_cpu(dip->di_lsn); >> >> if (lsn && lsn != -1 && XFS_LSN_CMP(lsn, current_lsn) > 0) { >> trace_xfs_log_recover_inode_skip(log, in_f); >> error = 0; >> goto out_owner_change; >> } >> } >> >> >> However, if the commits in the sequence below belong to three different >> checkpoint transactions having the same LSN, >> >> log inode >> commit A >> log inode >> commit B >> set NREXT64 >> log inode >> commit C >> writeback inode >> <crash before log tail moves> >> >> Then the above code snippet won't prevent an inode from becoming temporarily >> inconsistent due to commits A and B being replayed. > > Ah, this is a very special corner case. You snipped out the most > important part of the comment above that code: > > /* > * If the inode has an LSN in it, recover the inode only if the on-disk > * inode's LSN is older than the lsn of the transaction we are > * replaying. We can have multiple checkpoints with the same start LSN, > * so the current LSN being equal to the on-disk LSN doesn't necessarily > * mean that the on-disk inode is more recent than the change being > * replayed. > .... > > This is exactly the situation you are asking about here - what > happens in recovery when the LSNs are the same and there are > multiple checkpoints with the same LSN. > > The first thing to understand here is "how do we get checkpoints > with the same LSN" and then understand what it implies. > > We get checkpoints with the same start/commit LSNs when multiple > checkpoints are written in the same iclog. The start/commit LSNs are > determined by the LSN of the iclog they are written in, and hence if > they are the same they were written to the journal in a single > "atomic" IO. > > I say "atomic" because it's not an atomic IO at the hardware level. > It's atomic in that the entire iclog is protected by a CRC and hence > if the CRC check for the iclog passes at recovery, then the iclog write has been > recovered intact. If the write was torn, misdirected > or some other physical media failure occurred, then we don't > recovery the iclog at all. IOWs, none of the changes in the iclog > are recovered. IOWs, we have atomic "all or nothing" iclog recovery > semantics. > > Next, the fact that the inode has been written back and is up to > date on disk means that the iclog is entirely on stable storage. > The inode isn't unpinned until the flush/FUA associtate with the > iclog was completed, which happens before the iclog IO is completed > and the callbacks to unpin the inode are run. Hence ordering tells > us the entire iclog is on disk and should be recovered. > > What this really means is that we cannot possibly see the > intermediate commit A or commit B states on disk at runtime or > before recovery is run. The metadata is not unpinned until the iclog > that also contains commit C is written to the journal. Hence from > the POV of the on-disk inode, we go from the original version to > commit C in one step and we never, ever see A or B as intermediate > states. IOWs, the iclog contents defines old -> C as an atomic > on-disk modification, even though the contents are spread across > multiple checkpoints.[1] > > Hence in this specific case, we have 3 individual modifications to > the inode and it's related metadata sitting in the journal waiting > for log recovery to replay them as an atomic unit. They will all get > recovered, and each change that is replayed will be internally > consistent. Therefore, after replaying commit A, the inode and it's > metadata will be reverted to whatever was in that commit and it will > be consistent in that context. Then replay of commit B and commit C > bring it back up to being up to date on disk and providing the step > change from old -> C as the runtime code would have also done. > > Hence at the end of replay, the inode and all it's related metadata > will be consistent with commit C and so so this special transient > corner case should resolve itself correctly (at least, as far as my > poor dumb brain can reason about it being correct). > Thanks for the detailed explaination. I had figured out that multiple checkpoints can end up having the same LSN because they were written to the same iclog. The value of cil->xc_push_commit_stable is one of the things that determine if the iclog is supposed to be flushed or not just after writing the contents of a CIL context into it. However the "atomic replay" behaviour had not occured to me. >> To handle this, we should >> probably go with the additional rule of "Replay log inode if both the log >> inode and the ondisk inode have the same value for NREXT64 bit". > > No, we do not want case specific logic in recovery code like this > because inode core updates are simply overwrites. As long as the > overwrites are all replayed from A to C, we end up with the correct > result of an "atomic" step change from old to C on disk... > W.r.t processing per-inode NREXT64 bit status during log recovery, I think we can depend on the LSN comparison that is already implemented in xlog_recover_inode_commit_pass2() to skip checkpoint transactions (with different LSNs) which can make an ondisk inode enter an inconsistent state. > Cheers, > > Dave. > > [1] There's more really subtle, complex details around start LSN vs > commit LSN ordering with AIL, iclog and recovery LSNs and how to > treat same start/different commit LSNs, different start/same commit > LSNs, etc, but that's way beyond the scope of what is needed to be > understood here. These play into why we replay all the changes at > the same LSN as per above rather than skip them. Commit 32baa63d82ee > ("xfs: logging the on disk inode LSN can make it go backwards") > might give you some more insight into the complexities here. Thanks for the commit ID. I will add this to my list of items to read.
On 13 Oct 2021 at 20:14, Chandan Babu R wrote: > On 11 Oct 2021 at 03:19, Dave Chinner wrote: >> On Thu, Oct 07, 2021 at 04:22:25PM +0530, Chandan Babu R wrote: >>> On 01 Oct 2021 at 04:25, Dave Chinner wrote: >>> > On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote: >>> >> On 30 Sep 2021 at 10:01, Dave Chinner wrote: >>> >> > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote: >>> >> > >>> >> [...] >>> >> > FWIW, I also think doing something like this would help make the >>> >> > code be easier to read and confirm that it is obviously correct when >>> >> > reading it: >>> >> > >>> >> > __be32 di_gid; /* owner's group id */ >>> >> > __be32 di_nlink; /* number of links to file */ >>> >> > __be16 di_projid_lo; /* lower part of owner's project id */ >>> >> > __be16 di_projid_hi; /* higher part owner's project id */ >>> >> > union { >>> >> > __be64 di_big_dextcnt; /* NREXT64 data extents */ >>> >> > __u8 di_v3_pad[8]; /* !NREXT64 V3 inode zeroed space */ >>> >> > struct { >>> >> > __u8 di_v2_pad[6]; /* V2 inode zeroed space */ >>> >> > __be16 di_flushiter; /* V2 inode 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 */ >>> >> > union { >>> >> > struct { >>> >> > __be32 di_big_aextcnt; /* NREXT64 attr extents */ >>> >> > __be16 di_nrext64_pad; /* NREXT64 unused, zero */ >>> >> > }; >>> >> > struct { >>> >> > __be32 di_nextents; /* !NREXT64 data extents */ >>> >> > __be16 di_anextents; /* !NREXT64 attr extents */ >>> >> > } >>> >> > } >>> >>> The two structures above result in padding and hence result in a hole being >>> introduced. The entire union above can be replaced with the following, >>> >>> union { >>> __be32 di_big_aextcnt; /* NREXT64 attr extents */ >>> __be32 di_nextents; /* !NREXT64 data extents */ >>> }; >>> union { >>> __be16 di_nrext64_pad; /* NREXT64 unused, zero */ >>> __be16 di_anextents; /* !NREXT64 attr extents */ >>> }; >> >> I don't think this makes sense. This groups by field rather than >> by feature layout. It doesn't make it clear at all that these >> varaibles both change definition at the same time - they are either >> {di_nexts, di_anexts} pair or a {di_big_aexts, pad} pair. That's the >> whole point of using anonymous structs here - it defines and >> documents the relationship between the layouts when certain features >> are set rather than relying on people to parse the comments >> correctly to determine the relationship.... > > Ok. I will need to check if there are alternative ways of arranging the fields > to accomplish the goal stated above. I will think about this and get back as > soon as possible. The padding that results from the following structure layout, typedef struct xfs_dinode { __be16 di_magic; /* inode magic # = XFS_DINODE_MAGIC */ __be16 di_mode; /* mode and type of file */ __u8 di_version; /* inode version */ __u8 di_format; /* format of di_c data */ __be16 di_onlink; /* old number of links to file */ __be32 di_uid; /* owner's user id */ __be32 di_gid; /* owner's group id */ __be32 di_nlink; /* number of links to file */ __be16 di_projid_lo; /* lower part of owner's project id */ __be16 di_projid_hi; /* higher part owner's project id */ __u8 di_pad[6]; /* unused, zeroed space */ __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 */ union { struct { __be32 di_big_aextcnt; /* NREXT64 attr extents */ __be16 di_nrext64_pad; /* NREXT64 unused, zero */ }; struct { __be32 di_nextents; /* !NREXT64 data extents */ __be16 di_anextents; /* !NREXT64 attr extents */ }; }; __u8 di_forkoff; /* attr fork offs, <<3 for 64b align */ __s8 di_aformat; /* format of attr fork's data */ ... can be solved by packing the two structures contained within the union i.e. union { struct { __be32 di_big_aextcnt; /* NREXT64 attr extents */ __be16 di_nrext64_pad; /* NREXT64 unused, zero */ } __packed; struct { __be32 di_nextents; /* !NREXT64 data extents */ __be16 di_anextents; /* !NREXT64 attr extents */ } __packed; }; __u8 di_forkoff; /* attr fork offs, <<3 for 64b align */ __s8 di_aformat; /* format of attr fork's data */ Each of the two structures start at an 8-byte offset and the two 1-byte fields (di_forkoff & di_aformat) defined later in the structure, prevent introduction of holes inside dinode structure. Also, An exception shouldn't be generated if the address of any of the packed structure members is assigned to another pointer variable and later the pointer variable is dereferenced. This is because such an address would still be a 4-byte aligned address (in the case of di_big_aextcnt/di_nextents) or a 2-byte aligned address (in the case of di_nrext64_pad/di_anextents).
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index dba868f2c3e3..87c927d912f6 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -802,8 +802,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 */ @@ -941,11 +941,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_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index dc511630cc7a..882ed4873afe 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)); to->di_forkoff = ip->i_forkoff; to->di_aformat = xfs_ifork_format(ip->i_afp); to->di_flags = cpu_to_be16(ip->i_diflags); diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h index bd711d244c4b..9f352ff4352b 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 4133a91c9a57..19ea86aa9fd0 100644 --- a/fs/xfs/scrub/inode_repair.c +++ b/fs/xfs/scrub/inode_repair.c @@ -740,7 +740,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) { @@ -827,7 +827,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 e44ab2d9f85f..92888a6a6e51 100644 --- a/fs/xfs/scrub/trace.h +++ b/fs/xfs/scrub/trace.h @@ -1218,8 +1218,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) @@ -1238,8 +1238,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); @@ -1247,7 +1247,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 disize 0x%llx nblocks 0x%llx extsize %u nextents %u anextents %u forkoff 0x%x 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 disize 0x%llx nblocks 0x%llx extsize %u nextents32 %u nextents16 %u forkoff 0x%x aformat %u flags 0x%x gen 0x%x flags2 0x%llx cowextsize %u", MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->mode, @@ -1258,8 +1258,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 0659d19c211e..e4800a965670 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 239dd2e3384e..c21fb3d2ddca 100644 --- a/fs/xfs/xfs_inode_item_recover.c +++ b/fs/xfs/xfs_inode_item_recover.c @@ -167,8 +167,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); @@ -342,7 +342,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)); @@ -350,7 +350,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;
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. This change is done to enable a future commit to introduce a new 64-bit extent counter field. Signed-off-by: Chandan Babu R <chandan.babu@oracle.com> --- fs/xfs/libxfs/xfs_format.h | 8 ++++---- fs/xfs/libxfs/xfs_inode_buf.c | 4 ++-- 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(-)