Message ID | 20210714041912.2625692-11-david@fromorbit.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: rework feature flags | expand |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Jul 14, 2021 at 02:19:06PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Reporting filesystem features to userspace is currently superblock > based. Now we have a general mount-based feature infrastructure, > switch to using the xfs_mount rather than the superblock directly. > > This reduces the size of the function by over 300 bytes. > > $ size -t fs/xfs/built-in.a > text data bss dec hex filename > before 1127855 311352 484 1439691 15f7cb (TOTALS) > after 1127535 311352 484 1439371 15f68b (TOTALS) > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_sb.c | 46 ++++++++++++++++++++++-------------------- > fs/xfs/libxfs/xfs_sb.h | 2 +- > fs/xfs/xfs_ioctl.c | 2 +- > fs/xfs/xfs_ioctl32.c | 2 +- > 4 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index baaec7e6a975..5eaf14b6fe3c 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -1016,10 +1016,12 @@ xfs_sync_sb_buf( > > void > xfs_fs_geometry( > - struct xfs_sb *sbp, > + struct xfs_mount *mp, Hmm. I /think/ this won't cause problems in userspace, because db passes in &mp->m_sb, and mkfs passes in sbp, which points to &mp->m_sb, and mp itself points to &mbuf... > struct xfs_fsop_geom *geo, > int struct_version) > { > + struct xfs_sb *sbp = &mp->m_sb; > + > memset(geo, 0, sizeof(struct xfs_fsop_geom)); > > geo->blocksize = sbp->sb_blocksize; > @@ -1050,51 +1052,51 @@ xfs_fs_geometry( > geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK | > XFS_FSOP_GEOM_FLAGS_DIRV2 | > XFS_FSOP_GEOM_FLAGS_EXTFLG; > - if (xfs_sb_version_hasattr(sbp)) > + if (xfs_has_attr(mp)) ...provided those utilities set m_features before we get to this point, right? And provided that libxfs_init will take care of that, I think only mkfs would need special consideration, right? --D > geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR; > - if (xfs_sb_version_hasquota(sbp)) > + if (xfs_has_quota(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_QUOTA; > - if (xfs_sb_version_hasalign(sbp)) > + if (xfs_has_align(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_IALIGN; > - if (xfs_sb_version_hasdalign(sbp)) > + if (xfs_has_dalign(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_DALIGN; > - if (xfs_sb_version_hassector(sbp)) > - geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR; > - if (xfs_sb_version_hasasciici(sbp)) > + if (xfs_has_asciici(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_DIRV2CI; > - if (xfs_sb_version_haslazysbcount(sbp)) > + if (xfs_has_lazysbcount(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_LAZYSB; > - if (xfs_sb_version_hasattr2(sbp)) > + if (xfs_has_attr2(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR2; > - if (xfs_sb_version_hasprojid32(sbp)) > + if (xfs_has_projid32(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_PROJID32; > - if (xfs_sb_version_hascrc(sbp)) > + if (xfs_has_crc(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_V5SB; > - if (xfs_sb_version_hasftype(sbp)) > + if (xfs_has_ftype(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_FTYPE; > - if (xfs_sb_version_hasfinobt(sbp)) > + if (xfs_has_finobt(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_FINOBT; > - if (xfs_sb_version_hassparseinodes(sbp)) > + if (xfs_has_sparseinodes(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_SPINODES; > - if (xfs_sb_version_hasrmapbt(sbp)) > + if (xfs_has_rmapbt(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_RMAPBT; > - if (xfs_sb_version_hasreflink(sbp)) > + if (xfs_has_reflink(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_REFLINK; > - if (xfs_sb_version_hasbigtime(sbp)) > + if (xfs_has_bigtime(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_BIGTIME; > - if (xfs_sb_version_hasinobtcounts(sbp)) > + if (xfs_has_inobtcounts(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_INOBTCNT; > - if (xfs_sb_version_hassector(sbp)) > + if (xfs_has_sector(mp)) { > + geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR; > geo->logsectsize = sbp->sb_logsectsize; > - else > + } else { > geo->logsectsize = BBSIZE; > + } > geo->rtsectsize = sbp->sb_blocksize; > geo->dirblocksize = xfs_dir2_dirblock_bytes(sbp); > > if (struct_version < 4) > return; > > - if (xfs_sb_version_haslogv2(sbp)) > + if (xfs_has_logv2(mp)) > geo->flags |= XFS_FSOP_GEOM_FLAGS_LOGV2; > > geo->logsunit = sbp->sb_logsunit; > diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h > index d2dd99cb6921..8902f4bfa5df 100644 > --- a/fs/xfs/libxfs/xfs_sb.h > +++ b/fs/xfs/libxfs/xfs_sb.h > @@ -25,7 +25,7 @@ extern uint64_t xfs_sb_version_to_features(struct xfs_sb *sbp); > extern int xfs_update_secondary_sbs(struct xfs_mount *mp); > > #define XFS_FS_GEOM_MAX_STRUCT_VER (4) > -extern void xfs_fs_geometry(struct xfs_sb *sbp, struct xfs_fsop_geom *geo, > +extern void xfs_fs_geometry(struct xfs_mount *mp, struct xfs_fsop_geom *geo, > int struct_version); > extern int xfs_sb_read_secondary(struct xfs_mount *mp, > struct xfs_trans *tp, xfs_agnumber_t agno, > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index d54f2e0875d7..320d7481736d 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1010,7 +1010,7 @@ xfs_ioc_fsgeometry( > struct xfs_fsop_geom fsgeo; > size_t len; > > - xfs_fs_geometry(&mp->m_sb, &fsgeo, struct_version); > + xfs_fs_geometry(mp, &fsgeo, struct_version); > > if (struct_version <= 3) > len = sizeof(struct xfs_fsop_geom_v1); > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index 20adf35aa37b..8783af203cfc 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -50,7 +50,7 @@ xfs_compat_ioc_fsgeometry_v1( > { > struct xfs_fsop_geom fsgeo; > > - xfs_fs_geometry(&mp->m_sb, &fsgeo, 3); > + xfs_fs_geometry(mp, &fsgeo, 3); > /* The 32-bit variant simply has some padding at the end */ > if (copy_to_user(arg32, &fsgeo, sizeof(struct compat_xfs_fsop_geom_v1))) > return -EFAULT; > -- > 2.31.1 >
On Wed, Jul 14, 2021 at 04:15:49PM -0700, Darrick J. Wong wrote: > On Wed, Jul 14, 2021 at 02:19:06PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > Reporting filesystem features to userspace is currently superblock > > based. Now we have a general mount-based feature infrastructure, > > switch to using the xfs_mount rather than the superblock directly. > > > > This reduces the size of the function by over 300 bytes. > > > > $ size -t fs/xfs/built-in.a > > text data bss dec hex filename > > before 1127855 311352 484 1439691 15f7cb (TOTALS) > > after 1127535 311352 484 1439371 15f68b (TOTALS) > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > fs/xfs/libxfs/xfs_sb.c | 46 ++++++++++++++++++++++-------------------- > > fs/xfs/libxfs/xfs_sb.h | 2 +- > > fs/xfs/xfs_ioctl.c | 2 +- > > fs/xfs/xfs_ioctl32.c | 2 +- > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > index baaec7e6a975..5eaf14b6fe3c 100644 > > --- a/fs/xfs/libxfs/xfs_sb.c > > +++ b/fs/xfs/libxfs/xfs_sb.c > > @@ -1016,10 +1016,12 @@ xfs_sync_sb_buf( > > > > void > > xfs_fs_geometry( > > - struct xfs_sb *sbp, > > + struct xfs_mount *mp, > > Hmm. I /think/ this won't cause problems in userspace, because db > passes in &mp->m_sb, and mkfs passes in sbp, which points to &mp->m_sb, > and mp itself points to &mbuf... Yes, AFAICT that is correct. mkfs.xfs has the same setup so it should work there, too. > > struct xfs_fsop_geom *geo, > > int struct_version) > > { > > + struct xfs_sb *sbp = &mp->m_sb; > > + > > memset(geo, 0, sizeof(struct xfs_fsop_geom)); > > > > geo->blocksize = sbp->sb_blocksize; > > @@ -1050,51 +1052,51 @@ xfs_fs_geometry( > > geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK | > > XFS_FSOP_GEOM_FLAGS_DIRV2 | > > XFS_FSOP_GEOM_FLAGS_EXTFLG; > > - if (xfs_sb_version_hasattr(sbp)) > > + if (xfs_has_attr(mp)) > > ...provided those utilities set m_features before we get to this point, > right? And provided that libxfs_init will take care of that, I think > only mkfs would need special consideration, right? Yup, that's how I read the usrespace situation - the superblock we pass to xfs_fs_geometry() is always attached to a mount structure, and we'll set up the features field in the mount when we first read in the superblock in libxfs_init(). mkfs might require a specific call to set up the m_features field after the superblock is finalised (i.e in initialise_mount()), but other than that I think it all just works... Cheers, Dave.
On Thu, Jul 15, 2021 at 09:35:54AM +1000, Dave Chinner wrote: > On Wed, Jul 14, 2021 at 04:15:49PM -0700, Darrick J. Wong wrote: > > On Wed, Jul 14, 2021 at 02:19:06PM +1000, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > Reporting filesystem features to userspace is currently superblock > > > based. Now we have a general mount-based feature infrastructure, > > > switch to using the xfs_mount rather than the superblock directly. > > > > > > This reduces the size of the function by over 300 bytes. > > > > > > $ size -t fs/xfs/built-in.a > > > text data bss dec hex filename > > > before 1127855 311352 484 1439691 15f7cb (TOTALS) > > > after 1127535 311352 484 1439371 15f68b (TOTALS) > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > --- > > > fs/xfs/libxfs/xfs_sb.c | 46 ++++++++++++++++++++++-------------------- > > > fs/xfs/libxfs/xfs_sb.h | 2 +- > > > fs/xfs/xfs_ioctl.c | 2 +- > > > fs/xfs/xfs_ioctl32.c | 2 +- > > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > index baaec7e6a975..5eaf14b6fe3c 100644 > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > @@ -1016,10 +1016,12 @@ xfs_sync_sb_buf( > > > > > > void > > > xfs_fs_geometry( > > > - struct xfs_sb *sbp, > > > + struct xfs_mount *mp, > > > > Hmm. I /think/ this won't cause problems in userspace, because db > > passes in &mp->m_sb, and mkfs passes in sbp, which points to &mp->m_sb, > > and mp itself points to &mbuf... > > Yes, AFAICT that is correct. mkfs.xfs has the same setup so it > should work there, too. > > > > struct xfs_fsop_geom *geo, > > > int struct_version) > > > { > > > + struct xfs_sb *sbp = &mp->m_sb; > > > + > > > memset(geo, 0, sizeof(struct xfs_fsop_geom)); > > > > > > geo->blocksize = sbp->sb_blocksize; > > > @@ -1050,51 +1052,51 @@ xfs_fs_geometry( > > > geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK | > > > XFS_FSOP_GEOM_FLAGS_DIRV2 | > > > XFS_FSOP_GEOM_FLAGS_EXTFLG; > > > - if (xfs_sb_version_hasattr(sbp)) > > > + if (xfs_has_attr(mp)) > > > > ...provided those utilities set m_features before we get to this point, > > right? And provided that libxfs_init will take care of that, I think > > only mkfs would need special consideration, right? > > Yup, that's how I read the usrespace situation - the superblock we > pass to xfs_fs_geometry() is always attached to a mount structure, > and we'll set up the features field in the mount when we first read > in the superblock in libxfs_init(). mkfs might require a specific > call to set up the m_features field after the superblock is > finalised (i.e in initialise_mount()), but other than that I > think it all just works... Ok. I'm glad you thought about userspace too! Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c index baaec7e6a975..5eaf14b6fe3c 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -1016,10 +1016,12 @@ xfs_sync_sb_buf( void xfs_fs_geometry( - struct xfs_sb *sbp, + struct xfs_mount *mp, struct xfs_fsop_geom *geo, int struct_version) { + struct xfs_sb *sbp = &mp->m_sb; + memset(geo, 0, sizeof(struct xfs_fsop_geom)); geo->blocksize = sbp->sb_blocksize; @@ -1050,51 +1052,51 @@ xfs_fs_geometry( geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK | XFS_FSOP_GEOM_FLAGS_DIRV2 | XFS_FSOP_GEOM_FLAGS_EXTFLG; - if (xfs_sb_version_hasattr(sbp)) + if (xfs_has_attr(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR; - if (xfs_sb_version_hasquota(sbp)) + if (xfs_has_quota(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_QUOTA; - if (xfs_sb_version_hasalign(sbp)) + if (xfs_has_align(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_IALIGN; - if (xfs_sb_version_hasdalign(sbp)) + if (xfs_has_dalign(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_DALIGN; - if (xfs_sb_version_hassector(sbp)) - geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR; - if (xfs_sb_version_hasasciici(sbp)) + if (xfs_has_asciici(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_DIRV2CI; - if (xfs_sb_version_haslazysbcount(sbp)) + if (xfs_has_lazysbcount(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_LAZYSB; - if (xfs_sb_version_hasattr2(sbp)) + if (xfs_has_attr2(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR2; - if (xfs_sb_version_hasprojid32(sbp)) + if (xfs_has_projid32(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_PROJID32; - if (xfs_sb_version_hascrc(sbp)) + if (xfs_has_crc(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_V5SB; - if (xfs_sb_version_hasftype(sbp)) + if (xfs_has_ftype(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_FTYPE; - if (xfs_sb_version_hasfinobt(sbp)) + if (xfs_has_finobt(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_FINOBT; - if (xfs_sb_version_hassparseinodes(sbp)) + if (xfs_has_sparseinodes(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_SPINODES; - if (xfs_sb_version_hasrmapbt(sbp)) + if (xfs_has_rmapbt(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_RMAPBT; - if (xfs_sb_version_hasreflink(sbp)) + if (xfs_has_reflink(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_REFLINK; - if (xfs_sb_version_hasbigtime(sbp)) + if (xfs_has_bigtime(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_BIGTIME; - if (xfs_sb_version_hasinobtcounts(sbp)) + if (xfs_has_inobtcounts(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_INOBTCNT; - if (xfs_sb_version_hassector(sbp)) + if (xfs_has_sector(mp)) { + geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR; geo->logsectsize = sbp->sb_logsectsize; - else + } else { geo->logsectsize = BBSIZE; + } geo->rtsectsize = sbp->sb_blocksize; geo->dirblocksize = xfs_dir2_dirblock_bytes(sbp); if (struct_version < 4) return; - if (xfs_sb_version_haslogv2(sbp)) + if (xfs_has_logv2(mp)) geo->flags |= XFS_FSOP_GEOM_FLAGS_LOGV2; geo->logsunit = sbp->sb_logsunit; diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h index d2dd99cb6921..8902f4bfa5df 100644 --- a/fs/xfs/libxfs/xfs_sb.h +++ b/fs/xfs/libxfs/xfs_sb.h @@ -25,7 +25,7 @@ extern uint64_t xfs_sb_version_to_features(struct xfs_sb *sbp); extern int xfs_update_secondary_sbs(struct xfs_mount *mp); #define XFS_FS_GEOM_MAX_STRUCT_VER (4) -extern void xfs_fs_geometry(struct xfs_sb *sbp, struct xfs_fsop_geom *geo, +extern void xfs_fs_geometry(struct xfs_mount *mp, struct xfs_fsop_geom *geo, int struct_version); extern int xfs_sb_read_secondary(struct xfs_mount *mp, struct xfs_trans *tp, xfs_agnumber_t agno, diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index d54f2e0875d7..320d7481736d 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1010,7 +1010,7 @@ xfs_ioc_fsgeometry( struct xfs_fsop_geom fsgeo; size_t len; - xfs_fs_geometry(&mp->m_sb, &fsgeo, struct_version); + xfs_fs_geometry(mp, &fsgeo, struct_version); if (struct_version <= 3) len = sizeof(struct xfs_fsop_geom_v1); diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index 20adf35aa37b..8783af203cfc 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -50,7 +50,7 @@ xfs_compat_ioc_fsgeometry_v1( { struct xfs_fsop_geom fsgeo; - xfs_fs_geometry(&mp->m_sb, &fsgeo, 3); + xfs_fs_geometry(mp, &fsgeo, 3); /* The 32-bit variant simply has some padding at the end */ if (copy_to_user(arg32, &fsgeo, sizeof(struct compat_xfs_fsop_geom_v1))) return -EFAULT;