* [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper
2020-03-12 14:22 remove the di_version icdicone field v2 Christoph Hellwig
@ 2020-03-12 14:22 ` Christoph Hellwig
2020-03-16 13:16 ` Brian Foster
2020-03-16 14:46 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 2/5] xfs: only check the superblock version for dinode size calculation Christoph Hellwig
` (3 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-12 14:22 UTC (permalink / raw)
To: linux-xfs
Add a new wrapper to check if a file system supports the newer large
dinode format. Previously we uses xfs_sb_version_hascrc for that,
which is technically correct but a little confusing to read.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_format.h | 5 +++++
fs/xfs/libxfs/xfs_ialloc.c | 4 ++--
fs/xfs/libxfs/xfs_inode_buf.c | 12 ++++++++----
fs/xfs/libxfs/xfs_trans_resv.c | 2 +-
fs/xfs/xfs_buf_item.c | 2 +-
fs/xfs/xfs_log_recover.c | 2 +-
6 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index cd814f99da28..a28bf6a978ad 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
}
+static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
+{
+ return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
+}
+
static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
{
return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index b4a404278935..6adffaa68fb8 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
* That means for v3 inode we log the entire buffer rather than just the
* inode cores.
*/
- if (xfs_sb_version_hascrc(&mp->m_sb)) {
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
version = 3;
ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
@@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
* cannot change the behavior.
*/
igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
- if (xfs_sb_version_hascrc(&mp->m_sb)) {
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
int new_size = igeo->inode_cluster_size_raw;
new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 17e88a8c8353..a5aa2f220c28 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -44,14 +44,18 @@ xfs_inobp_check(
}
#endif
+/*
+ * v4 and earlier file systems only support the small dinode, and must use the
+ * v1 or v2 inode formats. v5 file systems support a larger dinode, and must
+ * use the v3 inode format.
+ */
bool
xfs_dinode_good_version(
struct xfs_mount *mp,
__u8 version)
{
- if (xfs_sb_version_hascrc(&mp->m_sb))
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb))
return version == 3;
-
return version == 1 || version == 2;
}
@@ -454,7 +458,7 @@ xfs_dinode_verify(
/* Verify v3 integrity information first */
if (dip->di_version >= 3) {
- if (!xfs_sb_version_hascrc(&mp->m_sb))
+ if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
return __this_address;
if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
XFS_DINODE_CRC_OFF))
@@ -629,7 +633,7 @@ xfs_iread(
/* shortcut IO on inode allocation if possible */
if ((iget_flags & XFS_IGET_CREATE) &&
- xfs_sb_version_hascrc(&mp->m_sb) &&
+ xfs_sb_version_has_large_dinode(&mp->m_sb) &&
!(mp->m_flags & XFS_MOUNT_IKEEP)) {
VFS_I(ip)->i_generation = prandom_u32();
ip->i_d.di_version = 3;
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 7a9c04920505..294e23d47912 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
XFS_FSB_TO_B(mp, 1));
if (alloc) {
/* icreate tx uses ordered buffers */
- if (xfs_sb_version_hascrc(&mp->m_sb))
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb))
return res;
size = XFS_FSB_TO_B(mp, 1);
}
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 663810e6cd59..d004ae3455d7 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -345,7 +345,7 @@ xfs_buf_item_format(
* occurs during recovery.
*/
if (bip->bli_flags & XFS_BLI_INODE_BUF) {
- if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
+ if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
!((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
xfs_log_item_in_current_chkpt(lip)))
bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 6abc0863c9c3..e5e976b5cc11 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
* superblock flag to determine whether we need to look at di_flushiter
* to skip replay when the on disk inode is newer than the log one
*/
- if (!xfs_sb_version_hascrc(&mp->m_sb) &&
+ if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
/*
* Deal with the wrap case, DI_MAX_FLUSH is less
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper
2020-03-12 14:22 ` [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper Christoph Hellwig
@ 2020-03-16 13:16 ` Brian Foster
2020-03-16 14:47 ` Christoph Hellwig
2020-03-16 14:51 ` Darrick J. Wong
2020-03-16 14:46 ` Chandan Rajendra
1 sibling, 2 replies; 21+ messages in thread
From: Brian Foster @ 2020-03-16 13:16 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thu, Mar 12, 2020 at 03:22:31PM +0100, Christoph Hellwig wrote:
> Add a new wrapper to check if a file system supports the newer large
> dinode format. Previously we uses xfs_sb_version_hascrc for that,
> which is technically correct but a little confusing to read.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_format.h | 5 +++++
> fs/xfs/libxfs/xfs_ialloc.c | 4 ++--
> fs/xfs/libxfs/xfs_inode_buf.c | 12 ++++++++----
> fs/xfs/libxfs/xfs_trans_resv.c | 2 +-
> fs/xfs/xfs_buf_item.c | 2 +-
> fs/xfs/xfs_log_recover.c | 2 +-
> 6 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index cd814f99da28..a28bf6a978ad 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
> return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> }
>
A comment would be useful here to indicate what this means (i.e., we can
assume v3 inode format). The function name is a little vague too I
suppose (will the inode ever get larger than large? :P). I wonder if
something like _has_v3_inode() is more clear, but we can always change
it easily enough and either way is better than hascrc() IMO.
Brian
> +static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
> +{
> + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> +}
> +
> static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
> {
> return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index b4a404278935..6adffaa68fb8 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
> * That means for v3 inode we log the entire buffer rather than just the
> * inode cores.
> */
> - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> version = 3;
> ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
>
> @@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
> * cannot change the behavior.
> */
> igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
> - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> int new_size = igeo->inode_cluster_size_raw;
>
> new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 17e88a8c8353..a5aa2f220c28 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -44,14 +44,18 @@ xfs_inobp_check(
> }
> #endif
>
> +/*
> + * v4 and earlier file systems only support the small dinode, and must use the
> + * v1 or v2 inode formats. v5 file systems support a larger dinode, and must
> + * use the v3 inode format.
> + */
> bool
> xfs_dinode_good_version(
> struct xfs_mount *mp,
> __u8 version)
> {
> - if (xfs_sb_version_hascrc(&mp->m_sb))
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> return version == 3;
> -
> return version == 1 || version == 2;
> }
>
> @@ -454,7 +458,7 @@ xfs_dinode_verify(
>
> /* Verify v3 integrity information first */
> if (dip->di_version >= 3) {
> - if (!xfs_sb_version_hascrc(&mp->m_sb))
> + if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
> return __this_address;
> if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
> XFS_DINODE_CRC_OFF))
> @@ -629,7 +633,7 @@ xfs_iread(
>
> /* shortcut IO on inode allocation if possible */
> if ((iget_flags & XFS_IGET_CREATE) &&
> - xfs_sb_version_hascrc(&mp->m_sb) &&
> + xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> VFS_I(ip)->i_generation = prandom_u32();
> ip->i_d.di_version = 3;
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 7a9c04920505..294e23d47912 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
> XFS_FSB_TO_B(mp, 1));
> if (alloc) {
> /* icreate tx uses ordered buffers */
> - if (xfs_sb_version_hascrc(&mp->m_sb))
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> return res;
> size = XFS_FSB_TO_B(mp, 1);
> }
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 663810e6cd59..d004ae3455d7 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -345,7 +345,7 @@ xfs_buf_item_format(
> * occurs during recovery.
> */
> if (bip->bli_flags & XFS_BLI_INODE_BUF) {
> - if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
> + if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
> !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
> xfs_log_item_in_current_chkpt(lip)))
> bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 6abc0863c9c3..e5e976b5cc11 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
> * superblock flag to determine whether we need to look at di_flushiter
> * to skip replay when the on disk inode is newer than the log one
> */
> - if (!xfs_sb_version_hascrc(&mp->m_sb) &&
> + if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
> /*
> * Deal with the wrap case, DI_MAX_FLUSH is less
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper
2020-03-16 13:16 ` Brian Foster
@ 2020-03-16 14:47 ` Christoph Hellwig
2020-03-16 14:51 ` Darrick J. Wong
1 sibling, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-16 14:47 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs
On Mon, Mar 16, 2020 at 09:16:49AM -0400, Brian Foster wrote:
> A comment would be useful here to indicate what this means (i.e., we can
> assume v3 inode format).
Sure.
> The function name is a little vague too I
> suppose (will the inode ever get larger than large? :P). I wonder if
> something like _has_v3_inode() is more clear, but we can always change
> it easily enough and either way is better than hascrc() IMO.
I'm not too fond of the v3 name as the check also guards any newer
version (but then again I hope we never need to rev the version..)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper
2020-03-16 13:16 ` Brian Foster
2020-03-16 14:47 ` Christoph Hellwig
@ 2020-03-16 14:51 ` Darrick J. Wong
1 sibling, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-03-16 14:51 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs
On Mon, Mar 16, 2020 at 09:16:49AM -0400, Brian Foster wrote:
> On Thu, Mar 12, 2020 at 03:22:31PM +0100, Christoph Hellwig wrote:
> > Add a new wrapper to check if a file system supports the newer large
> > dinode format. Previously we uses xfs_sb_version_hascrc for that,
> > which is technically correct but a little confusing to read.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > fs/xfs/libxfs/xfs_format.h | 5 +++++
> > fs/xfs/libxfs/xfs_ialloc.c | 4 ++--
> > fs/xfs/libxfs/xfs_inode_buf.c | 12 ++++++++----
> > fs/xfs/libxfs/xfs_trans_resv.c | 2 +-
> > fs/xfs/xfs_buf_item.c | 2 +-
> > fs/xfs/xfs_log_recover.c | 2 +-
> > 6 files changed, 18 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index cd814f99da28..a28bf6a978ad 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
> > return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > }
> >
>
> A comment would be useful here to indicate what this means (i.e., we can
> assume v3 inode format). The function name is a little vague too I
> suppose (will the inode ever get larger than large? :P). I wonder if
> something like _has_v3_inode() is more clear, but we can always change
> it easily enough and either way is better than hascrc() IMO.
>
> Brian
>
> > +static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
/me agrees that this function ought to have a comment saying that
"large_dinode" means v3 inodes.
Not sure about larger than large inodes though -- the last di_flags2
could be made to mean "...and now expand structure by X bytes" whenever
we run out of space.
(The rest of the series looks ok to me)
--D
> > +{
> > + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > +}
> > +
> > static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
> > {
> > return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > index b4a404278935..6adffaa68fb8 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > @@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
> > * That means for v3 inode we log the entire buffer rather than just the
> > * inode cores.
> > */
> > - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> > version = 3;
> > ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
> >
> > @@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
> > * cannot change the behavior.
> > */
> > igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
> > - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> > int new_size = igeo->inode_cluster_size_raw;
> >
> > new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index 17e88a8c8353..a5aa2f220c28 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -44,14 +44,18 @@ xfs_inobp_check(
> > }
> > #endif
> >
> > +/*
> > + * v4 and earlier file systems only support the small dinode, and must use the
> > + * v1 or v2 inode formats. v5 file systems support a larger dinode, and must
> > + * use the v3 inode format.
> > + */
> > bool
> > xfs_dinode_good_version(
> > struct xfs_mount *mp,
> > __u8 version)
> > {
> > - if (xfs_sb_version_hascrc(&mp->m_sb))
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> > return version == 3;
> > -
> > return version == 1 || version == 2;
> > }
> >
> > @@ -454,7 +458,7 @@ xfs_dinode_verify(
> >
> > /* Verify v3 integrity information first */
> > if (dip->di_version >= 3) {
> > - if (!xfs_sb_version_hascrc(&mp->m_sb))
> > + if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
> > return __this_address;
> > if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
> > XFS_DINODE_CRC_OFF))
> > @@ -629,7 +633,7 @@ xfs_iread(
> >
> > /* shortcut IO on inode allocation if possible */
> > if ((iget_flags & XFS_IGET_CREATE) &&
> > - xfs_sb_version_hascrc(&mp->m_sb) &&
> > + xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> > !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> > VFS_I(ip)->i_generation = prandom_u32();
> > ip->i_d.di_version = 3;
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 7a9c04920505..294e23d47912 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
> > XFS_FSB_TO_B(mp, 1));
> > if (alloc) {
> > /* icreate tx uses ordered buffers */
> > - if (xfs_sb_version_hascrc(&mp->m_sb))
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> > return res;
> > size = XFS_FSB_TO_B(mp, 1);
> > }
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 663810e6cd59..d004ae3455d7 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -345,7 +345,7 @@ xfs_buf_item_format(
> > * occurs during recovery.
> > */
> > if (bip->bli_flags & XFS_BLI_INODE_BUF) {
> > - if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
> > + if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
> > !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
> > xfs_log_item_in_current_chkpt(lip)))
> > bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 6abc0863c9c3..e5e976b5cc11 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
> > * superblock flag to determine whether we need to look at di_flushiter
> > * to skip replay when the on disk inode is newer than the log one
> > */
> > - if (!xfs_sb_version_hascrc(&mp->m_sb) &&
> > + if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> > ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
> > /*
> > * Deal with the wrap case, DI_MAX_FLUSH is less
> > --
> > 2.24.1
> >
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper
2020-03-12 14:22 ` [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper Christoph Hellwig
2020-03-16 13:16 ` Brian Foster
@ 2020-03-16 14:46 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Chandan Rajendra @ 2020-03-16 14:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote:
> Add a new wrapper to check if a file system supports the newer large
> dinode format. Previously we uses xfs_sb_version_hascrc for that,
> which is technically correct but a little confusing to read.
>
I don't see any logical issues.
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_format.h | 5 +++++
> fs/xfs/libxfs/xfs_ialloc.c | 4 ++--
> fs/xfs/libxfs/xfs_inode_buf.c | 12 ++++++++----
> fs/xfs/libxfs/xfs_trans_resv.c | 2 +-
> fs/xfs/xfs_buf_item.c | 2 +-
> fs/xfs/xfs_log_recover.c | 2 +-
> 6 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index cd814f99da28..a28bf6a978ad 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
> return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> }
>
> +static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
> +{
> + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> +}
> +
> static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
> {
> return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index b4a404278935..6adffaa68fb8 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
> * That means for v3 inode we log the entire buffer rather than just the
> * inode cores.
> */
> - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> version = 3;
> ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
>
> @@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
> * cannot change the behavior.
> */
> igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
> - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> int new_size = igeo->inode_cluster_size_raw;
>
> new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 17e88a8c8353..a5aa2f220c28 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -44,14 +44,18 @@ xfs_inobp_check(
> }
> #endif
>
> +/*
> + * v4 and earlier file systems only support the small dinode, and must use the
> + * v1 or v2 inode formats. v5 file systems support a larger dinode, and must
> + * use the v3 inode format.
> + */
> bool
> xfs_dinode_good_version(
> struct xfs_mount *mp,
> __u8 version)
> {
> - if (xfs_sb_version_hascrc(&mp->m_sb))
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> return version == 3;
> -
> return version == 1 || version == 2;
> }
>
> @@ -454,7 +458,7 @@ xfs_dinode_verify(
>
> /* Verify v3 integrity information first */
> if (dip->di_version >= 3) {
> - if (!xfs_sb_version_hascrc(&mp->m_sb))
> + if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
> return __this_address;
> if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
> XFS_DINODE_CRC_OFF))
> @@ -629,7 +633,7 @@ xfs_iread(
>
> /* shortcut IO on inode allocation if possible */
> if ((iget_flags & XFS_IGET_CREATE) &&
> - xfs_sb_version_hascrc(&mp->m_sb) &&
> + xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> VFS_I(ip)->i_generation = prandom_u32();
> ip->i_d.di_version = 3;
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 7a9c04920505..294e23d47912 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
> XFS_FSB_TO_B(mp, 1));
> if (alloc) {
> /* icreate tx uses ordered buffers */
> - if (xfs_sb_version_hascrc(&mp->m_sb))
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> return res;
> size = XFS_FSB_TO_B(mp, 1);
> }
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 663810e6cd59..d004ae3455d7 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -345,7 +345,7 @@ xfs_buf_item_format(
> * occurs during recovery.
> */
> if (bip->bli_flags & XFS_BLI_INODE_BUF) {
> - if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
> + if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
> !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
> xfs_log_item_in_current_chkpt(lip)))
> bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 6abc0863c9c3..e5e976b5cc11 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
> * superblock flag to determine whether we need to look at di_flushiter
> * to skip replay when the on disk inode is newer than the log one
> */
> - if (!xfs_sb_version_hascrc(&mp->m_sb) &&
> + if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
> /*
> * Deal with the wrap case, DI_MAX_FLUSH is less
>
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/5] xfs: only check the superblock version for dinode size calculation
2020-03-12 14:22 remove the di_version icdicone field v2 Christoph Hellwig
2020-03-12 14:22 ` [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper Christoph Hellwig
@ 2020-03-12 14:22 ` Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:47 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize Christoph Hellwig
` (2 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-12 14:22 UTC (permalink / raw)
To: linux-xfs
The size of the dinode structure is only dependent on the file system
version, so instead of checking the individual inode version just use
the newly added xfs_sb_version_has_large_dinode helper, and simplify
various calling conventions.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++---
fs/xfs/libxfs/xfs_bmap.c | 10 ++++------
fs/xfs/libxfs/xfs_format.h | 15 +++++++--------
fs/xfs/libxfs/xfs_ialloc.c | 2 +-
fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
fs/xfs/libxfs/xfs_inode_fork.h | 9 ++-------
fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
fs/xfs/xfs_inode_item.c | 4 ++--
fs/xfs/xfs_log_recover.c | 2 +-
fs/xfs/xfs_symlink.c | 2 +-
11 files changed, 26 insertions(+), 37 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 4be04aeee278..a4757e5e64ca 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -537,7 +537,7 @@ xfs_attr_shortform_bytesfit(
int offset;
/* rounded down */
- offset = (XFS_LITINO(mp, dp->i_d.di_version) - bytes) >> 3;
+ offset = (XFS_LITINO(mp) - bytes) >> 3;
if (dp->i_d.di_format == XFS_DINODE_FMT_DEV) {
minforkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
@@ -604,8 +604,7 @@ xfs_attr_shortform_bytesfit(
minforkoff = roundup(minforkoff, 8) >> 3;
/* attr fork btree root can have at least this many key/ptr pairs */
- maxforkoff = XFS_LITINO(mp, dp->i_d.di_version) -
- XFS_BMDR_SPACE_CALC(MINABTPTRS);
+ maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
maxforkoff = maxforkoff >> 3; /* rounded down */
if (offset >= maxforkoff)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 43ae2ab21084..a2fe8a585100 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -193,14 +193,12 @@ xfs_default_attroffset(
struct xfs_mount *mp = ip->i_mount;
uint offset;
- if (mp->m_sb.sb_inodesize == 256) {
- offset = XFS_LITINO(mp, ip->i_d.di_version) -
- XFS_BMDR_SPACE_CALC(MINABTPTRS);
- } else {
+ if (mp->m_sb.sb_inodesize == 256)
+ offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
+ else
offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS);
- }
- ASSERT(offset < XFS_LITINO(mp, ip->i_d.di_version));
+ ASSERT(offset < XFS_LITINO(mp));
return offset;
}
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index a28bf6a978ad..aeca184d63ee 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -942,8 +942,11 @@ enum xfs_dinode_fmt {
/*
* Inode size for given fs.
*/
-#define XFS_LITINO(mp, version) \
- ((int)(((mp)->m_sb.sb_inodesize) - xfs_dinode_size(version)))
+#define XFS_DINODE_SIZE(mp) \
+ (xfs_sb_version_has_large_dinode(&(mp)->m_sb) ? \
+ sizeof(struct xfs_dinode) : offsetof(struct xfs_dinode, di_crc))
+#define XFS_LITINO(mp) \
+ ((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(mp))
/*
* Inode data & attribute fork sizes, per inode.
@@ -952,13 +955,9 @@ enum xfs_dinode_fmt {
#define XFS_DFORK_BOFF(dip) ((int)((dip)->di_forkoff << 3))
#define XFS_DFORK_DSIZE(dip,mp) \
- (XFS_DFORK_Q(dip) ? \
- XFS_DFORK_BOFF(dip) : \
- XFS_LITINO(mp, (dip)->di_version))
+ (XFS_DFORK_Q(dip) ? XFS_DFORK_BOFF(dip) : XFS_LITINO(mp))
#define XFS_DFORK_ASIZE(dip,mp) \
- (XFS_DFORK_Q(dip) ? \
- XFS_LITINO(mp, (dip)->di_version) - XFS_DFORK_BOFF(dip) : \
- 0)
+ (XFS_DFORK_Q(dip) ? XFS_LITINO(mp) - XFS_DFORK_BOFF(dip) : 0)
#define XFS_DFORK_SIZE(dip,mp,w) \
((w) == XFS_DATA_FORK ? \
XFS_DFORK_DSIZE(dip, mp) : \
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 6adffaa68fb8..26de817351fa 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -339,7 +339,7 @@ xfs_ialloc_inode_init(
xfs_buf_zero(fbuf, 0, BBTOB(fbuf->b_length));
for (i = 0; i < M_IGEO(mp)->inodes_per_cluster; i++) {
int ioffset = i << mp->m_sb.sb_inodelog;
- uint isize = xfs_dinode_size(version);
+ uint isize = XFS_DINODE_SIZE(mp);
free = xfs_make_iptr(mp, fbuf, i);
free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index a5aa2f220c28..34ccf162abe1 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -432,7 +432,7 @@ xfs_dinode_verify_forkoff(
case XFS_DINODE_FMT_LOCAL: /* fall through ... */
case XFS_DINODE_FMT_EXTENTS: /* fall through ... */
case XFS_DINODE_FMT_BTREE:
- if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
+ if (dip->di_forkoff >= (XFS_LITINO(mp) >> 3))
return __this_address;
break;
default:
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index ad2b9c313fd2..518c6f0ec3a6 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -183,7 +183,7 @@ xfs_iformat_local(
*/
if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
xfs_warn(ip->i_mount,
- "corrupt inode %Lu (bad size %d for local fork, size = %d).",
+ "corrupt inode %Lu (bad size %d for local fork, size = %zd).",
(unsigned long long) ip->i_ino, size,
XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
xfs_inode_verifier_error(ip, -EFSCORRUPTED,
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 500333d0101e..668ee942be22 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -46,14 +46,9 @@ struct xfs_ifork {
(ip)->i_afp : \
(ip)->i_cowfp))
#define XFS_IFORK_DSIZE(ip) \
- (XFS_IFORK_Q(ip) ? \
- XFS_IFORK_BOFF(ip) : \
- XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
+ (XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
#define XFS_IFORK_ASIZE(ip) \
- (XFS_IFORK_Q(ip) ? \
- XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
- XFS_IFORK_BOFF(ip) : \
- 0)
+ (XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
#define XFS_IFORK_SIZE(ip,w) \
((w) == XFS_DATA_FORK ? \
XFS_IFORK_DSIZE(ip) : \
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 9bac0d2e56dc..76defbea8000 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -424,12 +424,10 @@ struct xfs_log_dinode {
/* structure must be padded to 64 bit alignment */
};
-static inline uint xfs_log_dinode_size(int version)
-{
- if (version == 3)
- return sizeof(struct xfs_log_dinode);
- return offsetof(struct xfs_log_dinode, di_next_unlinked);
-}
+#define xfs_log_dinode_size(mp) \
+ (xfs_sb_version_has_large_dinode(&(mp)->m_sb) ? \
+ sizeof(struct xfs_log_dinode) : \
+ offsetof(struct xfs_log_dinode, di_next_unlinked))
/*
* Buffer Log Format definitions
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index f021b55a0301..451f9b6b2806 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -125,7 +125,7 @@ xfs_inode_item_size(
*nvecs += 2;
*nbytes += sizeof(struct xfs_inode_log_format) +
- xfs_log_dinode_size(ip->i_d.di_version);
+ xfs_log_dinode_size(ip->i_mount);
xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
if (XFS_IFORK_Q(ip))
@@ -370,7 +370,7 @@ xfs_inode_item_format_core(
dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
- xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_d.di_version));
+ xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
}
/*
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e5e976b5cc11..79fc85a4ff08 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
error = -EFSCORRUPTED;
goto out_release;
}
- isize = xfs_log_dinode_size(ldip->di_version);
+ isize = xfs_log_dinode_size(mp);
if (unlikely(item->ri_buf[1].i_len > isize)) {
XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
XFS_ERRLEVEL_LOW, mp, ldip,
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index ea42e25ec1bf..fa0fa3c70f1a 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -192,7 +192,7 @@ xfs_symlink(
* The symlink will fit into the inode data fork?
* There can't be any attributes so we get the whole variable part.
*/
- if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
+ if (pathlen <= XFS_LITINO(mp))
fs_blocks = 0;
else
fs_blocks = xfs_symlink_blocks(mp, pathlen);
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/5] xfs: only check the superblock version for dinode size calculation
2020-03-12 14:22 ` [PATCH 2/5] xfs: only check the superblock version for dinode size calculation Christoph Hellwig
@ 2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Christoph Hellwig
2020-03-16 14:47 ` Chandan Rajendra
1 sibling, 1 reply; 21+ messages in thread
From: Brian Foster @ 2020-03-16 13:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thu, Mar 12, 2020 at 03:22:32PM +0100, Christoph Hellwig wrote:
> The size of the dinode structure is only dependent on the file system
> version, so instead of checking the individual inode version just use
> the newly added xfs_sb_version_has_large_dinode helper, and simplify
> various calling conventions.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++---
> fs/xfs/libxfs/xfs_bmap.c | 10 ++++------
> fs/xfs/libxfs/xfs_format.h | 15 +++++++--------
> fs/xfs/libxfs/xfs_ialloc.c | 2 +-
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.h | 9 ++-------
> fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_log_recover.c | 2 +-
> fs/xfs/xfs_symlink.c | 2 +-
> 11 files changed, 26 insertions(+), 37 deletions(-)
>
...
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index ad2b9c313fd2..518c6f0ec3a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -183,7 +183,7 @@ xfs_iformat_local(
> */
> if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
> xfs_warn(ip->i_mount,
> - "corrupt inode %Lu (bad size %d for local fork, size = %d).",
> + "corrupt inode %Lu (bad size %d for local fork, size = %zd).",
Is this here intentionally? Otherwise seems fine:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> (unsigned long long) ip->i_ino, size,
> XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
> xfs_inode_verifier_error(ip, -EFSCORRUPTED,
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 500333d0101e..668ee942be22 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -46,14 +46,9 @@ struct xfs_ifork {
> (ip)->i_afp : \
> (ip)->i_cowfp))
> #define XFS_IFORK_DSIZE(ip) \
> - (XFS_IFORK_Q(ip) ? \
> - XFS_IFORK_BOFF(ip) : \
> - XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
> + (XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
> #define XFS_IFORK_ASIZE(ip) \
> - (XFS_IFORK_Q(ip) ? \
> - XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
> - XFS_IFORK_BOFF(ip) : \
> - 0)
> + (XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
> #define XFS_IFORK_SIZE(ip,w) \
> ((w) == XFS_DATA_FORK ? \
> XFS_IFORK_DSIZE(ip) : \
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 9bac0d2e56dc..76defbea8000 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -424,12 +424,10 @@ struct xfs_log_dinode {
> /* structure must be padded to 64 bit alignment */
> };
>
> -static inline uint xfs_log_dinode_size(int version)
> -{
> - if (version == 3)
> - return sizeof(struct xfs_log_dinode);
> - return offsetof(struct xfs_log_dinode, di_next_unlinked);
> -}
> +#define xfs_log_dinode_size(mp) \
> + (xfs_sb_version_has_large_dinode(&(mp)->m_sb) ? \
> + sizeof(struct xfs_log_dinode) : \
> + offsetof(struct xfs_log_dinode, di_next_unlinked))
>
> /*
> * Buffer Log Format definitions
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f021b55a0301..451f9b6b2806 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -125,7 +125,7 @@ xfs_inode_item_size(
>
> *nvecs += 2;
> *nbytes += sizeof(struct xfs_inode_log_format) +
> - xfs_log_dinode_size(ip->i_d.di_version);
> + xfs_log_dinode_size(ip->i_mount);
>
> xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
> if (XFS_IFORK_Q(ip))
> @@ -370,7 +370,7 @@ xfs_inode_item_format_core(
>
> dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
> xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
> - xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_d.di_version));
> + xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
> }
>
> /*
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e5e976b5cc11..79fc85a4ff08 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
> error = -EFSCORRUPTED;
> goto out_release;
> }
> - isize = xfs_log_dinode_size(ldip->di_version);
> + isize = xfs_log_dinode_size(mp);
> if (unlikely(item->ri_buf[1].i_len > isize)) {
> XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
> XFS_ERRLEVEL_LOW, mp, ldip,
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index ea42e25ec1bf..fa0fa3c70f1a 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -192,7 +192,7 @@ xfs_symlink(
> * The symlink will fit into the inode data fork?
> * There can't be any attributes so we get the whole variable part.
> */
> - if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
> + if (pathlen <= XFS_LITINO(mp))
> fs_blocks = 0;
> else
> fs_blocks = xfs_symlink_blocks(mp, pathlen);
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/5] xfs: only check the superblock version for dinode size calculation
2020-03-16 13:17 ` Brian Foster
@ 2020-03-16 14:48 ` Christoph Hellwig
0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-16 14:48 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs
On Mon, Mar 16, 2020 at 09:17:07AM -0400, Brian Foster wrote:
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index ad2b9c313fd2..518c6f0ec3a6 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -183,7 +183,7 @@ xfs_iformat_local(
> > */
> > if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
> > xfs_warn(ip->i_mount,
> > - "corrupt inode %Lu (bad size %d for local fork, size = %d).",
> > + "corrupt inode %Lu (bad size %d for local fork, size = %zd).",
>
> Is this here intentionally? Otherwise seems fine:
Yes. XFS_DFORK_SIZE now returns a size_t, so the format strings needs
to match.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] xfs: only check the superblock version for dinode size calculation
2020-03-12 14:22 ` [PATCH 2/5] xfs: only check the superblock version for dinode size calculation Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
@ 2020-03-16 14:47 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Chandan Rajendra @ 2020-03-16 14:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote:
> The size of the dinode structure is only dependent on the file system
> version, so instead of checking the individual inode version just use
> the newly added xfs_sb_version_has_large_dinode helper, and simplify
> various calling conventions.
>
I don't see any logical issues.
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++---
> fs/xfs/libxfs/xfs_bmap.c | 10 ++++------
> fs/xfs/libxfs/xfs_format.h | 15 +++++++--------
> fs/xfs/libxfs/xfs_ialloc.c | 2 +-
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.h | 9 ++-------
> fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_log_recover.c | 2 +-
> fs/xfs/xfs_symlink.c | 2 +-
> 11 files changed, 26 insertions(+), 37 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 4be04aeee278..a4757e5e64ca 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -537,7 +537,7 @@ xfs_attr_shortform_bytesfit(
> int offset;
>
> /* rounded down */
> - offset = (XFS_LITINO(mp, dp->i_d.di_version) - bytes) >> 3;
> + offset = (XFS_LITINO(mp) - bytes) >> 3;
>
> if (dp->i_d.di_format == XFS_DINODE_FMT_DEV) {
> minforkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
> @@ -604,8 +604,7 @@ xfs_attr_shortform_bytesfit(
> minforkoff = roundup(minforkoff, 8) >> 3;
>
> /* attr fork btree root can have at least this many key/ptr pairs */
> - maxforkoff = XFS_LITINO(mp, dp->i_d.di_version) -
> - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> + maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> maxforkoff = maxforkoff >> 3; /* rounded down */
>
> if (offset >= maxforkoff)
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 43ae2ab21084..a2fe8a585100 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -193,14 +193,12 @@ xfs_default_attroffset(
> struct xfs_mount *mp = ip->i_mount;
> uint offset;
>
> - if (mp->m_sb.sb_inodesize == 256) {
> - offset = XFS_LITINO(mp, ip->i_d.di_version) -
> - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> - } else {
> + if (mp->m_sb.sb_inodesize == 256)
> + offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> + else
> offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS);
> - }
>
> - ASSERT(offset < XFS_LITINO(mp, ip->i_d.di_version));
> + ASSERT(offset < XFS_LITINO(mp));
> return offset;
> }
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index a28bf6a978ad..aeca184d63ee 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -942,8 +942,11 @@ enum xfs_dinode_fmt {
> /*
> * Inode size for given fs.
> */
> -#define XFS_LITINO(mp, version) \
> - ((int)(((mp)->m_sb.sb_inodesize) - xfs_dinode_size(version)))
> +#define XFS_DINODE_SIZE(mp) \
> + (xfs_sb_version_has_large_dinode(&(mp)->m_sb) ? \
> + sizeof(struct xfs_dinode) : offsetof(struct xfs_dinode, di_crc))
> +#define XFS_LITINO(mp) \
> + ((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(mp))
>
> /*
> * Inode data & attribute fork sizes, per inode.
> @@ -952,13 +955,9 @@ enum xfs_dinode_fmt {
> #define XFS_DFORK_BOFF(dip) ((int)((dip)->di_forkoff << 3))
>
> #define XFS_DFORK_DSIZE(dip,mp) \
> - (XFS_DFORK_Q(dip) ? \
> - XFS_DFORK_BOFF(dip) : \
> - XFS_LITINO(mp, (dip)->di_version))
> + (XFS_DFORK_Q(dip) ? XFS_DFORK_BOFF(dip) : XFS_LITINO(mp))
> #define XFS_DFORK_ASIZE(dip,mp) \
> - (XFS_DFORK_Q(dip) ? \
> - XFS_LITINO(mp, (dip)->di_version) - XFS_DFORK_BOFF(dip) : \
> - 0)
> + (XFS_DFORK_Q(dip) ? XFS_LITINO(mp) - XFS_DFORK_BOFF(dip) : 0)
> #define XFS_DFORK_SIZE(dip,mp,w) \
> ((w) == XFS_DATA_FORK ? \
> XFS_DFORK_DSIZE(dip, mp) : \
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 6adffaa68fb8..26de817351fa 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -339,7 +339,7 @@ xfs_ialloc_inode_init(
> xfs_buf_zero(fbuf, 0, BBTOB(fbuf->b_length));
> for (i = 0; i < M_IGEO(mp)->inodes_per_cluster; i++) {
> int ioffset = i << mp->m_sb.sb_inodelog;
> - uint isize = xfs_dinode_size(version);
> + uint isize = XFS_DINODE_SIZE(mp);
>
> free = xfs_make_iptr(mp, fbuf, i);
> free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index a5aa2f220c28..34ccf162abe1 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -432,7 +432,7 @@ xfs_dinode_verify_forkoff(
> case XFS_DINODE_FMT_LOCAL: /* fall through ... */
> case XFS_DINODE_FMT_EXTENTS: /* fall through ... */
> case XFS_DINODE_FMT_BTREE:
> - if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
> + if (dip->di_forkoff >= (XFS_LITINO(mp) >> 3))
> return __this_address;
> break;
> default:
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index ad2b9c313fd2..518c6f0ec3a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -183,7 +183,7 @@ xfs_iformat_local(
> */
> if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
> xfs_warn(ip->i_mount,
> - "corrupt inode %Lu (bad size %d for local fork, size = %d).",
> + "corrupt inode %Lu (bad size %d for local fork, size = %zd).",
> (unsigned long long) ip->i_ino, size,
> XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
> xfs_inode_verifier_error(ip, -EFSCORRUPTED,
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 500333d0101e..668ee942be22 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -46,14 +46,9 @@ struct xfs_ifork {
> (ip)->i_afp : \
> (ip)->i_cowfp))
> #define XFS_IFORK_DSIZE(ip) \
> - (XFS_IFORK_Q(ip) ? \
> - XFS_IFORK_BOFF(ip) : \
> - XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
> + (XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
> #define XFS_IFORK_ASIZE(ip) \
> - (XFS_IFORK_Q(ip) ? \
> - XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
> - XFS_IFORK_BOFF(ip) : \
> - 0)
> + (XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
> #define XFS_IFORK_SIZE(ip,w) \
> ((w) == XFS_DATA_FORK ? \
> XFS_IFORK_DSIZE(ip) : \
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 9bac0d2e56dc..76defbea8000 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -424,12 +424,10 @@ struct xfs_log_dinode {
> /* structure must be padded to 64 bit alignment */
> };
>
> -static inline uint xfs_log_dinode_size(int version)
> -{
> - if (version == 3)
> - return sizeof(struct xfs_log_dinode);
> - return offsetof(struct xfs_log_dinode, di_next_unlinked);
> -}
> +#define xfs_log_dinode_size(mp) \
> + (xfs_sb_version_has_large_dinode(&(mp)->m_sb) ? \
> + sizeof(struct xfs_log_dinode) : \
> + offsetof(struct xfs_log_dinode, di_next_unlinked))
>
> /*
> * Buffer Log Format definitions
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f021b55a0301..451f9b6b2806 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -125,7 +125,7 @@ xfs_inode_item_size(
>
> *nvecs += 2;
> *nbytes += sizeof(struct xfs_inode_log_format) +
> - xfs_log_dinode_size(ip->i_d.di_version);
> + xfs_log_dinode_size(ip->i_mount);
>
> xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
> if (XFS_IFORK_Q(ip))
> @@ -370,7 +370,7 @@ xfs_inode_item_format_core(
>
> dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
> xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
> - xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_d.di_version));
> + xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
> }
>
> /*
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e5e976b5cc11..79fc85a4ff08 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
> error = -EFSCORRUPTED;
> goto out_release;
> }
> - isize = xfs_log_dinode_size(ldip->di_version);
> + isize = xfs_log_dinode_size(mp);
> if (unlikely(item->ri_buf[1].i_len > isize)) {
> XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
> XFS_ERRLEVEL_LOW, mp, ldip,
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index ea42e25ec1bf..fa0fa3c70f1a 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -192,7 +192,7 @@ xfs_symlink(
> * The symlink will fit into the inode data fork?
> * There can't be any attributes so we get the whole variable part.
> */
> - if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
> + if (pathlen <= XFS_LITINO(mp))
> fs_blocks = 0;
> else
> fs_blocks = xfs_symlink_blocks(mp, pathlen);
>
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize
2020-03-12 14:22 remove the di_version icdicone field v2 Christoph Hellwig
2020-03-12 14:22 ` [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper Christoph Hellwig
2020-03-12 14:22 ` [PATCH 2/5] xfs: only check the superblock version for dinode size calculation Christoph Hellwig
@ 2020-03-12 14:22 ` Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc Christoph Hellwig
2020-03-12 14:22 ` [PATCH 5/5] xfs: remove the di_version field from struct icdinode Christoph Hellwig
4 siblings, 2 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-12 14:22 UTC (permalink / raw)
To: linux-xfs
Only v5 file systems can have the reflink feature, and those will
always use the large dinode format. Remove the extra check for the
inode version.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_ioctl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5a1d2b9cb05a..ad825ffa7e4c 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1473,8 +1473,7 @@ xfs_ioctl_setattr_check_cowextsize(
if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
return 0;
- if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb) ||
- ip->i_d.di_version != 3)
+ if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb))
return -EINVAL;
if (fa->fsx_cowextsize == 0)
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize
2020-03-12 14:22 ` [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize Christoph Hellwig
@ 2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Brian Foster @ 2020-03-16 13:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thu, Mar 12, 2020 at 03:22:33PM +0100, Christoph Hellwig wrote:
> Only v5 file systems can have the reflink feature, and those will
> always use the large dinode format. Remove the extra check for the
> inode version.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a1d2b9cb05a..ad825ffa7e4c 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1473,8 +1473,7 @@ xfs_ioctl_setattr_check_cowextsize(
> if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
> return 0;
>
> - if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb) ||
> - ip->i_d.di_version != 3)
> + if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb))
> return -EINVAL;
>
> if (fa->fsx_cowextsize == 0)
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize
2020-03-12 14:22 ` [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
@ 2020-03-16 14:48 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Chandan Rajendra @ 2020-03-16 14:48 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote:
> Only v5 file systems can have the reflink feature, and those will
> always use the large dinode format. Remove the extra check for the
> inode version.
>
I don't see any logical issues.
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_ioctl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a1d2b9cb05a..ad825ffa7e4c 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1473,8 +1473,7 @@ xfs_ioctl_setattr_check_cowextsize(
> if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
> return 0;
>
> - if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb) ||
> - ip->i_d.di_version != 3)
> + if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb))
> return -EINVAL;
>
> if (fa->fsx_cowextsize == 0)
>
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc
2020-03-12 14:22 remove the di_version icdicone field v2 Christoph Hellwig
` (2 preceding siblings ...)
2020-03-12 14:22 ` [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize Christoph Hellwig
@ 2020-03-12 14:22 ` Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 5/5] xfs: remove the di_version field from struct icdinode Christoph Hellwig
4 siblings, 2 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-12 14:22 UTC (permalink / raw)
To: linux-xfs
di_flags2 is initialized to zero for v4 and earlier file systems. This
means di_flags2 can only be non-zero for a v5 file systems, in which
case both the parent and child inodes can store the filed. Remove the
extra di_version check, and also remove the rather pointless local
di_flags2 variable while at it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_inode.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index addc3ee0cb73..ebfd8efb0efa 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -907,20 +907,13 @@ xfs_ialloc(
ip->i_d.di_flags |= di_flags;
}
- if (pip &&
- (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY) &&
- pip->i_d.di_version == 3 &&
- ip->i_d.di_version == 3) {
- uint64_t di_flags2 = 0;
-
+ if (pip && (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY)) {
if (pip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE) {
- di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
+ ip->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
ip->i_d.di_cowextsize = pip->i_d.di_cowextsize;
}
if (pip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
- di_flags2 |= XFS_DIFLAG2_DAX;
-
- ip->i_d.di_flags2 |= di_flags2;
+ ip->i_d.di_flags2 |= XFS_DIFLAG2_DAX;
}
/* FALLTHROUGH */
case S_IFLNK:
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc
2020-03-12 14:22 ` [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc Christoph Hellwig
@ 2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Brian Foster @ 2020-03-16 13:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thu, Mar 12, 2020 at 03:22:34PM +0100, Christoph Hellwig wrote:
> di_flags2 is initialized to zero for v4 and earlier file systems. This
> means di_flags2 can only be non-zero for a v5 file systems, in which
> case both the parent and child inodes can store the filed. Remove the
filed?
> extra di_version check, and also remove the rather pointless local
> di_flags2 variable while at it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Otherwise seems fine:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_inode.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index addc3ee0cb73..ebfd8efb0efa 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -907,20 +907,13 @@ xfs_ialloc(
>
> ip->i_d.di_flags |= di_flags;
> }
> - if (pip &&
> - (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY) &&
> - pip->i_d.di_version == 3 &&
> - ip->i_d.di_version == 3) {
> - uint64_t di_flags2 = 0;
> -
> + if (pip && (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY)) {
> if (pip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE) {
> - di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> + ip->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> ip->i_d.di_cowextsize = pip->i_d.di_cowextsize;
> }
> if (pip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
> - di_flags2 |= XFS_DIFLAG2_DAX;
> -
> - ip->i_d.di_flags2 |= di_flags2;
> + ip->i_d.di_flags2 |= XFS_DIFLAG2_DAX;
> }
> /* FALLTHROUGH */
> case S_IFLNK:
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc
2020-03-12 14:22 ` [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
@ 2020-03-16 14:48 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Chandan Rajendra @ 2020-03-16 14:48 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote:
> di_flags2 is initialized to zero for v4 and earlier file systems. This
> means di_flags2 can only be non-zero for a v5 file systems, in which
> case both the parent and child inodes can store the filed. Remove the
> extra di_version check, and also remove the rather pointless local
> di_flags2 variable while at it.
>
I don't see any logical issues.
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_inode.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index addc3ee0cb73..ebfd8efb0efa 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -907,20 +907,13 @@ xfs_ialloc(
>
> ip->i_d.di_flags |= di_flags;
> }
> - if (pip &&
> - (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY) &&
> - pip->i_d.di_version == 3 &&
> - ip->i_d.di_version == 3) {
> - uint64_t di_flags2 = 0;
> -
> + if (pip && (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY)) {
> if (pip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE) {
> - di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> + ip->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> ip->i_d.di_cowextsize = pip->i_d.di_cowextsize;
> }
> if (pip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
> - di_flags2 |= XFS_DIFLAG2_DAX;
> -
> - ip->i_d.di_flags2 |= di_flags2;
> + ip->i_d.di_flags2 |= XFS_DIFLAG2_DAX;
> }
> /* FALLTHROUGH */
> case S_IFLNK:
>
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/5] xfs: remove the di_version field from struct icdinode
2020-03-12 14:22 remove the di_version icdicone field v2 Christoph Hellwig
` (3 preceding siblings ...)
2020-03-12 14:22 ` [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc Christoph Hellwig
@ 2020-03-12 14:22 ` Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:49 ` Chandan Rajendra
4 siblings, 2 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-12 14:22 UTC (permalink / raw)
To: linux-xfs
We know the version is 3 if on a v5 file system. For earlier file
systems formats we always upgrade the remaining v1 inodes to v2 and
thus only use v2 inodes. Use the xfs_sb_version_has_large_dinode
helper to check if we deal with small or large dinodes, and thus
remove the need for the di_version field in struct icdinode.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_inode_buf.c | 16 ++++++----------
fs/xfs/libxfs/xfs_inode_buf.h | 1 -
fs/xfs/xfs_bmap_util.c | 16 ++++++++--------
fs/xfs/xfs_inode.c | 16 ++--------------
fs/xfs/xfs_inode_item.c | 8 +++-----
fs/xfs/xfs_ioctl.c | 5 ++---
fs/xfs/xfs_iops.c | 2 +-
fs/xfs/xfs_itable.c | 2 +-
fs/xfs/xfs_log_recover.c | 2 +-
9 files changed, 24 insertions(+), 44 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 34ccf162abe1..7384b9194922 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -209,16 +209,14 @@ xfs_inode_from_disk(
struct xfs_icdinode *to = &ip->i_d;
struct inode *inode = VFS_I(ip);
-
/*
* Convert v1 inodes immediately to v2 inode format as this is the
* minimum inode version format we support in the rest of the code.
+ * They will also be unconditionally written back to disk as v2 inodes.
*/
- to->di_version = from->di_version;
- if (to->di_version == 1) {
+ if (unlikely(from->di_version == 1)) {
set_nlink(inode, be16_to_cpu(from->di_onlink));
to->di_projid = 0;
- to->di_version = 2;
} else {
set_nlink(inode, be32_to_cpu(from->di_nlink));
to->di_projid = (prid_t)be16_to_cpu(from->di_projid_hi) << 16 |
@@ -256,7 +254,7 @@ xfs_inode_from_disk(
to->di_dmstate = be16_to_cpu(from->di_dmstate);
to->di_flags = be16_to_cpu(from->di_flags);
- if (to->di_version == 3) {
+ if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
inode_set_iversion_queried(inode,
be64_to_cpu(from->di_changecount));
to->di_crtime.tv_sec = be32_to_cpu(from->di_crtime.t_sec);
@@ -278,7 +276,6 @@ xfs_inode_to_disk(
to->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
to->di_onlink = 0;
- to->di_version = from->di_version;
to->di_format = from->di_format;
to->di_uid = cpu_to_be32(i_uid_read(inode));
to->di_gid = cpu_to_be32(i_gid_read(inode));
@@ -307,7 +304,8 @@ xfs_inode_to_disk(
to->di_dmstate = cpu_to_be16(from->di_dmstate);
to->di_flags = cpu_to_be16(from->di_flags);
- if (from->di_version == 3) {
+ if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
+ to->di_version = 3;
to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
to->di_crtime.t_sec = cpu_to_be32(from->di_crtime.tv_sec);
to->di_crtime.t_nsec = cpu_to_be32(from->di_crtime.tv_nsec);
@@ -319,6 +317,7 @@ xfs_inode_to_disk(
uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
to->di_flushiter = 0;
} else {
+ to->di_version = 2;
to->di_flushiter = cpu_to_be16(from->di_flushiter);
}
}
@@ -636,7 +635,6 @@ xfs_iread(
xfs_sb_version_has_large_dinode(&mp->m_sb) &&
!(mp->m_flags & XFS_MOUNT_IKEEP)) {
VFS_I(ip)->i_generation = prandom_u32();
- ip->i_d.di_version = 3;
return 0;
}
@@ -678,7 +676,6 @@ xfs_iread(
* Partial initialisation of the in-core inode. Just the bits
* that xfs_ialloc won't overwrite or relies on being correct.
*/
- ip->i_d.di_version = dip->di_version;
VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
@@ -692,7 +689,6 @@ xfs_iread(
VFS_I(ip)->i_mode = 0;
}
- ASSERT(ip->i_d.di_version >= 2);
ip->i_delayed_blks = 0;
/*
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index 2683e1e2c4a6..4e24fad3deb0 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -16,7 +16,6 @@ struct xfs_dinode;
* format specific structures at the appropriate time.
*/
struct xfs_icdinode {
- int8_t di_version; /* inode version */
int8_t di_format; /* format of di_c data */
uint16_t di_flushiter; /* incremented on flush */
uint32_t di_projid; /* owner's project id */
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 3df4d0af9f22..6ca37944d71f 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1449,12 +1449,12 @@ xfs_swap_extent_forks(
* event of a crash. Set the owner change log flags now and leave the
* bmbt scan as the last step.
*/
- if (ip->i_d.di_version == 3 &&
- ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
- (*target_log_flags) |= XFS_ILOG_DOWNER;
- if (tip->i_d.di_version == 3 &&
- tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
- (*src_log_flags) |= XFS_ILOG_DOWNER;
+ if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
+ if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
+ (*target_log_flags) |= XFS_ILOG_DOWNER;
+ if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
+ (*src_log_flags) |= XFS_ILOG_DOWNER;
+ }
/*
* Swap the data forks of the inodes
@@ -1489,7 +1489,7 @@ xfs_swap_extent_forks(
(*src_log_flags) |= XFS_ILOG_DEXT;
break;
case XFS_DINODE_FMT_BTREE:
- ASSERT(ip->i_d.di_version < 3 ||
+ ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
(*src_log_flags & XFS_ILOG_DOWNER));
(*src_log_flags) |= XFS_ILOG_DBROOT;
break;
@@ -1501,7 +1501,7 @@ xfs_swap_extent_forks(
break;
case XFS_DINODE_FMT_BTREE:
(*target_log_flags) |= XFS_ILOG_DBROOT;
- ASSERT(tip->i_d.di_version < 3 ||
+ ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
(*target_log_flags & XFS_ILOG_DOWNER));
break;
}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index ebfd8efb0efa..600c0b59bdd7 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -801,15 +801,6 @@ xfs_ialloc(
return error;
ASSERT(ip != NULL);
inode = VFS_I(ip);
-
- /*
- * We always convert v1 inodes to v2 now - we only support filesystems
- * with >= v2 inode capability, so there is no reason for ever leaving
- * an inode in v1 format.
- */
- if (ip->i_d.di_version == 1)
- ip->i_d.di_version = 2;
-
inode->i_mode = mode;
set_nlink(inode, nlink);
inode->i_uid = current_fsuid();
@@ -847,14 +838,13 @@ xfs_ialloc(
ip->i_d.di_dmstate = 0;
ip->i_d.di_flags = 0;
- if (ip->i_d.di_version == 3) {
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
inode_set_iversion(inode, 1);
ip->i_d.di_flags2 = 0;
ip->i_d.di_cowextsize = 0;
ip->i_d.di_crtime = tv;
}
-
flags = XFS_ILOG_CORE;
switch (mode & S_IFMT) {
case S_IFIFO:
@@ -1115,7 +1105,6 @@ xfs_bumplink(
{
xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
- ASSERT(ip->i_d.di_version > 1);
inc_nlink(VFS_I(ip));
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
}
@@ -3798,7 +3787,6 @@ xfs_iflush_int(
ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
ASSERT(iip != NULL && iip->ili_fields != 0);
- ASSERT(ip->i_d.di_version > 1);
/* set *dip = inode's place in the buffer */
dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
@@ -3859,7 +3847,7 @@ xfs_iflush_int(
* backwards compatibility with old kernels that predate logging all
* inode changes.
*/
- if (ip->i_d.di_version < 3)
+ if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
ip->i_d.di_flushiter++;
/* Check the inline fork data before we write out. */
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 451f9b6b2806..5800bfda96b2 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -305,8 +305,6 @@ xfs_inode_to_log_dinode(
struct inode *inode = VFS_I(ip);
to->di_magic = XFS_DINODE_MAGIC;
-
- to->di_version = from->di_version;
to->di_format = from->di_format;
to->di_uid = i_uid_read(inode);
to->di_gid = i_gid_read(inode);
@@ -339,7 +337,8 @@ xfs_inode_to_log_dinode(
/* log a dummy value to ensure log structure is fully initialised */
to->di_next_unlinked = NULLAGINO;
- if (from->di_version == 3) {
+ if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
+ to->di_version = 3;
to->di_changecount = inode_peek_iversion(inode);
to->di_crtime.t_sec = from->di_crtime.tv_sec;
to->di_crtime.t_nsec = from->di_crtime.tv_nsec;
@@ -351,6 +350,7 @@ xfs_inode_to_log_dinode(
uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
to->di_flushiter = 0;
} else {
+ to->di_version = 2;
to->di_flushiter = from->di_flushiter;
}
}
@@ -395,8 +395,6 @@ xfs_inode_item_format(
struct xfs_log_iovec *vecp = NULL;
struct xfs_inode_log_format *ilf;
- ASSERT(ip->i_d.di_version > 1);
-
ilf = xlog_prepare_iovec(lv, &vecp, XLOG_REG_TYPE_IFORMAT);
ilf->ilf_type = XFS_LI_INODE;
ilf->ilf_ino = ip->i_ino;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index ad825ffa7e4c..a98909cc7ecb 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1263,7 +1263,7 @@ xfs_ioctl_setattr_xflags(
/* diflags2 only valid for v3 inodes. */
di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
- if (di_flags2 && ip->i_d.di_version < 3)
+ if (di_flags2 && !xfs_sb_version_has_large_dinode(&mp->m_sb))
return -EINVAL;
ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
@@ -1601,7 +1601,6 @@ xfs_ioctl_setattr(
olddquot = xfs_qm_vop_chown(tp, ip,
&ip->i_pdquot, pdqp);
}
- ASSERT(ip->i_d.di_version > 1);
ip->i_d.di_projid = fa->fsx_projid;
}
@@ -1614,7 +1613,7 @@ xfs_ioctl_setattr(
ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
else
ip->i_d.di_extsize = 0;
- if (ip->i_d.di_version == 3 &&
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb) &&
(ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
ip->i_d.di_cowextsize = fa->fsx_cowextsize >>
mp->m_sb.sb_blocklog;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 87093a05aad7..df2cd57e984e 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -557,7 +557,7 @@ xfs_vn_getattr(
stat->blocks =
XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
- if (ip->i_d.di_version == 3) {
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
if (request_mask & STATX_BTIME) {
stat->result_mask |= STATX_BTIME;
stat->btime = ip->i_d.di_crtime;
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index d10660469884..4d4dad557c2f 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -110,7 +110,7 @@ xfs_bulkstat_one_int(
buf->bs_forkoff = XFS_IFORK_BOFF(ip);
buf->bs_version = XFS_BULKSTAT_VERSION_V5;
- if (dic->di_version == 3) {
+ if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
if (dic->di_flags2 & XFS_DIFLAG2_COWEXTSIZE)
buf->bs_cowextsize_blks = dic->di_cowextsize;
}
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 79fc85a4ff08..9db6003d125b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2869,8 +2869,8 @@ xfs_recover_inode_owner_change(
return -ENOMEM;
/* instantiate the inode */
+ ASSERT(dip->di_version >= 3);
xfs_inode_from_disk(ip, dip);
- ASSERT(ip->i_d.di_version >= 3);
error = xfs_iformat_fork(ip, dip);
if (error)
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/5] xfs: remove the di_version field from struct icdinode
2020-03-12 14:22 ` [PATCH 5/5] xfs: remove the di_version field from struct icdinode Christoph Hellwig
@ 2020-03-16 13:17 ` Brian Foster
2020-03-16 14:49 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Brian Foster @ 2020-03-16 13:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thu, Mar 12, 2020 at 03:22:35PM +0100, Christoph Hellwig wrote:
> We know the version is 3 if on a v5 file system. For earlier file
> systems formats we always upgrade the remaining v1 inodes to v2 and
> thus only use v2 inodes. Use the xfs_sb_version_has_large_dinode
> helper to check if we deal with small or large dinodes, and thus
> remove the need for the di_version field in struct icdinode.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_inode_buf.c | 16 ++++++----------
> fs/xfs/libxfs/xfs_inode_buf.h | 1 -
> fs/xfs/xfs_bmap_util.c | 16 ++++++++--------
> fs/xfs/xfs_inode.c | 16 ++--------------
> fs/xfs/xfs_inode_item.c | 8 +++-----
> fs/xfs/xfs_ioctl.c | 5 ++---
> fs/xfs/xfs_iops.c | 2 +-
> fs/xfs/xfs_itable.c | 2 +-
> fs/xfs/xfs_log_recover.c | 2 +-
> 9 files changed, 24 insertions(+), 44 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 34ccf162abe1..7384b9194922 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -209,16 +209,14 @@ xfs_inode_from_disk(
> struct xfs_icdinode *to = &ip->i_d;
> struct inode *inode = VFS_I(ip);
>
> -
> /*
> * Convert v1 inodes immediately to v2 inode format as this is the
> * minimum inode version format we support in the rest of the code.
> + * They will also be unconditionally written back to disk as v2 inodes.
> */
> - to->di_version = from->di_version;
> - if (to->di_version == 1) {
> + if (unlikely(from->di_version == 1)) {
> set_nlink(inode, be16_to_cpu(from->di_onlink));
> to->di_projid = 0;
> - to->di_version = 2;
> } else {
> set_nlink(inode, be32_to_cpu(from->di_nlink));
> to->di_projid = (prid_t)be16_to_cpu(from->di_projid_hi) << 16 |
> @@ -256,7 +254,7 @@ xfs_inode_from_disk(
> to->di_dmstate = be16_to_cpu(from->di_dmstate);
> to->di_flags = be16_to_cpu(from->di_flags);
>
> - if (to->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> inode_set_iversion_queried(inode,
> be64_to_cpu(from->di_changecount));
> to->di_crtime.tv_sec = be32_to_cpu(from->di_crtime.t_sec);
> @@ -278,7 +276,6 @@ xfs_inode_to_disk(
> to->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> to->di_onlink = 0;
>
> - to->di_version = from->di_version;
> to->di_format = from->di_format;
> to->di_uid = cpu_to_be32(i_uid_read(inode));
> to->di_gid = cpu_to_be32(i_gid_read(inode));
> @@ -307,7 +304,8 @@ xfs_inode_to_disk(
> to->di_dmstate = cpu_to_be16(from->di_dmstate);
> to->di_flags = cpu_to_be16(from->di_flags);
>
> - if (from->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> + to->di_version = 3;
> to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
> to->di_crtime.t_sec = cpu_to_be32(from->di_crtime.tv_sec);
> to->di_crtime.t_nsec = cpu_to_be32(from->di_crtime.tv_nsec);
> @@ -319,6 +317,7 @@ xfs_inode_to_disk(
> uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
> to->di_flushiter = 0;
> } else {
> + to->di_version = 2;
> to->di_flushiter = cpu_to_be16(from->di_flushiter);
> }
> }
> @@ -636,7 +635,6 @@ xfs_iread(
> xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> VFS_I(ip)->i_generation = prandom_u32();
> - ip->i_d.di_version = 3;
> return 0;
> }
>
> @@ -678,7 +676,6 @@ xfs_iread(
> * Partial initialisation of the in-core inode. Just the bits
> * that xfs_ialloc won't overwrite or relies on being correct.
> */
> - ip->i_d.di_version = dip->di_version;
> VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
> ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
>
> @@ -692,7 +689,6 @@ xfs_iread(
> VFS_I(ip)->i_mode = 0;
> }
>
> - ASSERT(ip->i_d.di_version >= 2);
> ip->i_delayed_blks = 0;
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index 2683e1e2c4a6..4e24fad3deb0 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -16,7 +16,6 @@ struct xfs_dinode;
> * format specific structures at the appropriate time.
> */
> struct xfs_icdinode {
> - int8_t di_version; /* inode version */
> int8_t di_format; /* format of di_c data */
> uint16_t di_flushiter; /* incremented on flush */
> uint32_t di_projid; /* owner's project id */
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 3df4d0af9f22..6ca37944d71f 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1449,12 +1449,12 @@ xfs_swap_extent_forks(
> * event of a crash. Set the owner change log flags now and leave the
> * bmbt scan as the last step.
> */
> - if (ip->i_d.di_version == 3 &&
> - ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> - (*target_log_flags) |= XFS_ILOG_DOWNER;
> - if (tip->i_d.di_version == 3 &&
> - tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> - (*src_log_flags) |= XFS_ILOG_DOWNER;
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> + if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> + (*target_log_flags) |= XFS_ILOG_DOWNER;
> + if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> + (*src_log_flags) |= XFS_ILOG_DOWNER;
> + }
>
> /*
> * Swap the data forks of the inodes
> @@ -1489,7 +1489,7 @@ xfs_swap_extent_forks(
> (*src_log_flags) |= XFS_ILOG_DEXT;
> break;
> case XFS_DINODE_FMT_BTREE:
> - ASSERT(ip->i_d.di_version < 3 ||
> + ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
> (*src_log_flags & XFS_ILOG_DOWNER));
> (*src_log_flags) |= XFS_ILOG_DBROOT;
> break;
> @@ -1501,7 +1501,7 @@ xfs_swap_extent_forks(
> break;
> case XFS_DINODE_FMT_BTREE:
> (*target_log_flags) |= XFS_ILOG_DBROOT;
> - ASSERT(tip->i_d.di_version < 3 ||
> + ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
> (*target_log_flags & XFS_ILOG_DOWNER));
> break;
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ebfd8efb0efa..600c0b59bdd7 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -801,15 +801,6 @@ xfs_ialloc(
> return error;
> ASSERT(ip != NULL);
> inode = VFS_I(ip);
> -
> - /*
> - * We always convert v1 inodes to v2 now - we only support filesystems
> - * with >= v2 inode capability, so there is no reason for ever leaving
> - * an inode in v1 format.
> - */
> - if (ip->i_d.di_version == 1)
> - ip->i_d.di_version = 2;
> -
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> inode->i_uid = current_fsuid();
> @@ -847,14 +838,13 @@ xfs_ialloc(
> ip->i_d.di_dmstate = 0;
> ip->i_d.di_flags = 0;
>
> - if (ip->i_d.di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> inode_set_iversion(inode, 1);
> ip->i_d.di_flags2 = 0;
> ip->i_d.di_cowextsize = 0;
> ip->i_d.di_crtime = tv;
> }
>
> -
> flags = XFS_ILOG_CORE;
> switch (mode & S_IFMT) {
> case S_IFIFO:
> @@ -1115,7 +1105,6 @@ xfs_bumplink(
> {
> xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>
> - ASSERT(ip->i_d.di_version > 1);
> inc_nlink(VFS_I(ip));
> xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> }
> @@ -3798,7 +3787,6 @@ xfs_iflush_int(
> ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
> ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
> ASSERT(iip != NULL && iip->ili_fields != 0);
> - ASSERT(ip->i_d.di_version > 1);
>
> /* set *dip = inode's place in the buffer */
> dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
> @@ -3859,7 +3847,7 @@ xfs_iflush_int(
> * backwards compatibility with old kernels that predate logging all
> * inode changes.
> */
> - if (ip->i_d.di_version < 3)
> + if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
> ip->i_d.di_flushiter++;
>
> /* Check the inline fork data before we write out. */
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 451f9b6b2806..5800bfda96b2 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -305,8 +305,6 @@ xfs_inode_to_log_dinode(
> struct inode *inode = VFS_I(ip);
>
> to->di_magic = XFS_DINODE_MAGIC;
> -
> - to->di_version = from->di_version;
> to->di_format = from->di_format;
> to->di_uid = i_uid_read(inode);
> to->di_gid = i_gid_read(inode);
> @@ -339,7 +337,8 @@ xfs_inode_to_log_dinode(
> /* log a dummy value to ensure log structure is fully initialised */
> to->di_next_unlinked = NULLAGINO;
>
> - if (from->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> + to->di_version = 3;
> to->di_changecount = inode_peek_iversion(inode);
> to->di_crtime.t_sec = from->di_crtime.tv_sec;
> to->di_crtime.t_nsec = from->di_crtime.tv_nsec;
> @@ -351,6 +350,7 @@ xfs_inode_to_log_dinode(
> uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
> to->di_flushiter = 0;
> } else {
> + to->di_version = 2;
> to->di_flushiter = from->di_flushiter;
> }
> }
> @@ -395,8 +395,6 @@ xfs_inode_item_format(
> struct xfs_log_iovec *vecp = NULL;
> struct xfs_inode_log_format *ilf;
>
> - ASSERT(ip->i_d.di_version > 1);
> -
> ilf = xlog_prepare_iovec(lv, &vecp, XLOG_REG_TYPE_IFORMAT);
> ilf->ilf_type = XFS_LI_INODE;
> ilf->ilf_ino = ip->i_ino;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index ad825ffa7e4c..a98909cc7ecb 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1263,7 +1263,7 @@ xfs_ioctl_setattr_xflags(
>
> /* diflags2 only valid for v3 inodes. */
> di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
> - if (di_flags2 && ip->i_d.di_version < 3)
> + if (di_flags2 && !xfs_sb_version_has_large_dinode(&mp->m_sb))
> return -EINVAL;
>
> ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> @@ -1601,7 +1601,6 @@ xfs_ioctl_setattr(
> olddquot = xfs_qm_vop_chown(tp, ip,
> &ip->i_pdquot, pdqp);
> }
> - ASSERT(ip->i_d.di_version > 1);
> ip->i_d.di_projid = fa->fsx_projid;
> }
>
> @@ -1614,7 +1613,7 @@ xfs_ioctl_setattr(
> ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> else
> ip->i_d.di_extsize = 0;
> - if (ip->i_d.di_version == 3 &&
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
> ip->i_d.di_cowextsize = fa->fsx_cowextsize >>
> mp->m_sb.sb_blocklog;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 87093a05aad7..df2cd57e984e 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -557,7 +557,7 @@ xfs_vn_getattr(
> stat->blocks =
> XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>
> - if (ip->i_d.di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> if (request_mask & STATX_BTIME) {
> stat->result_mask |= STATX_BTIME;
> stat->btime = ip->i_d.di_crtime;
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index d10660469884..4d4dad557c2f 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -110,7 +110,7 @@ xfs_bulkstat_one_int(
> buf->bs_forkoff = XFS_IFORK_BOFF(ip);
> buf->bs_version = XFS_BULKSTAT_VERSION_V5;
>
> - if (dic->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> if (dic->di_flags2 & XFS_DIFLAG2_COWEXTSIZE)
> buf->bs_cowextsize_blks = dic->di_cowextsize;
> }
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 79fc85a4ff08..9db6003d125b 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2869,8 +2869,8 @@ xfs_recover_inode_owner_change(
> return -ENOMEM;
>
> /* instantiate the inode */
> + ASSERT(dip->di_version >= 3);
> xfs_inode_from_disk(ip, dip);
> - ASSERT(ip->i_d.di_version >= 3);
>
> error = xfs_iformat_fork(ip, dip);
> if (error)
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 5/5] xfs: remove the di_version field from struct icdinode
2020-03-12 14:22 ` [PATCH 5/5] xfs: remove the di_version field from struct icdinode Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
@ 2020-03-16 14:49 ` Chandan Rajendra
1 sibling, 0 replies; 21+ messages in thread
From: Chandan Rajendra @ 2020-03-16 14:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote:
> We know the version is 3 if on a v5 file system. For earlier file
> systems formats we always upgrade the remaining v1 inodes to v2 and
> thus only use v2 inodes. Use the xfs_sb_version_has_large_dinode
> helper to check if we deal with small or large dinodes, and thus
> remove the need for the di_version field in struct icdinode.
>
I don't see any logical issues.
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 16 ++++++----------
> fs/xfs/libxfs/xfs_inode_buf.h | 1 -
> fs/xfs/xfs_bmap_util.c | 16 ++++++++--------
> fs/xfs/xfs_inode.c | 16 ++--------------
> fs/xfs/xfs_inode_item.c | 8 +++-----
> fs/xfs/xfs_ioctl.c | 5 ++---
> fs/xfs/xfs_iops.c | 2 +-
> fs/xfs/xfs_itable.c | 2 +-
> fs/xfs/xfs_log_recover.c | 2 +-
> 9 files changed, 24 insertions(+), 44 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 34ccf162abe1..7384b9194922 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -209,16 +209,14 @@ xfs_inode_from_disk(
> struct xfs_icdinode *to = &ip->i_d;
> struct inode *inode = VFS_I(ip);
>
> -
> /*
> * Convert v1 inodes immediately to v2 inode format as this is the
> * minimum inode version format we support in the rest of the code.
> + * They will also be unconditionally written back to disk as v2 inodes.
> */
> - to->di_version = from->di_version;
> - if (to->di_version == 1) {
> + if (unlikely(from->di_version == 1)) {
> set_nlink(inode, be16_to_cpu(from->di_onlink));
> to->di_projid = 0;
> - to->di_version = 2;
> } else {
> set_nlink(inode, be32_to_cpu(from->di_nlink));
> to->di_projid = (prid_t)be16_to_cpu(from->di_projid_hi) << 16 |
> @@ -256,7 +254,7 @@ xfs_inode_from_disk(
> to->di_dmstate = be16_to_cpu(from->di_dmstate);
> to->di_flags = be16_to_cpu(from->di_flags);
>
> - if (to->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> inode_set_iversion_queried(inode,
> be64_to_cpu(from->di_changecount));
> to->di_crtime.tv_sec = be32_to_cpu(from->di_crtime.t_sec);
> @@ -278,7 +276,6 @@ xfs_inode_to_disk(
> to->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> to->di_onlink = 0;
>
> - to->di_version = from->di_version;
> to->di_format = from->di_format;
> to->di_uid = cpu_to_be32(i_uid_read(inode));
> to->di_gid = cpu_to_be32(i_gid_read(inode));
> @@ -307,7 +304,8 @@ xfs_inode_to_disk(
> to->di_dmstate = cpu_to_be16(from->di_dmstate);
> to->di_flags = cpu_to_be16(from->di_flags);
>
> - if (from->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> + to->di_version = 3;
> to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
> to->di_crtime.t_sec = cpu_to_be32(from->di_crtime.tv_sec);
> to->di_crtime.t_nsec = cpu_to_be32(from->di_crtime.tv_nsec);
> @@ -319,6 +317,7 @@ xfs_inode_to_disk(
> uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
> to->di_flushiter = 0;
> } else {
> + to->di_version = 2;
> to->di_flushiter = cpu_to_be16(from->di_flushiter);
> }
> }
> @@ -636,7 +635,6 @@ xfs_iread(
> xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> VFS_I(ip)->i_generation = prandom_u32();
> - ip->i_d.di_version = 3;
> return 0;
> }
>
> @@ -678,7 +676,6 @@ xfs_iread(
> * Partial initialisation of the in-core inode. Just the bits
> * that xfs_ialloc won't overwrite or relies on being correct.
> */
> - ip->i_d.di_version = dip->di_version;
> VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
> ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
>
> @@ -692,7 +689,6 @@ xfs_iread(
> VFS_I(ip)->i_mode = 0;
> }
>
> - ASSERT(ip->i_d.di_version >= 2);
> ip->i_delayed_blks = 0;
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index 2683e1e2c4a6..4e24fad3deb0 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -16,7 +16,6 @@ struct xfs_dinode;
> * format specific structures at the appropriate time.
> */
> struct xfs_icdinode {
> - int8_t di_version; /* inode version */
> int8_t di_format; /* format of di_c data */
> uint16_t di_flushiter; /* incremented on flush */
> uint32_t di_projid; /* owner's project id */
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 3df4d0af9f22..6ca37944d71f 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1449,12 +1449,12 @@ xfs_swap_extent_forks(
> * event of a crash. Set the owner change log flags now and leave the
> * bmbt scan as the last step.
> */
> - if (ip->i_d.di_version == 3 &&
> - ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> - (*target_log_flags) |= XFS_ILOG_DOWNER;
> - if (tip->i_d.di_version == 3 &&
> - tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> - (*src_log_flags) |= XFS_ILOG_DOWNER;
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> + if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> + (*target_log_flags) |= XFS_ILOG_DOWNER;
> + if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> + (*src_log_flags) |= XFS_ILOG_DOWNER;
> + }
>
> /*
> * Swap the data forks of the inodes
> @@ -1489,7 +1489,7 @@ xfs_swap_extent_forks(
> (*src_log_flags) |= XFS_ILOG_DEXT;
> break;
> case XFS_DINODE_FMT_BTREE:
> - ASSERT(ip->i_d.di_version < 3 ||
> + ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
> (*src_log_flags & XFS_ILOG_DOWNER));
> (*src_log_flags) |= XFS_ILOG_DBROOT;
> break;
> @@ -1501,7 +1501,7 @@ xfs_swap_extent_forks(
> break;
> case XFS_DINODE_FMT_BTREE:
> (*target_log_flags) |= XFS_ILOG_DBROOT;
> - ASSERT(tip->i_d.di_version < 3 ||
> + ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
> (*target_log_flags & XFS_ILOG_DOWNER));
> break;
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ebfd8efb0efa..600c0b59bdd7 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -801,15 +801,6 @@ xfs_ialloc(
> return error;
> ASSERT(ip != NULL);
> inode = VFS_I(ip);
> -
> - /*
> - * We always convert v1 inodes to v2 now - we only support filesystems
> - * with >= v2 inode capability, so there is no reason for ever leaving
> - * an inode in v1 format.
> - */
> - if (ip->i_d.di_version == 1)
> - ip->i_d.di_version = 2;
> -
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> inode->i_uid = current_fsuid();
> @@ -847,14 +838,13 @@ xfs_ialloc(
> ip->i_d.di_dmstate = 0;
> ip->i_d.di_flags = 0;
>
> - if (ip->i_d.di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> inode_set_iversion(inode, 1);
> ip->i_d.di_flags2 = 0;
> ip->i_d.di_cowextsize = 0;
> ip->i_d.di_crtime = tv;
> }
>
> -
> flags = XFS_ILOG_CORE;
> switch (mode & S_IFMT) {
> case S_IFIFO:
> @@ -1115,7 +1105,6 @@ xfs_bumplink(
> {
> xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>
> - ASSERT(ip->i_d.di_version > 1);
> inc_nlink(VFS_I(ip));
> xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> }
> @@ -3798,7 +3787,6 @@ xfs_iflush_int(
> ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
> ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
> ASSERT(iip != NULL && iip->ili_fields != 0);
> - ASSERT(ip->i_d.di_version > 1);
>
> /* set *dip = inode's place in the buffer */
> dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
> @@ -3859,7 +3847,7 @@ xfs_iflush_int(
> * backwards compatibility with old kernels that predate logging all
> * inode changes.
> */
> - if (ip->i_d.di_version < 3)
> + if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
> ip->i_d.di_flushiter++;
>
> /* Check the inline fork data before we write out. */
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 451f9b6b2806..5800bfda96b2 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -305,8 +305,6 @@ xfs_inode_to_log_dinode(
> struct inode *inode = VFS_I(ip);
>
> to->di_magic = XFS_DINODE_MAGIC;
> -
> - to->di_version = from->di_version;
> to->di_format = from->di_format;
> to->di_uid = i_uid_read(inode);
> to->di_gid = i_gid_read(inode);
> @@ -339,7 +337,8 @@ xfs_inode_to_log_dinode(
> /* log a dummy value to ensure log structure is fully initialised */
> to->di_next_unlinked = NULLAGINO;
>
> - if (from->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> + to->di_version = 3;
> to->di_changecount = inode_peek_iversion(inode);
> to->di_crtime.t_sec = from->di_crtime.tv_sec;
> to->di_crtime.t_nsec = from->di_crtime.tv_nsec;
> @@ -351,6 +350,7 @@ xfs_inode_to_log_dinode(
> uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
> to->di_flushiter = 0;
> } else {
> + to->di_version = 2;
> to->di_flushiter = from->di_flushiter;
> }
> }
> @@ -395,8 +395,6 @@ xfs_inode_item_format(
> struct xfs_log_iovec *vecp = NULL;
> struct xfs_inode_log_format *ilf;
>
> - ASSERT(ip->i_d.di_version > 1);
> -
> ilf = xlog_prepare_iovec(lv, &vecp, XLOG_REG_TYPE_IFORMAT);
> ilf->ilf_type = XFS_LI_INODE;
> ilf->ilf_ino = ip->i_ino;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index ad825ffa7e4c..a98909cc7ecb 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1263,7 +1263,7 @@ xfs_ioctl_setattr_xflags(
>
> /* diflags2 only valid for v3 inodes. */
> di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
> - if (di_flags2 && ip->i_d.di_version < 3)
> + if (di_flags2 && !xfs_sb_version_has_large_dinode(&mp->m_sb))
> return -EINVAL;
>
> ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> @@ -1601,7 +1601,6 @@ xfs_ioctl_setattr(
> olddquot = xfs_qm_vop_chown(tp, ip,
> &ip->i_pdquot, pdqp);
> }
> - ASSERT(ip->i_d.di_version > 1);
> ip->i_d.di_projid = fa->fsx_projid;
> }
>
> @@ -1614,7 +1613,7 @@ xfs_ioctl_setattr(
> ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> else
> ip->i_d.di_extsize = 0;
> - if (ip->i_d.di_version == 3 &&
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
> ip->i_d.di_cowextsize = fa->fsx_cowextsize >>
> mp->m_sb.sb_blocklog;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 87093a05aad7..df2cd57e984e 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -557,7 +557,7 @@ xfs_vn_getattr(
> stat->blocks =
> XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>
> - if (ip->i_d.di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> if (request_mask & STATX_BTIME) {
> stat->result_mask |= STATX_BTIME;
> stat->btime = ip->i_d.di_crtime;
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index d10660469884..4d4dad557c2f 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -110,7 +110,7 @@ xfs_bulkstat_one_int(
> buf->bs_forkoff = XFS_IFORK_BOFF(ip);
> buf->bs_version = XFS_BULKSTAT_VERSION_V5;
>
> - if (dic->di_version == 3) {
> + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> if (dic->di_flags2 & XFS_DIFLAG2_COWEXTSIZE)
> buf->bs_cowextsize_blks = dic->di_cowextsize;
> }
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 79fc85a4ff08..9db6003d125b 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2869,8 +2869,8 @@ xfs_recover_inode_owner_change(
> return -ENOMEM;
>
> /* instantiate the inode */
> + ASSERT(dip->di_version >= 3);
> xfs_inode_from_disk(ip, dip);
> - ASSERT(ip->i_d.di_version >= 3);
>
> error = xfs_iformat_fork(ip, dip);
> if (error)
>
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/5] xfs: only check the superblock version for dinode size calculation
2020-03-17 18:57 remove the di_version icdinode field v3 Christoph Hellwig
@ 2020-03-17 18:57 ` Christoph Hellwig
2020-03-18 15:32 ` Darrick J. Wong
0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-03-17 18:57 UTC (permalink / raw)
To: linux-xfs; +Cc: Brian Foster, Chandan Rajendra
The size of the dinode structure is only dependent on the file system
version, so instead of checking the individual inode version just use
the newly added xfs_sb_version_has_large_dinode helper, and simplify
various calling conventions.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++---
fs/xfs/libxfs/xfs_bmap.c | 10 ++++------
fs/xfs/libxfs/xfs_format.h | 16 ++++++++--------
fs/xfs/libxfs/xfs_ialloc.c | 2 +-
fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
fs/xfs/libxfs/xfs_inode_fork.h | 9 ++-------
fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
fs/xfs/xfs_inode_item.c | 4 ++--
fs/xfs/xfs_log_recover.c | 2 +-
fs/xfs/xfs_symlink.c | 2 +-
11 files changed, 27 insertions(+), 37 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 6eda1828a079..863444e2dda7 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -537,7 +537,7 @@ xfs_attr_shortform_bytesfit(
int offset;
/* rounded down */
- offset = (XFS_LITINO(mp, dp->i_d.di_version) - bytes) >> 3;
+ offset = (XFS_LITINO(mp) - bytes) >> 3;
if (dp->i_d.di_format == XFS_DINODE_FMT_DEV) {
minforkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
@@ -604,8 +604,7 @@ xfs_attr_shortform_bytesfit(
minforkoff = roundup(minforkoff, 8) >> 3;
/* attr fork btree root can have at least this many key/ptr pairs */
- maxforkoff = XFS_LITINO(mp, dp->i_d.di_version) -
- XFS_BMDR_SPACE_CALC(MINABTPTRS);
+ maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
maxforkoff = maxforkoff >> 3; /* rounded down */
if (offset >= maxforkoff)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 8057486c02b5..fda13cd7add0 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -193,14 +193,12 @@ xfs_default_attroffset(
struct xfs_mount *mp = ip->i_mount;
uint offset;
- if (mp->m_sb.sb_inodesize == 256) {
- offset = XFS_LITINO(mp, ip->i_d.di_version) -
- XFS_BMDR_SPACE_CALC(MINABTPTRS);
- } else {
+ if (mp->m_sb.sb_inodesize == 256)
+ offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
+ else
offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS);
- }
- ASSERT(offset < XFS_LITINO(mp, ip->i_d.di_version));
+ ASSERT(offset < XFS_LITINO(mp));
return offset;
}
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 19899d48517c..045556e78ee2 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -954,8 +954,12 @@ enum xfs_dinode_fmt {
/*
* Inode size for given fs.
*/
-#define XFS_LITINO(mp, version) \
- ((int)(((mp)->m_sb.sb_inodesize) - xfs_dinode_size(version)))
+#define XFS_DINODE_SIZE(sbp) \
+ (xfs_sb_version_has_v3inode(sbp) ? \
+ sizeof(struct xfs_dinode) : \
+ offsetof(struct xfs_dinode, di_crc))
+#define XFS_LITINO(mp) \
+ ((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(&(mp)->m_sb))
/*
* Inode data & attribute fork sizes, per inode.
@@ -964,13 +968,9 @@ enum xfs_dinode_fmt {
#define XFS_DFORK_BOFF(dip) ((int)((dip)->di_forkoff << 3))
#define XFS_DFORK_DSIZE(dip,mp) \
- (XFS_DFORK_Q(dip) ? \
- XFS_DFORK_BOFF(dip) : \
- XFS_LITINO(mp, (dip)->di_version))
+ (XFS_DFORK_Q(dip) ? XFS_DFORK_BOFF(dip) : XFS_LITINO(mp))
#define XFS_DFORK_ASIZE(dip,mp) \
- (XFS_DFORK_Q(dip) ? \
- XFS_LITINO(mp, (dip)->di_version) - XFS_DFORK_BOFF(dip) : \
- 0)
+ (XFS_DFORK_Q(dip) ? XFS_LITINO(mp) - XFS_DFORK_BOFF(dip) : 0)
#define XFS_DFORK_SIZE(dip,mp,w) \
((w) == XFS_DATA_FORK ? \
XFS_DFORK_DSIZE(dip, mp) : \
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4de61af3b840..7fcf62b324b0 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -339,7 +339,7 @@ xfs_ialloc_inode_init(
xfs_buf_zero(fbuf, 0, BBTOB(fbuf->b_length));
for (i = 0; i < M_IGEO(mp)->inodes_per_cluster; i++) {
int ioffset = i << mp->m_sb.sb_inodelog;
- uint isize = xfs_dinode_size(version);
+ uint isize = XFS_DINODE_SIZE(&mp->m_sb);
free = xfs_make_iptr(mp, fbuf, i);
free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index c862c8f1aaa9..240d74840306 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -417,7 +417,7 @@ xfs_dinode_verify_forkoff(
case XFS_DINODE_FMT_LOCAL: /* fall through ... */
case XFS_DINODE_FMT_EXTENTS: /* fall through ... */
case XFS_DINODE_FMT_BTREE:
- if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
+ if (dip->di_forkoff >= (XFS_LITINO(mp) >> 3))
return __this_address;
break;
default:
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index ad2b9c313fd2..518c6f0ec3a6 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -183,7 +183,7 @@ xfs_iformat_local(
*/
if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
xfs_warn(ip->i_mount,
- "corrupt inode %Lu (bad size %d for local fork, size = %d).",
+ "corrupt inode %Lu (bad size %d for local fork, size = %zd).",
(unsigned long long) ip->i_ino, size,
XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
xfs_inode_verifier_error(ip, -EFSCORRUPTED,
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 500333d0101e..668ee942be22 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -46,14 +46,9 @@ struct xfs_ifork {
(ip)->i_afp : \
(ip)->i_cowfp))
#define XFS_IFORK_DSIZE(ip) \
- (XFS_IFORK_Q(ip) ? \
- XFS_IFORK_BOFF(ip) : \
- XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
+ (XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
#define XFS_IFORK_ASIZE(ip) \
- (XFS_IFORK_Q(ip) ? \
- XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
- XFS_IFORK_BOFF(ip) : \
- 0)
+ (XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
#define XFS_IFORK_SIZE(ip,w) \
((w) == XFS_DATA_FORK ? \
XFS_IFORK_DSIZE(ip) : \
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 9bac0d2e56dc..e3400c9c71cd 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -424,12 +424,10 @@ struct xfs_log_dinode {
/* structure must be padded to 64 bit alignment */
};
-static inline uint xfs_log_dinode_size(int version)
-{
- if (version == 3)
- return sizeof(struct xfs_log_dinode);
- return offsetof(struct xfs_log_dinode, di_next_unlinked);
-}
+#define xfs_log_dinode_size(mp) \
+ (xfs_sb_version_has_v3inode(&(mp)->m_sb) ? \
+ sizeof(struct xfs_log_dinode) : \
+ offsetof(struct xfs_log_dinode, di_next_unlinked))
/*
* Buffer Log Format definitions
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index f021b55a0301..451f9b6b2806 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -125,7 +125,7 @@ xfs_inode_item_size(
*nvecs += 2;
*nbytes += sizeof(struct xfs_inode_log_format) +
- xfs_log_dinode_size(ip->i_d.di_version);
+ xfs_log_dinode_size(ip->i_mount);
xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
if (XFS_IFORK_Q(ip))
@@ -370,7 +370,7 @@ xfs_inode_item_format_core(
dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
- xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_d.di_version));
+ xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
}
/*
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index c467488212c2..308cc5dcac14 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
error = -EFSCORRUPTED;
goto out_release;
}
- isize = xfs_log_dinode_size(ldip->di_version);
+ isize = xfs_log_dinode_size(mp);
if (unlikely(item->ri_buf[1].i_len > isize)) {
XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
XFS_ERRLEVEL_LOW, mp, ldip,
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index ea42e25ec1bf..fa0fa3c70f1a 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -192,7 +192,7 @@ xfs_symlink(
* The symlink will fit into the inode data fork?
* There can't be any attributes so we get the whole variable part.
*/
- if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
+ if (pathlen <= XFS_LITINO(mp))
fs_blocks = 0;
else
fs_blocks = xfs_symlink_blocks(mp, pathlen);
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/5] xfs: only check the superblock version for dinode size calculation
2020-03-17 18:57 ` [PATCH 2/5] xfs: only check the superblock version for dinode size calculation Christoph Hellwig
@ 2020-03-18 15:32 ` Darrick J. Wong
0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-03-18 15:32 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Brian Foster, Chandan Rajendra
On Tue, Mar 17, 2020 at 11:57:53AM -0700, Christoph Hellwig wrote:
> The size of the dinode structure is only dependent on the file system
> version, so instead of checking the individual inode version just use
> the newly added xfs_sb_version_has_large_dinode helper, and simplify
> various calling conventions.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Blergh macros,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++---
> fs/xfs/libxfs/xfs_bmap.c | 10 ++++------
> fs/xfs/libxfs/xfs_format.h | 16 ++++++++--------
> fs/xfs/libxfs/xfs_ialloc.c | 2 +-
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.h | 9 ++-------
> fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_log_recover.c | 2 +-
> fs/xfs/xfs_symlink.c | 2 +-
> 11 files changed, 27 insertions(+), 37 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 6eda1828a079..863444e2dda7 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -537,7 +537,7 @@ xfs_attr_shortform_bytesfit(
> int offset;
>
> /* rounded down */
> - offset = (XFS_LITINO(mp, dp->i_d.di_version) - bytes) >> 3;
> + offset = (XFS_LITINO(mp) - bytes) >> 3;
>
> if (dp->i_d.di_format == XFS_DINODE_FMT_DEV) {
> minforkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
> @@ -604,8 +604,7 @@ xfs_attr_shortform_bytesfit(
> minforkoff = roundup(minforkoff, 8) >> 3;
>
> /* attr fork btree root can have at least this many key/ptr pairs */
> - maxforkoff = XFS_LITINO(mp, dp->i_d.di_version) -
> - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> + maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> maxforkoff = maxforkoff >> 3; /* rounded down */
>
> if (offset >= maxforkoff)
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 8057486c02b5..fda13cd7add0 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -193,14 +193,12 @@ xfs_default_attroffset(
> struct xfs_mount *mp = ip->i_mount;
> uint offset;
>
> - if (mp->m_sb.sb_inodesize == 256) {
> - offset = XFS_LITINO(mp, ip->i_d.di_version) -
> - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> - } else {
> + if (mp->m_sb.sb_inodesize == 256)
> + offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> + else
> offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS);
> - }
>
> - ASSERT(offset < XFS_LITINO(mp, ip->i_d.di_version));
> + ASSERT(offset < XFS_LITINO(mp));
> return offset;
> }
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 19899d48517c..045556e78ee2 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -954,8 +954,12 @@ enum xfs_dinode_fmt {
> /*
> * Inode size for given fs.
> */
> -#define XFS_LITINO(mp, version) \
> - ((int)(((mp)->m_sb.sb_inodesize) - xfs_dinode_size(version)))
> +#define XFS_DINODE_SIZE(sbp) \
> + (xfs_sb_version_has_v3inode(sbp) ? \
> + sizeof(struct xfs_dinode) : \
> + offsetof(struct xfs_dinode, di_crc))
> +#define XFS_LITINO(mp) \
> + ((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(&(mp)->m_sb))
>
> /*
> * Inode data & attribute fork sizes, per inode.
> @@ -964,13 +968,9 @@ enum xfs_dinode_fmt {
> #define XFS_DFORK_BOFF(dip) ((int)((dip)->di_forkoff << 3))
>
> #define XFS_DFORK_DSIZE(dip,mp) \
> - (XFS_DFORK_Q(dip) ? \
> - XFS_DFORK_BOFF(dip) : \
> - XFS_LITINO(mp, (dip)->di_version))
> + (XFS_DFORK_Q(dip) ? XFS_DFORK_BOFF(dip) : XFS_LITINO(mp))
> #define XFS_DFORK_ASIZE(dip,mp) \
> - (XFS_DFORK_Q(dip) ? \
> - XFS_LITINO(mp, (dip)->di_version) - XFS_DFORK_BOFF(dip) : \
> - 0)
> + (XFS_DFORK_Q(dip) ? XFS_LITINO(mp) - XFS_DFORK_BOFF(dip) : 0)
> #define XFS_DFORK_SIZE(dip,mp,w) \
> ((w) == XFS_DATA_FORK ? \
> XFS_DFORK_DSIZE(dip, mp) : \
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 4de61af3b840..7fcf62b324b0 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -339,7 +339,7 @@ xfs_ialloc_inode_init(
> xfs_buf_zero(fbuf, 0, BBTOB(fbuf->b_length));
> for (i = 0; i < M_IGEO(mp)->inodes_per_cluster; i++) {
> int ioffset = i << mp->m_sb.sb_inodelog;
> - uint isize = xfs_dinode_size(version);
> + uint isize = XFS_DINODE_SIZE(&mp->m_sb);
>
> free = xfs_make_iptr(mp, fbuf, i);
> free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index c862c8f1aaa9..240d74840306 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -417,7 +417,7 @@ xfs_dinode_verify_forkoff(
> case XFS_DINODE_FMT_LOCAL: /* fall through ... */
> case XFS_DINODE_FMT_EXTENTS: /* fall through ... */
> case XFS_DINODE_FMT_BTREE:
> - if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
> + if (dip->di_forkoff >= (XFS_LITINO(mp) >> 3))
> return __this_address;
> break;
> default:
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index ad2b9c313fd2..518c6f0ec3a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -183,7 +183,7 @@ xfs_iformat_local(
> */
> if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
> xfs_warn(ip->i_mount,
> - "corrupt inode %Lu (bad size %d for local fork, size = %d).",
> + "corrupt inode %Lu (bad size %d for local fork, size = %zd).",
> (unsigned long long) ip->i_ino, size,
> XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
> xfs_inode_verifier_error(ip, -EFSCORRUPTED,
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 500333d0101e..668ee942be22 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -46,14 +46,9 @@ struct xfs_ifork {
> (ip)->i_afp : \
> (ip)->i_cowfp))
> #define XFS_IFORK_DSIZE(ip) \
> - (XFS_IFORK_Q(ip) ? \
> - XFS_IFORK_BOFF(ip) : \
> - XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
> + (XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
> #define XFS_IFORK_ASIZE(ip) \
> - (XFS_IFORK_Q(ip) ? \
> - XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
> - XFS_IFORK_BOFF(ip) : \
> - 0)
> + (XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
> #define XFS_IFORK_SIZE(ip,w) \
> ((w) == XFS_DATA_FORK ? \
> XFS_IFORK_DSIZE(ip) : \
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 9bac0d2e56dc..e3400c9c71cd 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -424,12 +424,10 @@ struct xfs_log_dinode {
> /* structure must be padded to 64 bit alignment */
> };
>
> -static inline uint xfs_log_dinode_size(int version)
> -{
> - if (version == 3)
> - return sizeof(struct xfs_log_dinode);
> - return offsetof(struct xfs_log_dinode, di_next_unlinked);
> -}
> +#define xfs_log_dinode_size(mp) \
> + (xfs_sb_version_has_v3inode(&(mp)->m_sb) ? \
> + sizeof(struct xfs_log_dinode) : \
> + offsetof(struct xfs_log_dinode, di_next_unlinked))
>
> /*
> * Buffer Log Format definitions
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f021b55a0301..451f9b6b2806 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -125,7 +125,7 @@ xfs_inode_item_size(
>
> *nvecs += 2;
> *nbytes += sizeof(struct xfs_inode_log_format) +
> - xfs_log_dinode_size(ip->i_d.di_version);
> + xfs_log_dinode_size(ip->i_mount);
>
> xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
> if (XFS_IFORK_Q(ip))
> @@ -370,7 +370,7 @@ xfs_inode_item_format_core(
>
> dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
> xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
> - xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_d.di_version));
> + xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
> }
>
> /*
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index c467488212c2..308cc5dcac14 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
> error = -EFSCORRUPTED;
> goto out_release;
> }
> - isize = xfs_log_dinode_size(ldip->di_version);
> + isize = xfs_log_dinode_size(mp);
> if (unlikely(item->ri_buf[1].i_len > isize)) {
> XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
> XFS_ERRLEVEL_LOW, mp, ldip,
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index ea42e25ec1bf..fa0fa3c70f1a 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -192,7 +192,7 @@ xfs_symlink(
> * The symlink will fit into the inode data fork?
> * There can't be any attributes so we get the whole variable part.
> */
> - if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
> + if (pathlen <= XFS_LITINO(mp))
> fs_blocks = 0;
> else
> fs_blocks = xfs_symlink_blocks(mp, pathlen);
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread