| Message ID | 20220922054458.40826-26-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Parent Pointers | expand |
On Wed, Sep 21, 2022 at 10:44:57PM -0700, allison.henderson@oracle.com wrote: > From: Allison Henderson <allison.henderson@oracle.com> Er, did you change this patch much? I was really hoping you'd *RVB* tag it and send it back out. :) --D > > Dave and I were discussing some recent test regressions as a result of > me turning on nrext64=1 on realtime filesystems, when we noticed that > the minimum log size of a 32M filesystem jumped from 954 blocks to 4287 > blocks. > > Digging through xfs_log_calc_max_attrsetm_res, Dave noticed that @size > contains the maximum estimated amount of space needed for a local format > xattr, in bytes, but we feed this quantity to XFS_NEXTENTADD_SPACE_RES, > which requires units of blocks. This has resulted in an overestimation > of the minimum log size over the years. > > We should nominally correct this, but there's a backwards compatibility > problem -- if we enable it now, the minimum log size will decrease. If > a corrected mkfs formats a filesystem with this new smaller log size, a > user will encounter mount failures on an uncorrected kernel due to the > larger minimum log size computations there. > > However, the large extent counters feature is still EXPERIMENTAL, so we > can gate the correction on that feature (or any features that get added > after that) being enabled. Any filesystem with nrext64 or any of the > as-yet-undefined feature bits turned on will be rejected by old > uncorrected kernels, so this should be safe even in the upgrade case. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_log_rlimit.c | 43 ++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c > index 9975b93a7412..e5c606fb7a6a 100644 > --- a/fs/xfs/libxfs/xfs_log_rlimit.c > +++ b/fs/xfs/libxfs/xfs_log_rlimit.c > @@ -16,6 +16,39 @@ > #include "xfs_bmap_btree.h" > #include "xfs_trace.h" > > +/* > + * Decide if the filesystem has the parent pointer feature or any feature > + * added after that. > + */ > +static inline bool > +xfs_has_parent_or_newer_feature( > + struct xfs_mount *mp) > +{ > + if (!xfs_sb_is_v5(&mp->m_sb)) > + return false; > + > + if (xfs_sb_has_compat_feature(&mp->m_sb, ~0)) > + return true; > + > + if (xfs_sb_has_ro_compat_feature(&mp->m_sb, > + ~(XFS_SB_FEAT_RO_COMPAT_FINOBT | > + XFS_SB_FEAT_RO_COMPAT_RMAPBT | > + XFS_SB_FEAT_RO_COMPAT_REFLINK | > + XFS_SB_FEAT_RO_COMPAT_INOBTCNT))) > + return true; > + > + if (xfs_sb_has_incompat_feature(&mp->m_sb, > + ~(XFS_SB_FEAT_INCOMPAT_FTYPE | > + XFS_SB_FEAT_INCOMPAT_SPINODES | > + XFS_SB_FEAT_INCOMPAT_META_UUID | > + XFS_SB_FEAT_INCOMPAT_BIGTIME | > + XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR | > + XFS_SB_FEAT_INCOMPAT_NREXT64))) > + return true; > + > + return false; > +} > + > /* > * Calculate the maximum length in bytes that would be required for a local > * attribute value as large attributes out of line are not logged. > @@ -31,6 +64,16 @@ xfs_log_calc_max_attrsetm_res( > MAXNAMELEN - 1; > nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK); > nblks += XFS_B_TO_FSB(mp, size); > + > + /* > + * Starting with the parent pointer feature, every new fs feature > + * corrects a unit conversion error in the xattr transaction > + * reservation code that resulted in oversized minimum log size > + * computations. > + */ > + if (xfs_has_parent_or_newer_feature(mp)) > + size = XFS_B_TO_FSB(mp, size); > + > nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK); > > return M_RES(mp)->tr_attrsetm.tr_logres + > -- > 2.25.1 >
On Fri, 2022-09-23 at 14:47 -0700, Darrick J. Wong wrote: > On Wed, Sep 21, 2022 at 10:44:57PM -0700, > allison.henderson@oracle.com wrote: > > From: Allison Henderson <allison.henderson@oracle.com> > > Er, did you change this patch much? > > I was really hoping you'd *RVB* tag it and send it back out. :) Oh, I didnt change anything, but I assumed if it's unmerged it's supposed to have the submitters SOB? The sob is a sort of legal signature that you certify that the contents are clear to be open src right? TBH, most of the patches originally came from Dave or Mark, but have sort of evolved over the reviews and rebases. It's not really clear who authored what anymore, but the point is that in submitting it, you certify that no ones un-sobed code has wandered in. At least that was my understanding? > > --D > > > > > Dave and I were discussing some recent test regressions as a result > > of > > me turning on nrext64=1 on realtime filesystems, when we noticed > > that > > the minimum log size of a 32M filesystem jumped from 954 blocks to > > 4287 > > blocks. > > > > Digging through xfs_log_calc_max_attrsetm_res, Dave noticed that > > @size > > contains the maximum estimated amount of space needed for a local > > format > > xattr, in bytes, but we feed this quantity to > > XFS_NEXTENTADD_SPACE_RES, > > which requires units of blocks. This has resulted in an > > overestimation > > of the minimum log size over the years. > > > > We should nominally correct this, but there's a backwards > > compatibility > > problem -- if we enable it now, the minimum log size will > > decrease. If > > a corrected mkfs formats a filesystem with this new smaller log > > size, a > > user will encounter mount failures on an uncorrected kernel due to > > the > > larger minimum log size computations there. > > > > However, the large extent counters feature is still EXPERIMENTAL, > > so we > > can gate the correction on that feature (or any features that get > > added > > after that) being enabled. Any filesystem with nrext64 or any of > > the > > as-yet-undefined feature bits turned on will be rejected by old > > uncorrected kernels, so this should be safe even in the upgrade > > case. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > --- > > fs/xfs/libxfs/xfs_log_rlimit.c | 43 > > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c > > b/fs/xfs/libxfs/xfs_log_rlimit.c > > index 9975b93a7412..e5c606fb7a6a 100644 > > --- a/fs/xfs/libxfs/xfs_log_rlimit.c > > +++ b/fs/xfs/libxfs/xfs_log_rlimit.c > > @@ -16,6 +16,39 @@ > > #include "xfs_bmap_btree.h" > > #include "xfs_trace.h" > > > > +/* > > + * Decide if the filesystem has the parent pointer feature or any > > feature > > + * added after that. > > + */ > > +static inline bool > > +xfs_has_parent_or_newer_feature( > > + struct xfs_mount *mp) > > +{ > > + if (!xfs_sb_is_v5(&mp->m_sb)) > > + return false; > > + > > + if (xfs_sb_has_compat_feature(&mp->m_sb, ~0)) > > + return true; > > + > > + if (xfs_sb_has_ro_compat_feature(&mp->m_sb, > > + ~(XFS_SB_FEAT_RO_COMPAT_FINOBT | > > + XFS_SB_FEAT_RO_COMPAT_RMAPBT | > > + XFS_SB_FEAT_RO_COMPAT_REFLINK | > > + XFS_SB_FEAT_RO_COMPAT_INOBTCNT))) > > + return true; > > + > > + if (xfs_sb_has_incompat_feature(&mp->m_sb, > > + ~(XFS_SB_FEAT_INCOMPAT_FTYPE | > > + XFS_SB_FEAT_INCOMPAT_SPINODES | > > + XFS_SB_FEAT_INCOMPAT_META_UUID | > > + XFS_SB_FEAT_INCOMPAT_BIGTIME | > > + XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR | > > + XFS_SB_FEAT_INCOMPAT_NREXT64))) > > + return true; > > + > > + return false; > > +} > > + > > /* > > * Calculate the maximum length in bytes that would be required > > for a local > > * attribute value as large attributes out of line are not logged. > > @@ -31,6 +64,16 @@ xfs_log_calc_max_attrsetm_res( > > MAXNAMELEN - 1; > > nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK); > > nblks += XFS_B_TO_FSB(mp, size); > > + > > + /* > > + * Starting with the parent pointer feature, every new fs > > feature > > + * corrects a unit conversion error in the xattr > > transaction > > + * reservation code that resulted in oversized minimum log > > size > > + * computations. > > + */ > > + if (xfs_has_parent_or_newer_feature(mp)) > > + size = XFS_B_TO_FSB(mp, size); > > + > > nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK); > > > > return M_RES(mp)->tr_attrsetm.tr_logres + > > -- > > 2.25.1 > >
On Mon, Sep 26, 2022 at 09:50:09PM +0000, Allison Henderson wrote: > On Fri, 2022-09-23 at 14:47 -0700, Darrick J. Wong wrote: > > On Wed, Sep 21, 2022 at 10:44:57PM -0700, > > allison.henderson@oracle.com wrote: > > > From: Allison Henderson <allison.henderson@oracle.com> > > > > Er, did you change this patch much? > > > > I was really hoping you'd *RVB* tag it and send it back out. :) > Oh, I didnt change anything, but I assumed if it's unmerged it's > supposed to have the submitters SOB? The sob is a sort of legal > signature that you certify that the contents are clear to be open src > right? > > TBH, most of the patches originally came from Dave or Mark, but have > sort of evolved over the reviews and rebases. It's not really clear > who authored what anymore, but the point is that in submitting it, you > certify that no ones un-sobed code has wandered in. > > At least that was my understanding? <shrug> My understanding is that if someone sends you a patch and you add it to your tree unchanged, you're allowed to retain the From: of the original author and tag if RVB if you like, and since you didn't make any changes, you don't need to add a SOB. IOWS I /think/ you only need to add your own SOB if you're /not/ passing it along unchanged. <usual IANAL disclaimer> <<nearly said usual BANANA disclaimer>> --D > > > > > > > > --D > > > > > > > > Dave and I were discussing some recent test regressions as a result > > > of > > > me turning on nrext64=1 on realtime filesystems, when we noticed > > > that > > > the minimum log size of a 32M filesystem jumped from 954 blocks to > > > 4287 > > > blocks. > > > > > > Digging through xfs_log_calc_max_attrsetm_res, Dave noticed that > > > @size > > > contains the maximum estimated amount of space needed for a local > > > format > > > xattr, in bytes, but we feed this quantity to > > > XFS_NEXTENTADD_SPACE_RES, > > > which requires units of blocks. This has resulted in an > > > overestimation > > > of the minimum log size over the years. > > > > > > We should nominally correct this, but there's a backwards > > > compatibility > > > problem -- if we enable it now, the minimum log size will > > > decrease. If > > > a corrected mkfs formats a filesystem with this new smaller log > > > size, a > > > user will encounter mount failures on an uncorrected kernel due to > > > the > > > larger minimum log size computations there. > > > > > > However, the large extent counters feature is still EXPERIMENTAL, > > > so we > > > can gate the correction on that feature (or any features that get > > > added > > > after that) being enabled. Any filesystem with nrext64 or any of > > > the > > > as-yet-undefined feature bits turned on will be rejected by old > > > uncorrected kernels, so this should be safe even in the upgrade > > > case. > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_log_rlimit.c | 43 > > > ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 43 insertions(+) > > > > > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c > > > b/fs/xfs/libxfs/xfs_log_rlimit.c > > > index 9975b93a7412..e5c606fb7a6a 100644 > > > --- a/fs/xfs/libxfs/xfs_log_rlimit.c > > > +++ b/fs/xfs/libxfs/xfs_log_rlimit.c > > > @@ -16,6 +16,39 @@ > > > #include "xfs_bmap_btree.h" > > > #include "xfs_trace.h" > > > > > > +/* > > > + * Decide if the filesystem has the parent pointer feature or any > > > feature > > > + * added after that. > > > + */ > > > +static inline bool > > > +xfs_has_parent_or_newer_feature( > > > + struct xfs_mount *mp) > > > +{ > > > + if (!xfs_sb_is_v5(&mp->m_sb)) > > > + return false; > > > + > > > + if (xfs_sb_has_compat_feature(&mp->m_sb, ~0)) > > > + return true; > > > + > > > + if (xfs_sb_has_ro_compat_feature(&mp->m_sb, > > > + ~(XFS_SB_FEAT_RO_COMPAT_FINOBT | > > > + XFS_SB_FEAT_RO_COMPAT_RMAPBT | > > > + XFS_SB_FEAT_RO_COMPAT_REFLINK | > > > + XFS_SB_FEAT_RO_COMPAT_INOBTCNT))) > > > + return true; > > > + > > > + if (xfs_sb_has_incompat_feature(&mp->m_sb, > > > + ~(XFS_SB_FEAT_INCOMPAT_FTYPE | > > > + XFS_SB_FEAT_INCOMPAT_SPINODES | > > > + XFS_SB_FEAT_INCOMPAT_META_UUID | > > > + XFS_SB_FEAT_INCOMPAT_BIGTIME | > > > + XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR | > > > + XFS_SB_FEAT_INCOMPAT_NREXT64))) > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > /* > > > * Calculate the maximum length in bytes that would be required > > > for a local > > > * attribute value as large attributes out of line are not logged. > > > @@ -31,6 +64,16 @@ xfs_log_calc_max_attrsetm_res( > > > MAXNAMELEN - 1; > > > nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK); > > > nblks += XFS_B_TO_FSB(mp, size); > > > + > > > + /* > > > + * Starting with the parent pointer feature, every new fs > > > feature > > > + * corrects a unit conversion error in the xattr > > > transaction > > > + * reservation code that resulted in oversized minimum log > > > size > > > + * computations. > > > + */ > > > + if (xfs_has_parent_or_newer_feature(mp)) > > > + size = XFS_B_TO_FSB(mp, size); > > > + > > > nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK); > > > > > > return M_RES(mp)->tr_attrsetm.tr_logres + > > > -- > > > 2.25.1 > > > >
diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c index 9975b93a7412..e5c606fb7a6a 100644 --- a/fs/xfs/libxfs/xfs_log_rlimit.c +++ b/fs/xfs/libxfs/xfs_log_rlimit.c @@ -16,6 +16,39 @@ #include "xfs_bmap_btree.h" #include "xfs_trace.h" +/* + * Decide if the filesystem has the parent pointer feature or any feature + * added after that. + */ +static inline bool +xfs_has_parent_or_newer_feature( + struct xfs_mount *mp) +{ + if (!xfs_sb_is_v5(&mp->m_sb)) + return false; + + if (xfs_sb_has_compat_feature(&mp->m_sb, ~0)) + return true; + + if (xfs_sb_has_ro_compat_feature(&mp->m_sb, + ~(XFS_SB_FEAT_RO_COMPAT_FINOBT | + XFS_SB_FEAT_RO_COMPAT_RMAPBT | + XFS_SB_FEAT_RO_COMPAT_REFLINK | + XFS_SB_FEAT_RO_COMPAT_INOBTCNT))) + return true; + + if (xfs_sb_has_incompat_feature(&mp->m_sb, + ~(XFS_SB_FEAT_INCOMPAT_FTYPE | + XFS_SB_FEAT_INCOMPAT_SPINODES | + XFS_SB_FEAT_INCOMPAT_META_UUID | + XFS_SB_FEAT_INCOMPAT_BIGTIME | + XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR | + XFS_SB_FEAT_INCOMPAT_NREXT64))) + return true; + + return false; +} + /* * Calculate the maximum length in bytes that would be required for a local * attribute value as large attributes out of line are not logged. @@ -31,6 +64,16 @@ xfs_log_calc_max_attrsetm_res( MAXNAMELEN - 1; nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK); nblks += XFS_B_TO_FSB(mp, size); + + /* + * Starting with the parent pointer feature, every new fs feature + * corrects a unit conversion error in the xattr transaction + * reservation code that resulted in oversized minimum log size + * computations. + */ + if (xfs_has_parent_or_newer_feature(mp)) + size = XFS_B_TO_FSB(mp, size); + nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK); return M_RES(mp)->tr_attrsetm.tr_logres +