* [PATCH 1/8] xfs: refactor quota limits initialization
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:19 ` Brian Foster
2018-05-11 23:44 ` [PATCH v2 " Darrick J. Wong
2018-05-10 19:18 ` [PATCH 2/8] xfs: don't continue scrub if already corrupt Darrick J. Wong
` (6 subsequent siblings)
7 siblings, 2 replies; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Replace all the if (!error) weirdness with helper functions that follow
our regular coding practices.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_qm.c | 141 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 77 insertions(+), 64 deletions(-)
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index f927b7d72db1..9ce6dffc25f2 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -561,26 +561,87 @@ xfs_qm_set_defquota(
{
xfs_dquot_t *dqp;
struct xfs_def_quota *defq;
+ struct xfs_disk_dquot *ddqp;
int error;
error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
- if (!error) {
- xfs_disk_dquot_t *ddqp = &dqp->q_core;
+ if (error)
+ return;
- defq = xfs_get_defquota(dqp, qinf);
+ ddqp = &dqp->q_core;
+ defq = xfs_get_defquota(dqp, qinf);
- /*
- * Timers and warnings have been already set, let's just set the
- * default limits for this quota type
- */
- defq->bhardlimit = be64_to_cpu(ddqp->d_blk_hardlimit);
- defq->bsoftlimit = be64_to_cpu(ddqp->d_blk_softlimit);
- defq->ihardlimit = be64_to_cpu(ddqp->d_ino_hardlimit);
- defq->isoftlimit = be64_to_cpu(ddqp->d_ino_softlimit);
- defq->rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
- defq->rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
- xfs_qm_dqdestroy(dqp);
- }
+ /*
+ * Timers and warnings have been already set, let's just set the
+ * default limits for this quota type
+ */
+ defq->bhardlimit = be64_to_cpu(ddqp->d_blk_hardlimit);
+ defq->bsoftlimit = be64_to_cpu(ddqp->d_blk_softlimit);
+ defq->ihardlimit = be64_to_cpu(ddqp->d_ino_hardlimit);
+ defq->isoftlimit = be64_to_cpu(ddqp->d_ino_softlimit);
+ defq->rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
+ defq->rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
+ xfs_qm_dqdestroy(dqp);
+}
+
+/* Initialize quota time limits from the root dquot. */
+static void
+xfs_qm_init_timelimits(
+ struct xfs_mount *mp,
+ struct xfs_quotainfo *qinf)
+{
+ struct xfs_disk_dquot *ddqp;
+ struct xfs_dquot *dqp;
+ uint type;
+ int error;
+
+ qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
+ qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
+ qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
+ qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
+ qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
+ qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
+
+ /*
+ * We try to get the limits from the superuser's limits fields.
+ * This is quite hacky, but it is standard quota practice.
+ *
+ * Since we may not have done a quotacheck by this point, just read
+ * the dquot without attaching it to any hashtables or lists.
+ *
+ * Timers and warnings are globally set by the first timer found in
+ * user/group/proj quota types, otherwise a default value is used.
+ * This should be split into different fields per quota type.
+ */
+ if (XFS_IS_UQUOTA_RUNNING(mp))
+ type = XFS_DQ_USER;
+ else if (XFS_IS_GQUOTA_RUNNING(mp))
+ type = XFS_DQ_GROUP;
+ else
+ type = XFS_DQ_PROJ;
+ error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
+ if (error)
+ return;
+
+ ddqp = &dqp->q_core;
+ /*
+ * The warnings and timers set the grace period given to
+ * a user or group before he or she can not perform any
+ * more writing. If it is zero, a default is used.
+ */
+ qinf->qi_btimelimit = ddqp->d_btimer ?
+ be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
+ qinf->qi_itimelimit = ddqp->d_itimer ?
+ be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
+ qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
+ be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
+ qinf->qi_bwarnlimit = ddqp->d_bwarns ?
+ be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
+ qinf->qi_iwarnlimit = ddqp->d_iwarns ?
+ be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
+ qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
+ be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
+ xfs_qm_dqdestroy(dqp);
}
/*
@@ -592,8 +653,6 @@ xfs_qm_init_quotainfo(
struct xfs_mount *mp)
{
struct xfs_quotainfo *qinf;
- struct xfs_dquot *dqp;
- uint type;
int error;
ASSERT(XFS_IS_QUOTA_RUNNING(mp));
@@ -626,53 +685,7 @@ xfs_qm_init_quotainfo(
mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
- /*
- * We try to get the limits from the superuser's limits fields.
- * This is quite hacky, but it is standard quota practice.
- *
- * Since we may not have done a quotacheck by this point, just read
- * the dquot without attaching it to any hashtables or lists.
- *
- * Timers and warnings are globally set by the first timer found in
- * user/group/proj quota types, otherwise a default value is used.
- * This should be split into different fields per quota type.
- */
- if (XFS_IS_UQUOTA_RUNNING(mp))
- type = XFS_DQ_USER;
- else if (XFS_IS_GQUOTA_RUNNING(mp))
- type = XFS_DQ_GROUP;
- else
- type = XFS_DQ_PROJ;
- error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
- if (!error) {
- xfs_disk_dquot_t *ddqp = &dqp->q_core;
-
- /*
- * The warnings and timers set the grace period given to
- * a user or group before he or she can not perform any
- * more writing. If it is zero, a default is used.
- */
- qinf->qi_btimelimit = ddqp->d_btimer ?
- be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
- qinf->qi_itimelimit = ddqp->d_itimer ?
- be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
- qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
- be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
- qinf->qi_bwarnlimit = ddqp->d_bwarns ?
- be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
- qinf->qi_iwarnlimit = ddqp->d_iwarns ?
- be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
- qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
- be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
- xfs_qm_dqdestroy(dqp);
- } else {
- qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
- qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
- qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
- qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
- qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
- qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
- }
+ xfs_qm_init_timelimits(mp, qinf);
if (XFS_IS_UQUOTA_RUNNING(mp))
xfs_qm_set_defquota(mp, XFS_DQ_USER, qinf);
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 1/8] xfs: refactor quota limits initialization
2018-05-10 19:18 ` [PATCH 1/8] xfs: refactor quota limits initialization Darrick J. Wong
@ 2018-05-11 15:19 ` Brian Foster
2018-05-11 22:43 ` Darrick J. Wong
2018-05-11 23:44 ` [PATCH v2 " Darrick J. Wong
1 sibling, 1 reply; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:06PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Replace all the if (!error) weirdness with helper functions that follow
> our regular coding practices.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_qm.c | 141 ++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 77 insertions(+), 64 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index f927b7d72db1..9ce6dffc25f2 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -561,26 +561,87 @@ xfs_qm_set_defquota(
...
> +/* Initialize quota time limits from the root dquot. */
> +static void
> +xfs_qm_init_timelimits(
> + struct xfs_mount *mp,
> + struct xfs_quotainfo *qinf)
> +{
> + struct xfs_disk_dquot *ddqp;
> + struct xfs_dquot *dqp;
> + uint type;
> + int error;
> +
> + qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> + qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> + qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> + qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> + qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> + qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> +
> + /*
> + * We try to get the limits from the superuser's limits fields.
> + * This is quite hacky, but it is standard quota practice.
> + *
> + * Since we may not have done a quotacheck by this point, just read
> + * the dquot without attaching it to any hashtables or lists.
> + *
> + * Timers and warnings are globally set by the first timer found in
> + * user/group/proj quota types, otherwise a default value is used.
> + * This should be split into different fields per quota type.
> + */
> + if (XFS_IS_UQUOTA_RUNNING(mp))
> + type = XFS_DQ_USER;
> + else if (XFS_IS_GQUOTA_RUNNING(mp))
> + type = XFS_DQ_GROUP;
> + else
> + type = XFS_DQ_PROJ;
> + error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> + if (error)
> + return;
> +
> + ddqp = &dqp->q_core;
> + /*
> + * The warnings and timers set the grace period given to
> + * a user or group before he or she can not perform any
> + * more writing. If it is zero, a default is used.
> + */
> + qinf->qi_btimelimit = ddqp->d_btimer ?
> + be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
> + qinf->qi_itimelimit = ddqp->d_itimer ?
> + be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
> + qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
> + be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
> + qinf->qi_bwarnlimit = ddqp->d_bwarns ?
> + be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
> + qinf->qi_iwarnlimit = ddqp->d_iwarns ?
> + be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
> + qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
> + be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
There's no need for these ternary statements if we initialized default
values above. How about we just convert these to if checks? I.e.,
if (ddqp->d_btimer)
qinf->qi_btimelimit = be32_to_cpu(ddqp->d_btimer);
if (ddqp->d_itimer)
qinf->qi_itimelimit = be32_to_cpu(ddqp->d_itimer);
...
Otherwise looks Ok.
Brian
> + xfs_qm_dqdestroy(dqp);
> }
>
> /*
> @@ -592,8 +653,6 @@ xfs_qm_init_quotainfo(
> struct xfs_mount *mp)
> {
> struct xfs_quotainfo *qinf;
> - struct xfs_dquot *dqp;
> - uint type;
> int error;
>
> ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> @@ -626,53 +685,7 @@ xfs_qm_init_quotainfo(
>
> mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
>
> - /*
> - * We try to get the limits from the superuser's limits fields.
> - * This is quite hacky, but it is standard quota practice.
> - *
> - * Since we may not have done a quotacheck by this point, just read
> - * the dquot without attaching it to any hashtables or lists.
> - *
> - * Timers and warnings are globally set by the first timer found in
> - * user/group/proj quota types, otherwise a default value is used.
> - * This should be split into different fields per quota type.
> - */
> - if (XFS_IS_UQUOTA_RUNNING(mp))
> - type = XFS_DQ_USER;
> - else if (XFS_IS_GQUOTA_RUNNING(mp))
> - type = XFS_DQ_GROUP;
> - else
> - type = XFS_DQ_PROJ;
> - error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> - if (!error) {
> - xfs_disk_dquot_t *ddqp = &dqp->q_core;
> -
> - /*
> - * The warnings and timers set the grace period given to
> - * a user or group before he or she can not perform any
> - * more writing. If it is zero, a default is used.
> - */
> - qinf->qi_btimelimit = ddqp->d_btimer ?
> - be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
> - qinf->qi_itimelimit = ddqp->d_itimer ?
> - be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
> - qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
> - be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
> - qinf->qi_bwarnlimit = ddqp->d_bwarns ?
> - be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
> - qinf->qi_iwarnlimit = ddqp->d_iwarns ?
> - be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
> - qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
> - be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
> - xfs_qm_dqdestroy(dqp);
> - } else {
> - qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> - qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> - qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> - qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> - qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> - qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> - }
> + xfs_qm_init_timelimits(mp, qinf);
>
> if (XFS_IS_UQUOTA_RUNNING(mp))
> xfs_qm_set_defquota(mp, XFS_DQ_USER, qinf);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/8] xfs: refactor quota limits initialization
2018-05-11 15:19 ` Brian Foster
@ 2018-05-11 22:43 ` Darrick J. Wong
0 siblings, 0 replies; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-11 22:43 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Fri, May 11, 2018 at 11:19:18AM -0400, Brian Foster wrote:
> On Thu, May 10, 2018 at 12:18:06PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Replace all the if (!error) weirdness with helper functions that follow
> > our regular coding practices.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/xfs/xfs_qm.c | 141 ++++++++++++++++++++++++++++++-------------------------
> > 1 file changed, 77 insertions(+), 64 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index f927b7d72db1..9ce6dffc25f2 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -561,26 +561,87 @@ xfs_qm_set_defquota(
> ...
> > +/* Initialize quota time limits from the root dquot. */
> > +static void
> > +xfs_qm_init_timelimits(
> > + struct xfs_mount *mp,
> > + struct xfs_quotainfo *qinf)
> > +{
> > + struct xfs_disk_dquot *ddqp;
> > + struct xfs_dquot *dqp;
> > + uint type;
> > + int error;
> > +
> > + qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> > + qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> > + qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> > + qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> > + qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> > + qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> > +
> > + /*
> > + * We try to get the limits from the superuser's limits fields.
> > + * This is quite hacky, but it is standard quota practice.
> > + *
> > + * Since we may not have done a quotacheck by this point, just read
> > + * the dquot without attaching it to any hashtables or lists.
> > + *
> > + * Timers and warnings are globally set by the first timer found in
> > + * user/group/proj quota types, otherwise a default value is used.
> > + * This should be split into different fields per quota type.
> > + */
> > + if (XFS_IS_UQUOTA_RUNNING(mp))
> > + type = XFS_DQ_USER;
> > + else if (XFS_IS_GQUOTA_RUNNING(mp))
> > + type = XFS_DQ_GROUP;
> > + else
> > + type = XFS_DQ_PROJ;
> > + error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> > + if (error)
> > + return;
> > +
> > + ddqp = &dqp->q_core;
> > + /*
> > + * The warnings and timers set the grace period given to
> > + * a user or group before he or she can not perform any
> > + * more writing. If it is zero, a default is used.
> > + */
> > + qinf->qi_btimelimit = ddqp->d_btimer ?
> > + be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
> > + qinf->qi_itimelimit = ddqp->d_itimer ?
> > + be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
> > + qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
> > + be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
> > + qinf->qi_bwarnlimit = ddqp->d_bwarns ?
> > + be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
> > + qinf->qi_iwarnlimit = ddqp->d_iwarns ?
> > + be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
> > + qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
> > + be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
>
> There's no need for these ternary statements if we initialized default
> values above. How about we just convert these to if checks? I.e.,
>
> if (ddqp->d_btimer)
> qinf->qi_btimelimit = be32_to_cpu(ddqp->d_btimer);
> if (ddqp->d_itimer)
> qinf->qi_itimelimit = be32_to_cpu(ddqp->d_itimer);
> ...
>
> Otherwise looks Ok.
Fixed.
--D
> Brian
>
> > + xfs_qm_dqdestroy(dqp);
> > }
> >
> > /*
> > @@ -592,8 +653,6 @@ xfs_qm_init_quotainfo(
> > struct xfs_mount *mp)
> > {
> > struct xfs_quotainfo *qinf;
> > - struct xfs_dquot *dqp;
> > - uint type;
> > int error;
> >
> > ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> > @@ -626,53 +685,7 @@ xfs_qm_init_quotainfo(
> >
> > mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
> >
> > - /*
> > - * We try to get the limits from the superuser's limits fields.
> > - * This is quite hacky, but it is standard quota practice.
> > - *
> > - * Since we may not have done a quotacheck by this point, just read
> > - * the dquot without attaching it to any hashtables or lists.
> > - *
> > - * Timers and warnings are globally set by the first timer found in
> > - * user/group/proj quota types, otherwise a default value is used.
> > - * This should be split into different fields per quota type.
> > - */
> > - if (XFS_IS_UQUOTA_RUNNING(mp))
> > - type = XFS_DQ_USER;
> > - else if (XFS_IS_GQUOTA_RUNNING(mp))
> > - type = XFS_DQ_GROUP;
> > - else
> > - type = XFS_DQ_PROJ;
> > - error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> > - if (!error) {
> > - xfs_disk_dquot_t *ddqp = &dqp->q_core;
> > -
> > - /*
> > - * The warnings and timers set the grace period given to
> > - * a user or group before he or she can not perform any
> > - * more writing. If it is zero, a default is used.
> > - */
> > - qinf->qi_btimelimit = ddqp->d_btimer ?
> > - be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
> > - qinf->qi_itimelimit = ddqp->d_itimer ?
> > - be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
> > - qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
> > - be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
> > - qinf->qi_bwarnlimit = ddqp->d_bwarns ?
> > - be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
> > - qinf->qi_iwarnlimit = ddqp->d_iwarns ?
> > - be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
> > - qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
> > - be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
> > - xfs_qm_dqdestroy(dqp);
> > - } else {
> > - qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> > - qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> > - qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> > - qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> > - qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> > - qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> > - }
> > + xfs_qm_init_timelimits(mp, qinf);
> >
> > if (XFS_IS_UQUOTA_RUNNING(mp))
> > xfs_qm_set_defquota(mp, XFS_DQ_USER, qinf);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/8] xfs: refactor quota limits initialization
2018-05-10 19:18 ` [PATCH 1/8] xfs: refactor quota limits initialization Darrick J. Wong
2018-05-11 15:19 ` Brian Foster
@ 2018-05-11 23:44 ` Darrick J. Wong
2018-05-14 10:25 ` Brian Foster
1 sibling, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-11 23:44 UTC (permalink / raw)
To: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Replace all the if (!error) weirdness with helper functions that follow
our regular coding practices, and factor out the ternary expression soup.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: refactor out ternary expressions
---
fs/xfs/xfs_qm.c | 142 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 78 insertions(+), 64 deletions(-)
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index f927b7d72db1..913d46d01bfc 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -561,26 +561,88 @@ xfs_qm_set_defquota(
{
xfs_dquot_t *dqp;
struct xfs_def_quota *defq;
+ struct xfs_disk_dquot *ddqp;
int error;
error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
- if (!error) {
- xfs_disk_dquot_t *ddqp = &dqp->q_core;
+ if (error)
+ return;
- defq = xfs_get_defquota(dqp, qinf);
+ ddqp = &dqp->q_core;
+ defq = xfs_get_defquota(dqp, qinf);
- /*
- * Timers and warnings have been already set, let's just set the
- * default limits for this quota type
- */
- defq->bhardlimit = be64_to_cpu(ddqp->d_blk_hardlimit);
- defq->bsoftlimit = be64_to_cpu(ddqp->d_blk_softlimit);
- defq->ihardlimit = be64_to_cpu(ddqp->d_ino_hardlimit);
- defq->isoftlimit = be64_to_cpu(ddqp->d_ino_softlimit);
- defq->rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
- defq->rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
- xfs_qm_dqdestroy(dqp);
- }
+ /*
+ * Timers and warnings have been already set, let's just set the
+ * default limits for this quota type
+ */
+ defq->bhardlimit = be64_to_cpu(ddqp->d_blk_hardlimit);
+ defq->bsoftlimit = be64_to_cpu(ddqp->d_blk_softlimit);
+ defq->ihardlimit = be64_to_cpu(ddqp->d_ino_hardlimit);
+ defq->isoftlimit = be64_to_cpu(ddqp->d_ino_softlimit);
+ defq->rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
+ defq->rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
+ xfs_qm_dqdestroy(dqp);
+}
+
+/* Initialize quota time limits from the root dquot. */
+static void
+xfs_qm_init_timelimits(
+ struct xfs_mount *mp,
+ struct xfs_quotainfo *qinf)
+{
+ struct xfs_disk_dquot *ddqp;
+ struct xfs_dquot *dqp;
+ uint type;
+ int error;
+
+ qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
+ qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
+ qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
+ qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
+ qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
+ qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
+
+ /*
+ * We try to get the limits from the superuser's limits fields.
+ * This is quite hacky, but it is standard quota practice.
+ *
+ * Since we may not have done a quotacheck by this point, just read
+ * the dquot without attaching it to any hashtables or lists.
+ *
+ * Timers and warnings are globally set by the first timer found in
+ * user/group/proj quota types, otherwise a default value is used.
+ * This should be split into different fields per quota type.
+ */
+ if (XFS_IS_UQUOTA_RUNNING(mp))
+ type = XFS_DQ_USER;
+ else if (XFS_IS_GQUOTA_RUNNING(mp))
+ type = XFS_DQ_GROUP;
+ else
+ type = XFS_DQ_PROJ;
+ error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
+ if (error)
+ return;
+
+ ddqp = &dqp->q_core;
+ /*
+ * The warnings and timers set the grace period given to
+ * a user or group before he or she can not perform any
+ * more writing. If it is zero, a default is used.
+ */
+ if (ddqp->d_btimer)
+ qinf->qi_btimelimit = be32_to_cpu(ddqp->d_btimer);
+ if (ddqp->d_itimer)
+ qinf->qi_itimelimit = be32_to_cpu(ddqp->d_itimer);
+ if (ddqp->d_rtbtimer)
+ qinf->qi_rtbtimelimit = be32_to_cpu(ddqp->d_rtbtimer);
+ if (ddqp->d_bwarns)
+ qinf->qi_bwarnlimit = be32_to_cpu(ddqp->d_bwarns);
+ if (ddqp->d_iwarns)
+ qinf->qi_iwarnlimit = be32_to_cpu(ddqp->d_iwarns);
+ if (ddqp->d_rtbwarns)
+ qinf->qi_rtbwarnlimit = be32_to_cpu(ddqp->d_rtbwarns);
+
+ xfs_qm_dqdestroy(dqp);
}
/*
@@ -592,8 +654,6 @@ xfs_qm_init_quotainfo(
struct xfs_mount *mp)
{
struct xfs_quotainfo *qinf;
- struct xfs_dquot *dqp;
- uint type;
int error;
ASSERT(XFS_IS_QUOTA_RUNNING(mp));
@@ -626,53 +686,7 @@ xfs_qm_init_quotainfo(
mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
- /*
- * We try to get the limits from the superuser's limits fields.
- * This is quite hacky, but it is standard quota practice.
- *
- * Since we may not have done a quotacheck by this point, just read
- * the dquot without attaching it to any hashtables or lists.
- *
- * Timers and warnings are globally set by the first timer found in
- * user/group/proj quota types, otherwise a default value is used.
- * This should be split into different fields per quota type.
- */
- if (XFS_IS_UQUOTA_RUNNING(mp))
- type = XFS_DQ_USER;
- else if (XFS_IS_GQUOTA_RUNNING(mp))
- type = XFS_DQ_GROUP;
- else
- type = XFS_DQ_PROJ;
- error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
- if (!error) {
- xfs_disk_dquot_t *ddqp = &dqp->q_core;
-
- /*
- * The warnings and timers set the grace period given to
- * a user or group before he or she can not perform any
- * more writing. If it is zero, a default is used.
- */
- qinf->qi_btimelimit = ddqp->d_btimer ?
- be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
- qinf->qi_itimelimit = ddqp->d_itimer ?
- be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
- qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
- be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
- qinf->qi_bwarnlimit = ddqp->d_bwarns ?
- be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
- qinf->qi_iwarnlimit = ddqp->d_iwarns ?
- be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
- qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
- be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
- xfs_qm_dqdestroy(dqp);
- } else {
- qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
- qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
- qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
- qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
- qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
- qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
- }
+ xfs_qm_init_timelimits(mp, qinf);
if (XFS_IS_UQUOTA_RUNNING(mp))
xfs_qm_set_defquota(mp, XFS_DQ_USER, qinf);
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 1/8] xfs: refactor quota limits initialization
2018-05-11 23:44 ` [PATCH v2 " Darrick J. Wong
@ 2018-05-14 10:25 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-14 10:25 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Fri, May 11, 2018 at 04:44:24PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Replace all the if (!error) weirdness with helper functions that follow
> our regular coding practices, and factor out the ternary expression soup.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: refactor out ternary expressions
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_qm.c | 142 ++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 78 insertions(+), 64 deletions(-)
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index f927b7d72db1..913d46d01bfc 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -561,26 +561,88 @@ xfs_qm_set_defquota(
> {
> xfs_dquot_t *dqp;
> struct xfs_def_quota *defq;
> + struct xfs_disk_dquot *ddqp;
> int error;
>
> error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> - if (!error) {
> - xfs_disk_dquot_t *ddqp = &dqp->q_core;
> + if (error)
> + return;
>
> - defq = xfs_get_defquota(dqp, qinf);
> + ddqp = &dqp->q_core;
> + defq = xfs_get_defquota(dqp, qinf);
>
> - /*
> - * Timers and warnings have been already set, let's just set the
> - * default limits for this quota type
> - */
> - defq->bhardlimit = be64_to_cpu(ddqp->d_blk_hardlimit);
> - defq->bsoftlimit = be64_to_cpu(ddqp->d_blk_softlimit);
> - defq->ihardlimit = be64_to_cpu(ddqp->d_ino_hardlimit);
> - defq->isoftlimit = be64_to_cpu(ddqp->d_ino_softlimit);
> - defq->rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
> - defq->rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
> - xfs_qm_dqdestroy(dqp);
> - }
> + /*
> + * Timers and warnings have been already set, let's just set the
> + * default limits for this quota type
> + */
> + defq->bhardlimit = be64_to_cpu(ddqp->d_blk_hardlimit);
> + defq->bsoftlimit = be64_to_cpu(ddqp->d_blk_softlimit);
> + defq->ihardlimit = be64_to_cpu(ddqp->d_ino_hardlimit);
> + defq->isoftlimit = be64_to_cpu(ddqp->d_ino_softlimit);
> + defq->rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
> + defq->rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
> + xfs_qm_dqdestroy(dqp);
> +}
> +
> +/* Initialize quota time limits from the root dquot. */
> +static void
> +xfs_qm_init_timelimits(
> + struct xfs_mount *mp,
> + struct xfs_quotainfo *qinf)
> +{
> + struct xfs_disk_dquot *ddqp;
> + struct xfs_dquot *dqp;
> + uint type;
> + int error;
> +
> + qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> + qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> + qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> + qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> + qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> + qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> +
> + /*
> + * We try to get the limits from the superuser's limits fields.
> + * This is quite hacky, but it is standard quota practice.
> + *
> + * Since we may not have done a quotacheck by this point, just read
> + * the dquot without attaching it to any hashtables or lists.
> + *
> + * Timers and warnings are globally set by the first timer found in
> + * user/group/proj quota types, otherwise a default value is used.
> + * This should be split into different fields per quota type.
> + */
> + if (XFS_IS_UQUOTA_RUNNING(mp))
> + type = XFS_DQ_USER;
> + else if (XFS_IS_GQUOTA_RUNNING(mp))
> + type = XFS_DQ_GROUP;
> + else
> + type = XFS_DQ_PROJ;
> + error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> + if (error)
> + return;
> +
> + ddqp = &dqp->q_core;
> + /*
> + * The warnings and timers set the grace period given to
> + * a user or group before he or she can not perform any
> + * more writing. If it is zero, a default is used.
> + */
> + if (ddqp->d_btimer)
> + qinf->qi_btimelimit = be32_to_cpu(ddqp->d_btimer);
> + if (ddqp->d_itimer)
> + qinf->qi_itimelimit = be32_to_cpu(ddqp->d_itimer);
> + if (ddqp->d_rtbtimer)
> + qinf->qi_rtbtimelimit = be32_to_cpu(ddqp->d_rtbtimer);
> + if (ddqp->d_bwarns)
> + qinf->qi_bwarnlimit = be32_to_cpu(ddqp->d_bwarns);
> + if (ddqp->d_iwarns)
> + qinf->qi_iwarnlimit = be32_to_cpu(ddqp->d_iwarns);
> + if (ddqp->d_rtbwarns)
> + qinf->qi_rtbwarnlimit = be32_to_cpu(ddqp->d_rtbwarns);
> +
> + xfs_qm_dqdestroy(dqp);
> }
>
> /*
> @@ -592,8 +654,6 @@ xfs_qm_init_quotainfo(
> struct xfs_mount *mp)
> {
> struct xfs_quotainfo *qinf;
> - struct xfs_dquot *dqp;
> - uint type;
> int error;
>
> ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> @@ -626,53 +686,7 @@ xfs_qm_init_quotainfo(
>
> mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
>
> - /*
> - * We try to get the limits from the superuser's limits fields.
> - * This is quite hacky, but it is standard quota practice.
> - *
> - * Since we may not have done a quotacheck by this point, just read
> - * the dquot without attaching it to any hashtables or lists.
> - *
> - * Timers and warnings are globally set by the first timer found in
> - * user/group/proj quota types, otherwise a default value is used.
> - * This should be split into different fields per quota type.
> - */
> - if (XFS_IS_UQUOTA_RUNNING(mp))
> - type = XFS_DQ_USER;
> - else if (XFS_IS_GQUOTA_RUNNING(mp))
> - type = XFS_DQ_GROUP;
> - else
> - type = XFS_DQ_PROJ;
> - error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> - if (!error) {
> - xfs_disk_dquot_t *ddqp = &dqp->q_core;
> -
> - /*
> - * The warnings and timers set the grace period given to
> - * a user or group before he or she can not perform any
> - * more writing. If it is zero, a default is used.
> - */
> - qinf->qi_btimelimit = ddqp->d_btimer ?
> - be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
> - qinf->qi_itimelimit = ddqp->d_itimer ?
> - be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
> - qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
> - be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
> - qinf->qi_bwarnlimit = ddqp->d_bwarns ?
> - be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
> - qinf->qi_iwarnlimit = ddqp->d_iwarns ?
> - be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
> - qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
> - be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
> - xfs_qm_dqdestroy(dqp);
> - } else {
> - qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> - qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> - qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> - qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> - qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> - qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> - }
> + xfs_qm_init_timelimits(mp, qinf);
>
> if (XFS_IS_UQUOTA_RUNNING(mp))
> xfs_qm_set_defquota(mp, XFS_DQ_USER, qinf);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/8] xfs: don't continue scrub if already corrupt
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
2018-05-10 19:18 ` [PATCH 1/8] xfs: refactor quota limits initialization Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:19 ` Brian Foster
2018-05-10 19:18 ` [PATCH 3/8] xfs: quota scrub should use bmapbtd scrubber Darrick J. Wong
` (5 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
If we've already decided that something is corrupt, we might as well
abort all the loops and exit as quickly as possible.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/scrub/attr.c | 3 ++-
fs/xfs/scrub/bmap.c | 3 ++-
fs/xfs/scrub/dir.c | 35 +++++++++++++++++++++++++++--------
fs/xfs/scrub/parent.c | 3 +++
4 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index 127575f0abfb..84b6d6b66578 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -126,8 +126,9 @@ xfs_scrub_xattr_listent(
if (args.valuelen != valuelen)
xfs_scrub_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK,
args.blkno);
-
fail_xref:
+ if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ context->seen_enough = 1;
return;
}
diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index e5a4611abf86..42a115e83739 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -683,7 +683,8 @@ xfs_scrub_bmap(
info.lastoff = 0;
ifp = XFS_IFORK_PTR(ip, whichfork);
for_each_xfs_iext(ifp, &icur, &irec) {
- if (xfs_scrub_should_terminate(sc, &error))
+ if (xfs_scrub_should_terminate(sc, &error) ||
+ (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
break;
if (isnullstartblock(irec.br_startblock))
continue;
diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index 38f29806eb54..1a4309b3e786 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -172,7 +172,7 @@ xfs_scrub_dir_actor(
error = xfs_dir_lookup(sdc->sc->tp, ip, &xname, &lookup_ino, NULL);
if (!xfs_scrub_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
&error))
- goto fail_xref;
+ goto out;
if (lookup_ino != ino) {
xfs_scrub_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
goto out;
@@ -183,8 +183,13 @@ xfs_scrub_dir_actor(
if (error)
goto out;
out:
- return error;
-fail_xref:
+ /*
+ * A negative error code returned here is supposed to cause the
+ * dir_emit caller (xfs_readdir) to abort the directory iteration
+ * and return zero to xfs_scrub_directory.
+ */
+ if (error == 0 && sdc->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ return -EFSCORRUPTED;
return error;
}
@@ -240,6 +245,9 @@ xfs_scrub_dir_rec(
}
xfs_scrub_buffer_recheck(ds->sc, bp);
+ if (ds->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ goto out_relse;
+
dent = (struct xfs_dir2_data_entry *)(((char *)bp->b_addr) + off);
/* Make sure we got a real directory entry. */
@@ -357,6 +365,9 @@ xfs_scrub_directory_data_bestfree(
/* XXX: Check xfs_dir3_data_hdr.pad is zero once we start setting it. */
+ if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ goto out_buf;
+
/* Do the bestfrees correspond to actual free space? */
bf = d_ops->data_bestfree_p(bp->b_addr);
smallest_bestfree = UINT_MAX;
@@ -413,14 +424,18 @@ xfs_scrub_directory_data_bestfree(
/* Spot check this free entry */
tag = be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup));
- if (tag != ((char *)dup - (char *)bp->b_addr))
+ if (tag != ((char *)dup - (char *)bp->b_addr)) {
xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
+ goto out_buf;
+ }
/*
* Either this entry is a bestfree or it's smaller than
* any of the bestfrees.
*/
xfs_scrub_directory_check_free_entry(sc, lblk, bf, dup);
+ if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ goto out_buf;
/* Move on. */
newlen = be16_to_cpu(dup->length);
@@ -546,6 +561,8 @@ xfs_scrub_directory_leaf1_bestfree(
}
if (leafhdr.stale != stale)
xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
+ if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ goto out;
/* Check all the bestfree entries. */
for (i = 0; i < bestcount; i++, bestp++) {
@@ -556,9 +573,11 @@ xfs_scrub_directory_leaf1_bestfree(
i * args->geo->fsbcount, -1, &dbp);
if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, lblk,
&error))
- continue;
+ break;
xfs_scrub_directory_check_freesp(sc, lblk, dbp, best);
xfs_trans_brelse(sc->tp, dbp);
+ if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ goto out;
}
out:
return error;
@@ -607,7 +626,7 @@ xfs_scrub_directory_free_bestfree(
-1, &dbp);
if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, lblk,
&error))
- continue;
+ break;
xfs_scrub_directory_check_freesp(sc, lblk, dbp, best);
xfs_trans_brelse(sc->tp, dbp);
}
@@ -656,7 +675,7 @@ xfs_scrub_directory_blocks(
/* Iterate all the data extents in the directory... */
found = xfs_iext_lookup_extent(sc->ip, ifp, lblk, &icur, &got);
- while (found) {
+ while (found && !(sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)) {
/* Block directories only have a single block at offset 0. */
if (is_block &&
(got.br_startoff > 0 ||
@@ -719,7 +738,7 @@ xfs_scrub_directory_blocks(
/* Scan for free blocks */
lblk = free_lblk;
found = xfs_iext_lookup_extent(sc->ip, ifp, lblk, &icur, &got);
- while (found) {
+ while (found && !(sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)) {
/*
* Dirs can't have blocks mapped above 2^32.
* Single-block dirs shouldn't even be here.
diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
index 1fb88c18d455..fc336807e156 100644
--- a/fs/xfs/scrub/parent.c
+++ b/fs/xfs/scrub/parent.c
@@ -147,6 +147,9 @@ xfs_scrub_parent_validate(
*try_again = false;
+ if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ goto out;
+
/* '..' must not point to ourselves. */
if (sc->ip->i_ino == dnum) {
xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/8] xfs: don't continue scrub if already corrupt
2018-05-10 19:18 ` [PATCH 2/8] xfs: don't continue scrub if already corrupt Darrick J. Wong
@ 2018-05-11 15:19 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:13PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> If we've already decided that something is corrupt, we might as well
> abort all the loops and exit as quickly as possible.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/scrub/attr.c | 3 ++-
> fs/xfs/scrub/bmap.c | 3 ++-
> fs/xfs/scrub/dir.c | 35 +++++++++++++++++++++++++++--------
> fs/xfs/scrub/parent.c | 3 +++
> 4 files changed, 34 insertions(+), 10 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index 127575f0abfb..84b6d6b66578 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -126,8 +126,9 @@ xfs_scrub_xattr_listent(
> if (args.valuelen != valuelen)
> xfs_scrub_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK,
> args.blkno);
> -
> fail_xref:
> + if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + context->seen_enough = 1;
> return;
> }
>
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index e5a4611abf86..42a115e83739 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -683,7 +683,8 @@ xfs_scrub_bmap(
> info.lastoff = 0;
> ifp = XFS_IFORK_PTR(ip, whichfork);
> for_each_xfs_iext(ifp, &icur, &irec) {
> - if (xfs_scrub_should_terminate(sc, &error))
> + if (xfs_scrub_should_terminate(sc, &error) ||
> + (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> break;
> if (isnullstartblock(irec.br_startblock))
> continue;
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index 38f29806eb54..1a4309b3e786 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -172,7 +172,7 @@ xfs_scrub_dir_actor(
> error = xfs_dir_lookup(sdc->sc->tp, ip, &xname, &lookup_ino, NULL);
> if (!xfs_scrub_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
> &error))
> - goto fail_xref;
> + goto out;
> if (lookup_ino != ino) {
> xfs_scrub_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> goto out;
> @@ -183,8 +183,13 @@ xfs_scrub_dir_actor(
> if (error)
> goto out;
> out:
> - return error;
> -fail_xref:
> + /*
> + * A negative error code returned here is supposed to cause the
> + * dir_emit caller (xfs_readdir) to abort the directory iteration
> + * and return zero to xfs_scrub_directory.
> + */
> + if (error == 0 && sdc->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + return -EFSCORRUPTED;
> return error;
> }
>
> @@ -240,6 +245,9 @@ xfs_scrub_dir_rec(
> }
> xfs_scrub_buffer_recheck(ds->sc, bp);
>
> + if (ds->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + goto out_relse;
> +
> dent = (struct xfs_dir2_data_entry *)(((char *)bp->b_addr) + off);
>
> /* Make sure we got a real directory entry. */
> @@ -357,6 +365,9 @@ xfs_scrub_directory_data_bestfree(
>
> /* XXX: Check xfs_dir3_data_hdr.pad is zero once we start setting it. */
>
> + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + goto out_buf;
> +
> /* Do the bestfrees correspond to actual free space? */
> bf = d_ops->data_bestfree_p(bp->b_addr);
> smallest_bestfree = UINT_MAX;
> @@ -413,14 +424,18 @@ xfs_scrub_directory_data_bestfree(
>
> /* Spot check this free entry */
> tag = be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup));
> - if (tag != ((char *)dup - (char *)bp->b_addr))
> + if (tag != ((char *)dup - (char *)bp->b_addr)) {
> xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
> + goto out_buf;
> + }
>
> /*
> * Either this entry is a bestfree or it's smaller than
> * any of the bestfrees.
> */
> xfs_scrub_directory_check_free_entry(sc, lblk, bf, dup);
> + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + goto out_buf;
>
> /* Move on. */
> newlen = be16_to_cpu(dup->length);
> @@ -546,6 +561,8 @@ xfs_scrub_directory_leaf1_bestfree(
> }
> if (leafhdr.stale != stale)
> xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
> + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + goto out;
>
> /* Check all the bestfree entries. */
> for (i = 0; i < bestcount; i++, bestp++) {
> @@ -556,9 +573,11 @@ xfs_scrub_directory_leaf1_bestfree(
> i * args->geo->fsbcount, -1, &dbp);
> if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, lblk,
> &error))
> - continue;
> + break;
> xfs_scrub_directory_check_freesp(sc, lblk, dbp, best);
> xfs_trans_brelse(sc->tp, dbp);
> + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + goto out;
> }
> out:
> return error;
> @@ -607,7 +626,7 @@ xfs_scrub_directory_free_bestfree(
> -1, &dbp);
> if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, lblk,
> &error))
> - continue;
> + break;
> xfs_scrub_directory_check_freesp(sc, lblk, dbp, best);
> xfs_trans_brelse(sc->tp, dbp);
> }
> @@ -656,7 +675,7 @@ xfs_scrub_directory_blocks(
>
> /* Iterate all the data extents in the directory... */
> found = xfs_iext_lookup_extent(sc->ip, ifp, lblk, &icur, &got);
> - while (found) {
> + while (found && !(sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)) {
> /* Block directories only have a single block at offset 0. */
> if (is_block &&
> (got.br_startoff > 0 ||
> @@ -719,7 +738,7 @@ xfs_scrub_directory_blocks(
> /* Scan for free blocks */
> lblk = free_lblk;
> found = xfs_iext_lookup_extent(sc->ip, ifp, lblk, &icur, &got);
> - while (found) {
> + while (found && !(sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)) {
> /*
> * Dirs can't have blocks mapped above 2^32.
> * Single-block dirs shouldn't even be here.
> diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
> index 1fb88c18d455..fc336807e156 100644
> --- a/fs/xfs/scrub/parent.c
> +++ b/fs/xfs/scrub/parent.c
> @@ -147,6 +147,9 @@ xfs_scrub_parent_validate(
>
> *try_again = false;
>
> + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + goto out;
> +
> /* '..' must not point to ourselves. */
> if (sc->ip->i_ino == dnum) {
> xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/8] xfs: quota scrub should use bmapbtd scrubber
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
2018-05-10 19:18 ` [PATCH 1/8] xfs: refactor quota limits initialization Darrick J. Wong
2018-05-10 19:18 ` [PATCH 2/8] xfs: don't continue scrub if already corrupt Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:19 ` Brian Foster
2018-05-10 19:18 ` [PATCH 4/8] xfs: scrub the data fork of the realtime inodes Darrick J. Wong
` (4 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Replace the quota scrubber's open-coded data fork scrubber with a
redirected call to the bmapbtd scrubber. This strengthens the quota
scrub to include all the cross-referencing that it does.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/scrub/common.c | 57 ++++++++++++++++++++++++++++++++++
fs/xfs/scrub/common.h | 2 +
fs/xfs/scrub/quota.c | 83 ++++++++++++++++++++++++-------------------------
3 files changed, 99 insertions(+), 43 deletions(-)
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 2ac8576c4670..62b33c99efe4 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -44,6 +44,8 @@
#include "xfs_rmap_btree.h"
#include "xfs_log.h"
#include "xfs_trans_priv.h"
+#include "xfs_attr.h"
+#include "xfs_reflink.h"
#include "scrub/xfs_scrub.h"
#include "scrub/scrub.h"
#include "scrub/common.h"
@@ -787,3 +789,58 @@ xfs_scrub_buffer_recheck(
sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
trace_xfs_scrub_block_error(sc, bp->b_bn, fa);
}
+
+/*
+ * Scrub the attr/data forks of a metadata inode. The metadata inode must be
+ * pointed to by sc->ip and the ILOCK must be held.
+ */
+int
+xfs_scrub_metadata_inode_forks(
+ struct xfs_scrub_context *sc)
+{
+ __u32 smtype;
+ bool shared;
+ int error;
+
+ if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ return 0;
+
+ /* Metadata inodes don't live on the rt device. */
+ if (sc->ip->i_d.di_flags & XFS_DIFLAG_REALTIME) {
+ xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
+ return 0;
+ }
+
+ /* They should never participate in reflink. */
+ if (xfs_is_reflink_inode(sc->ip)) {
+ xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
+ return 0;
+ }
+
+ /* They also should never have extended attributes. */
+ if (xfs_inode_hasattr(sc->ip)) {
+ xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
+ return 0;
+ }
+
+ /* Invoke the data fork scrubber. */
+ smtype = sc->sm->sm_type;
+ sc->sm->sm_type = XFS_SCRUB_TYPE_BMBTD;
+ error = xfs_scrub_bmap_data(sc);
+ sc->sm->sm_type = smtype;
+ if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
+ return error;
+
+ /* Look for incorrect shared blocks. */
+ if (xfs_sb_version_hasreflink(&sc->mp->m_sb)) {
+ error = xfs_reflink_inode_has_shared_extents(sc->tp, sc->ip,
+ &shared);
+ if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, 0,
+ &error))
+ return error;
+ if (shared)
+ xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
+ }
+
+ return error;
+}
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index a23ad7fa2b6c..5d78bb9602ab 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -155,4 +155,6 @@ static inline bool xfs_scrub_skip_xref(struct xfs_scrub_metadata *sm)
XFS_SCRUB_OFLAG_XCORRUPT);
}
+int xfs_scrub_metadata_inode_forks(struct xfs_scrub_context *sc);
+
#endif /* __XFS_SCRUB_COMMON_H__ */
diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index d3d08978f53a..15ae4d23d6ac 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -206,65 +206,62 @@ xfs_scrub_quota_item(
return 0;
}
-/* Scrub all of a quota type's items. */
-int
-xfs_scrub_quota(
+/* Check the quota's data fork. */
+STATIC int
+xfs_scrub_quota_data_fork(
struct xfs_scrub_context *sc)
{
struct xfs_bmbt_irec irec = { 0 };
- struct xfs_scrub_quota_info sqi;
- struct xfs_mount *mp = sc->mp;
- struct xfs_quotainfo *qi = mp->m_quotainfo;
+ struct xfs_iext_cursor icur;
+ struct xfs_quotainfo *qi = sc->mp->m_quotainfo;
+ struct xfs_ifork *ifp;
xfs_fileoff_t max_dqid_off;
- xfs_fileoff_t off = 0;
- uint dqtype;
- int nimaps;
int error = 0;
- dqtype = xfs_scrub_quota_to_dqtype(sc);
+ /* Invoke the fork scrubber. */
+ error = xfs_scrub_metadata_inode_forks(sc);
+ if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
+ return error;
- /* Look for problem extents. */
- if (sc->ip->i_d.di_flags & XFS_DIFLAG_REALTIME) {
- xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
- goto out;
- }
+ /* Check for data fork problems that apply only to quota files. */
max_dqid_off = ((xfs_dqid_t)-1) / qi->qi_dqperchunk;
- while (1) {
+ ifp = XFS_IFORK_PTR(sc->ip, XFS_DATA_FORK);
+ for_each_xfs_iext(ifp, &icur, &irec) {
if (xfs_scrub_should_terminate(sc, &error))
break;
-
- off = irec.br_startoff + irec.br_blockcount;
- nimaps = 1;
- error = xfs_bmapi_read(sc->ip, off, -1, &irec, &nimaps,
- XFS_BMAPI_ENTIRE);
- if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, off,
- &error))
- goto out;
- if (!nimaps)
- break;
- if (irec.br_startblock == HOLESTARTBLOCK)
- continue;
-
- /* Check the extent record doesn't point to crap. */
- if (irec.br_startblock + irec.br_blockcount <=
- irec.br_startblock)
- xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK,
- irec.br_startoff);
- if (!xfs_verify_fsbno(mp, irec.br_startblock) ||
- !xfs_verify_fsbno(mp, irec.br_startblock +
- irec.br_blockcount - 1))
- xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK,
- irec.br_startoff);
-
/*
- * Unwritten extents or blocks mapped above the highest
+ * delalloc extents or blocks mapped above the highest
* quota id shouldn't happen.
*/
if (isnullstartblock(irec.br_startblock) ||
irec.br_startoff > max_dqid_off ||
- irec.br_startoff + irec.br_blockcount > max_dqid_off + 1)
- xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, off);
+ irec.br_startoff + irec.br_blockcount - 1 > max_dqid_off) {
+ xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK,
+ irec.br_startoff);
+ break;
+ }
}
+
+ return error;
+}
+
+/* Scrub all of a quota type's items. */
+int
+xfs_scrub_quota(
+ struct xfs_scrub_context *sc)
+{
+ struct xfs_scrub_quota_info sqi;
+ struct xfs_mount *mp = sc->mp;
+ struct xfs_quotainfo *qi = mp->m_quotainfo;
+ uint dqtype;
+ int error = 0;
+
+ dqtype = xfs_scrub_quota_to_dqtype(sc);
+
+ /* Look for problem extents. */
+ error = xfs_scrub_quota_data_fork(sc);
+ if (error)
+ goto out;
if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
goto out;
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/8] xfs: quota scrub should use bmapbtd scrubber
2018-05-10 19:18 ` [PATCH 3/8] xfs: quota scrub should use bmapbtd scrubber Darrick J. Wong
@ 2018-05-11 15:19 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:19PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Replace the quota scrubber's open-coded data fork scrubber with a
> redirected call to the bmapbtd scrubber. This strengthens the quota
> scrub to include all the cross-referencing that it does.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/scrub/common.c | 57 ++++++++++++++++++++++++++++++++++
> fs/xfs/scrub/common.h | 2 +
> fs/xfs/scrub/quota.c | 83 ++++++++++++++++++++++++-------------------------
> 3 files changed, 99 insertions(+), 43 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index 2ac8576c4670..62b33c99efe4 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -44,6 +44,8 @@
> #include "xfs_rmap_btree.h"
> #include "xfs_log.h"
> #include "xfs_trans_priv.h"
> +#include "xfs_attr.h"
> +#include "xfs_reflink.h"
> #include "scrub/xfs_scrub.h"
> #include "scrub/scrub.h"
> #include "scrub/common.h"
> @@ -787,3 +789,58 @@ xfs_scrub_buffer_recheck(
> sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
> trace_xfs_scrub_block_error(sc, bp->b_bn, fa);
> }
> +
> +/*
> + * Scrub the attr/data forks of a metadata inode. The metadata inode must be
> + * pointed to by sc->ip and the ILOCK must be held.
> + */
> +int
> +xfs_scrub_metadata_inode_forks(
> + struct xfs_scrub_context *sc)
> +{
> + __u32 smtype;
> + bool shared;
> + int error;
> +
> + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + return 0;
> +
> + /* Metadata inodes don't live on the rt device. */
> + if (sc->ip->i_d.di_flags & XFS_DIFLAG_REALTIME) {
> + xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
> + return 0;
> + }
> +
> + /* They should never participate in reflink. */
> + if (xfs_is_reflink_inode(sc->ip)) {
> + xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
> + return 0;
> + }
> +
> + /* They also should never have extended attributes. */
> + if (xfs_inode_hasattr(sc->ip)) {
> + xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
> + return 0;
> + }
> +
> + /* Invoke the data fork scrubber. */
> + smtype = sc->sm->sm_type;
> + sc->sm->sm_type = XFS_SCRUB_TYPE_BMBTD;
> + error = xfs_scrub_bmap_data(sc);
> + sc->sm->sm_type = smtype;
> + if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> + return error;
> +
> + /* Look for incorrect shared blocks. */
> + if (xfs_sb_version_hasreflink(&sc->mp->m_sb)) {
> + error = xfs_reflink_inode_has_shared_extents(sc->tp, sc->ip,
> + &shared);
> + if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, 0,
> + &error))
> + return error;
> + if (shared)
> + xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
> + }
> +
> + return error;
> +}
> diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
> index a23ad7fa2b6c..5d78bb9602ab 100644
> --- a/fs/xfs/scrub/common.h
> +++ b/fs/xfs/scrub/common.h
> @@ -155,4 +155,6 @@ static inline bool xfs_scrub_skip_xref(struct xfs_scrub_metadata *sm)
> XFS_SCRUB_OFLAG_XCORRUPT);
> }
>
> +int xfs_scrub_metadata_inode_forks(struct xfs_scrub_context *sc);
> +
> #endif /* __XFS_SCRUB_COMMON_H__ */
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index d3d08978f53a..15ae4d23d6ac 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -206,65 +206,62 @@ xfs_scrub_quota_item(
> return 0;
> }
>
> -/* Scrub all of a quota type's items. */
> -int
> -xfs_scrub_quota(
> +/* Check the quota's data fork. */
> +STATIC int
> +xfs_scrub_quota_data_fork(
> struct xfs_scrub_context *sc)
> {
> struct xfs_bmbt_irec irec = { 0 };
> - struct xfs_scrub_quota_info sqi;
> - struct xfs_mount *mp = sc->mp;
> - struct xfs_quotainfo *qi = mp->m_quotainfo;
> + struct xfs_iext_cursor icur;
> + struct xfs_quotainfo *qi = sc->mp->m_quotainfo;
> + struct xfs_ifork *ifp;
> xfs_fileoff_t max_dqid_off;
> - xfs_fileoff_t off = 0;
> - uint dqtype;
> - int nimaps;
> int error = 0;
>
> - dqtype = xfs_scrub_quota_to_dqtype(sc);
> + /* Invoke the fork scrubber. */
> + error = xfs_scrub_metadata_inode_forks(sc);
> + if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> + return error;
>
> - /* Look for problem extents. */
> - if (sc->ip->i_d.di_flags & XFS_DIFLAG_REALTIME) {
> - xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
> - goto out;
> - }
> + /* Check for data fork problems that apply only to quota files. */
> max_dqid_off = ((xfs_dqid_t)-1) / qi->qi_dqperchunk;
> - while (1) {
> + ifp = XFS_IFORK_PTR(sc->ip, XFS_DATA_FORK);
> + for_each_xfs_iext(ifp, &icur, &irec) {
> if (xfs_scrub_should_terminate(sc, &error))
> break;
> -
> - off = irec.br_startoff + irec.br_blockcount;
> - nimaps = 1;
> - error = xfs_bmapi_read(sc->ip, off, -1, &irec, &nimaps,
> - XFS_BMAPI_ENTIRE);
> - if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, off,
> - &error))
> - goto out;
> - if (!nimaps)
> - break;
> - if (irec.br_startblock == HOLESTARTBLOCK)
> - continue;
> -
> - /* Check the extent record doesn't point to crap. */
> - if (irec.br_startblock + irec.br_blockcount <=
> - irec.br_startblock)
> - xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK,
> - irec.br_startoff);
> - if (!xfs_verify_fsbno(mp, irec.br_startblock) ||
> - !xfs_verify_fsbno(mp, irec.br_startblock +
> - irec.br_blockcount - 1))
> - xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK,
> - irec.br_startoff);
> -
> /*
> - * Unwritten extents or blocks mapped above the highest
> + * delalloc extents or blocks mapped above the highest
> * quota id shouldn't happen.
> */
> if (isnullstartblock(irec.br_startblock) ||
> irec.br_startoff > max_dqid_off ||
> - irec.br_startoff + irec.br_blockcount > max_dqid_off + 1)
> - xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, off);
> + irec.br_startoff + irec.br_blockcount - 1 > max_dqid_off) {
> + xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK,
> + irec.br_startoff);
> + break;
> + }
> }
> +
> + return error;
> +}
> +
> +/* Scrub all of a quota type's items. */
> +int
> +xfs_scrub_quota(
> + struct xfs_scrub_context *sc)
> +{
> + struct xfs_scrub_quota_info sqi;
> + struct xfs_mount *mp = sc->mp;
> + struct xfs_quotainfo *qi = mp->m_quotainfo;
> + uint dqtype;
> + int error = 0;
> +
> + dqtype = xfs_scrub_quota_to_dqtype(sc);
> +
> + /* Look for problem extents. */
> + error = xfs_scrub_quota_data_fork(sc);
> + if (error)
> + goto out;
> if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> goto out;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/8] xfs: scrub the data fork of the realtime inodes
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
` (2 preceding siblings ...)
2018-05-10 19:18 ` [PATCH 3/8] xfs: quota scrub should use bmapbtd scrubber Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:19 ` Brian Foster
2018-05-10 19:18 ` [PATCH 5/8] xfs: avoid ABBA deadlock when scrubbing parent pointers Darrick J. Wong
` (3 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
The realtime bitmap and summary inodes live on the metadata device, so
we can scrub their data forks with the regular scrubbers.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/scrub/rtbitmap.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
index 8b048f107af2..abc885dab122 100644
--- a/fs/xfs/scrub/rtbitmap.c
+++ b/fs/xfs/scrub/rtbitmap.c
@@ -82,6 +82,11 @@ xfs_scrub_rtbitmap(
{
int error;
+ /* Invoke the fork scrubber. */
+ error = xfs_scrub_metadata_inode_forks(sc);
+ if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
+ return error;
+
error = xfs_rtalloc_query_all(sc->tp, xfs_scrub_rtbitmap_rec, sc);
if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, 0, &error))
goto out;
@@ -95,8 +100,35 @@ int
xfs_scrub_rtsummary(
struct xfs_scrub_context *sc)
{
+ struct xfs_inode *rsumip = sc->mp->m_rsumip;
+ uint old_ilock_flags;
+ int error = 0;
+
+ /*
+ * We ILOCK'd the rt bitmap ip in the setup routine, now lock the
+ * rt summary ip in compliance with the rt inode locking rules.
+ *
+ * Since we switch sc->ip to rsumip we have to save the old ilock
+ * flags so that we don't mix up the inode state that @sc tracks.
+ */
+ old_ilock_flags = sc->ilock_flags;
+ sc->ip = rsumip;
+ sc->ilock_flags = XFS_ILOCK_EXCL | XFS_ILOCK_RTSUM;
+ xfs_ilock(sc->ip, sc->ilock_flags);
+
+ /* Invoke the fork scrubber. */
+ error = xfs_scrub_metadata_inode_forks(sc);
+ if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
+ goto out;
+
/* XXX: implement this some day */
- return -ENOENT;
+ xfs_scrub_set_incomplete(sc);
+out:
+ /* Switch back to the rtbitmap inode and lock flags. */
+ xfs_iunlock(sc->ip, sc->ilock_flags);
+ sc->ilock_flags = old_ilock_flags;
+ sc->ip = sc->mp->m_rbmip;
+ return error;
}
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 4/8] xfs: scrub the data fork of the realtime inodes
2018-05-10 19:18 ` [PATCH 4/8] xfs: scrub the data fork of the realtime inodes Darrick J. Wong
@ 2018-05-11 15:19 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:25PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> The realtime bitmap and summary inodes live on the metadata device, so
> we can scrub their data forks with the regular scrubbers.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/scrub/rtbitmap.c | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
>
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index 8b048f107af2..abc885dab122 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -82,6 +82,11 @@ xfs_scrub_rtbitmap(
> {
> int error;
>
> + /* Invoke the fork scrubber. */
> + error = xfs_scrub_metadata_inode_forks(sc);
> + if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> + return error;
> +
> error = xfs_rtalloc_query_all(sc->tp, xfs_scrub_rtbitmap_rec, sc);
> if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, 0, &error))
> goto out;
> @@ -95,8 +100,35 @@ int
> xfs_scrub_rtsummary(
> struct xfs_scrub_context *sc)
> {
> + struct xfs_inode *rsumip = sc->mp->m_rsumip;
> + uint old_ilock_flags;
> + int error = 0;
> +
> + /*
> + * We ILOCK'd the rt bitmap ip in the setup routine, now lock the
> + * rt summary ip in compliance with the rt inode locking rules.
> + *
> + * Since we switch sc->ip to rsumip we have to save the old ilock
> + * flags so that we don't mix up the inode state that @sc tracks.
> + */
> + old_ilock_flags = sc->ilock_flags;
> + sc->ip = rsumip;
> + sc->ilock_flags = XFS_ILOCK_EXCL | XFS_ILOCK_RTSUM;
> + xfs_ilock(sc->ip, sc->ilock_flags);
> +
> + /* Invoke the fork scrubber. */
> + error = xfs_scrub_metadata_inode_forks(sc);
> + if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> + goto out;
> +
> /* XXX: implement this some day */
> - return -ENOENT;
> + xfs_scrub_set_incomplete(sc);
> +out:
> + /* Switch back to the rtbitmap inode and lock flags. */
> + xfs_iunlock(sc->ip, sc->ilock_flags);
> + sc->ilock_flags = old_ilock_flags;
> + sc->ip = sc->mp->m_rbmip;
I'd use an old_ip here as well just to avoid hardcoding against some
other function. With that, looks fine:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> + return error;
> }
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/8] xfs: avoid ABBA deadlock when scrubbing parent pointers
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
` (3 preceding siblings ...)
2018-05-10 19:18 ` [PATCH 4/8] xfs: scrub the data fork of the realtime inodes Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:20 ` Brian Foster
2018-05-10 19:18 ` [PATCH 6/8] xfs: hoist xfs_scrub_agfl_walk to libxfs as xfs_agfl_walk Darrick J. Wong
` (2 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
In normal operation, the XFS convention is to take an inode's iolock
and then allocate a transaction. However, when scrubbing parent inodes
this is inverted -- we allocated the transaction to do the scrub, and
now we're trying to grab the parent's iolock. This can lead to ABBA
deadlocks: some thread grabbed the parent's iolock and is waiting for
space for a transaction while our parent scrubber is sitting on a
transaction trying to get the parent's iolock.
Therefore, convert all iolock attempts to use trylock; if that fails,
they can use the existing mechanisms to back off and try again.
The ABBA deadlock didn't happen with a non-repair scrub because the
transactions don't reserve any space, but repair scrubs require
reservation in order to update metadata. However, any other concurrent
metadata update (e.g. directory create in the parent) could also induce
this deadlock with the parent scrubber.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/scrub/common.c | 22 ++++++++++++++++++++++
fs/xfs/scrub/common.h | 1 +
fs/xfs/scrub/parent.c | 16 ++++++++++++++--
3 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 62b33c99efe4..518bff2be0c9 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -844,3 +844,25 @@ xfs_scrub_metadata_inode_forks(
return error;
}
+
+/*
+ * Try to lock an inode in violation of the usual locking order rules. For
+ * example, trying to get the IOLOCK while in transaction context, or just
+ * plain breaking AG-order or inode-order inode locking rules. Either way,
+ * the only way to avoid an ABBA deadlock is to use trylock and back off if
+ * we can't.
+ */
+int
+xfs_scrub_ilock_inverted(
+ struct xfs_inode *ip,
+ uint lock_mode)
+{
+ int i;
+
+ for (i = 0; i < 20; i++) {
+ if (xfs_ilock_nowait(ip, lock_mode))
+ return 0;
+ delay(1);
+ }
+ return -EDEADLOCK;
+}
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 5d78bb9602ab..119d9b6db887 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -156,5 +156,6 @@ static inline bool xfs_scrub_skip_xref(struct xfs_scrub_metadata *sm)
}
int xfs_scrub_metadata_inode_forks(struct xfs_scrub_context *sc);
+int xfs_scrub_ilock_inverted(struct xfs_inode *ip, uint lock_mode);
#endif /* __XFS_SCRUB_COMMON_H__ */
diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
index fc336807e156..77c6b22c6bfd 100644
--- a/fs/xfs/scrub/parent.c
+++ b/fs/xfs/scrub/parent.c
@@ -214,7 +214,9 @@ xfs_scrub_parent_validate(
*/
xfs_iunlock(sc->ip, sc->ilock_flags);
sc->ilock_flags = 0;
- xfs_ilock(dp, XFS_IOLOCK_SHARED);
+ error = xfs_scrub_ilock_inverted(dp, XFS_IOLOCK_SHARED);
+ if (error)
+ goto out_rele;
/* Go looking for our dentry. */
error = xfs_scrub_parent_count_parent_dentries(sc, dp, &nlink);
@@ -223,8 +225,10 @@ xfs_scrub_parent_validate(
/* Drop the parent lock, relock this inode. */
xfs_iunlock(dp, XFS_IOLOCK_SHARED);
+ error = xfs_scrub_ilock_inverted(sc->ip, XFS_IOLOCK_EXCL);
+ if (error)
+ goto out_rele;
sc->ilock_flags = XFS_IOLOCK_EXCL;
- xfs_ilock(sc->ip, sc->ilock_flags);
/*
* If we're an unlinked directory, the parent /won't/ have a link
@@ -326,5 +330,13 @@ xfs_scrub_parent(
if (try_again && tries == 20)
xfs_scrub_set_incomplete(sc);
out:
+ /*
+ * If we failed to lock the parent inode even after a retry, just mark
+ * this scrub incomplete and return.
+ */
+ if (sc->try_harder && error == -EDEADLOCK) {
+ error = 0;
+ xfs_scrub_set_incomplete(sc);
+ }
return error;
}
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 5/8] xfs: avoid ABBA deadlock when scrubbing parent pointers
2018-05-10 19:18 ` [PATCH 5/8] xfs: avoid ABBA deadlock when scrubbing parent pointers Darrick J. Wong
@ 2018-05-11 15:20 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:20 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:32PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> In normal operation, the XFS convention is to take an inode's iolock
> and then allocate a transaction. However, when scrubbing parent inodes
> this is inverted -- we allocated the transaction to do the scrub, and
> now we're trying to grab the parent's iolock. This can lead to ABBA
> deadlocks: some thread grabbed the parent's iolock and is waiting for
> space for a transaction while our parent scrubber is sitting on a
> transaction trying to get the parent's iolock.
>
> Therefore, convert all iolock attempts to use trylock; if that fails,
> they can use the existing mechanisms to back off and try again.
>
> The ABBA deadlock didn't happen with a non-repair scrub because the
> transactions don't reserve any space, but repair scrubs require
> reservation in order to update metadata. However, any other concurrent
> metadata update (e.g. directory create in the parent) could also induce
> this deadlock with the parent scrubber.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/scrub/common.c | 22 ++++++++++++++++++++++
> fs/xfs/scrub/common.h | 1 +
> fs/xfs/scrub/parent.c | 16 ++++++++++++++--
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index 62b33c99efe4..518bff2be0c9 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -844,3 +844,25 @@ xfs_scrub_metadata_inode_forks(
>
> return error;
> }
> +
> +/*
> + * Try to lock an inode in violation of the usual locking order rules. For
> + * example, trying to get the IOLOCK while in transaction context, or just
> + * plain breaking AG-order or inode-order inode locking rules. Either way,
> + * the only way to avoid an ABBA deadlock is to use trylock and back off if
> + * we can't.
> + */
> +int
> +xfs_scrub_ilock_inverted(
> + struct xfs_inode *ip,
> + uint lock_mode)
> +{
> + int i;
> +
> + for (i = 0; i < 20; i++) {
> + if (xfs_ilock_nowait(ip, lock_mode))
> + return 0;
> + delay(1);
> + }
> + return -EDEADLOCK;
> +}
This is definitely hacky. It would be nice if we could come up with
something cleaner to explicitly unwind or something. While this may
address the issue described in the commit log, I wonder whether these
kind of loops introduce the possibility of really long runtimes due to
racing with parent changes and whatnot (though I guess the "try_again"
thing is also capped to 20 retries, but when you factor them all
together per-inode...).
Anyways, experimental code and a better solution probably requires more
thought:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
> index 5d78bb9602ab..119d9b6db887 100644
> --- a/fs/xfs/scrub/common.h
> +++ b/fs/xfs/scrub/common.h
> @@ -156,5 +156,6 @@ static inline bool xfs_scrub_skip_xref(struct xfs_scrub_metadata *sm)
> }
>
> int xfs_scrub_metadata_inode_forks(struct xfs_scrub_context *sc);
> +int xfs_scrub_ilock_inverted(struct xfs_inode *ip, uint lock_mode);
>
> #endif /* __XFS_SCRUB_COMMON_H__ */
> diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
> index fc336807e156..77c6b22c6bfd 100644
> --- a/fs/xfs/scrub/parent.c
> +++ b/fs/xfs/scrub/parent.c
> @@ -214,7 +214,9 @@ xfs_scrub_parent_validate(
> */
> xfs_iunlock(sc->ip, sc->ilock_flags);
> sc->ilock_flags = 0;
> - xfs_ilock(dp, XFS_IOLOCK_SHARED);
> + error = xfs_scrub_ilock_inverted(dp, XFS_IOLOCK_SHARED);
> + if (error)
> + goto out_rele;
>
> /* Go looking for our dentry. */
> error = xfs_scrub_parent_count_parent_dentries(sc, dp, &nlink);
> @@ -223,8 +225,10 @@ xfs_scrub_parent_validate(
>
> /* Drop the parent lock, relock this inode. */
> xfs_iunlock(dp, XFS_IOLOCK_SHARED);
> + error = xfs_scrub_ilock_inverted(sc->ip, XFS_IOLOCK_EXCL);
> + if (error)
> + goto out_rele;
> sc->ilock_flags = XFS_IOLOCK_EXCL;
> - xfs_ilock(sc->ip, sc->ilock_flags);
>
> /*
> * If we're an unlinked directory, the parent /won't/ have a link
> @@ -326,5 +330,13 @@ xfs_scrub_parent(
> if (try_again && tries == 20)
> xfs_scrub_set_incomplete(sc);
> out:
> + /*
> + * If we failed to lock the parent inode even after a retry, just mark
> + * this scrub incomplete and return.
> + */
> + if (sc->try_harder && error == -EDEADLOCK) {
> + error = 0;
> + xfs_scrub_set_incomplete(sc);
> + }
> return error;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 6/8] xfs: hoist xfs_scrub_agfl_walk to libxfs as xfs_agfl_walk
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
` (4 preceding siblings ...)
2018-05-10 19:18 ` [PATCH 5/8] xfs: avoid ABBA deadlock when scrubbing parent pointers Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:20 ` Brian Foster
2018-05-10 19:18 ` [PATCH 7/8] xfs: make xfs_bmapi_remapi work with attribute forks Darrick J. Wong
2018-05-10 19:18 ` [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags Darrick J. Wong
7 siblings, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
This function is basically a generic AGFL block iterator, so promote it
to libxfs ahead of online repair wanting to use it.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_alloc.c | 37 +++++++++++++++++++++
fs/xfs/libxfs/xfs_alloc.h | 5 +++
| 78 ++++++++-------------------------------------
fs/xfs/scrub/common.h | 4 --
4 files changed, 55 insertions(+), 69 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 5410635893df..dc9dd3805d97 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -3180,3 +3180,40 @@ xfs_alloc_has_record(
return xfs_btree_has_record(cur, &low, &high, exists);
}
+
+/*
+ * Walk all the blocks in the AGFL. The @walk_fn can return any negative
+ * error code or XFS_BTREE_QUERY_RANGE_ABORT.
+ */
+int
+xfs_agfl_walk(
+ struct xfs_mount *mp,
+ struct xfs_agf *agf,
+ struct xfs_buf *agflbp,
+ xfs_agfl_walk_fn walk_fn,
+ void *priv)
+{
+ __be32 *agfl_bno;
+ unsigned int i;
+ int error;
+
+ agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+ i = be32_to_cpu(agf->agf_flfirst);
+
+ /* Nothing to walk in an empty AGFL. */
+ if (agf->agf_flcount == cpu_to_be32(0))
+ return 0;
+
+ /* Otherwise, walk from first to last, wrapping as needed. */
+ for (;;) {
+ error = walk_fn(mp, be32_to_cpu(agfl_bno[i]), priv);
+ if (error)
+ return error;
+ if (i == be32_to_cpu(agf->agf_fllast))
+ break;
+ if (++i == xfs_agfl_size(mp))
+ i = 0;
+ }
+
+ return 0;
+}
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 46d48c6f83b7..0747adcd57d6 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -262,4 +262,9 @@ bool xfs_verify_fsbno(struct xfs_mount *mp, xfs_fsblock_t fsbno);
int xfs_alloc_has_record(struct xfs_btree_cur *cur, xfs_agblock_t bno,
xfs_extlen_t len, bool *exist);
+typedef int (*xfs_agfl_walk_fn)(struct xfs_mount *mp, xfs_agblock_t bno,
+ void *priv);
+int xfs_agfl_walk(struct xfs_mount *mp, struct xfs_agf *agf,
+ struct xfs_buf *agflbp, xfs_agfl_walk_fn walk_fn, void *priv);
+
#endif /* __XFS_ALLOC_H__ */
--git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 831acc0a328f..1f71793f7db4 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -38,68 +38,6 @@
#include "scrub/common.h"
#include "scrub/trace.h"
-/*
- * Walk all the blocks in the AGFL. The fn function can return any negative
- * error code or XFS_BTREE_QUERY_RANGE_ABORT.
- */
-int
-xfs_scrub_walk_agfl(
- struct xfs_scrub_context *sc,
- int (*fn)(struct xfs_scrub_context *,
- xfs_agblock_t bno, void *),
- void *priv)
-{
- struct xfs_agf *agf;
- __be32 *agfl_bno;
- struct xfs_mount *mp = sc->mp;
- unsigned int flfirst;
- unsigned int fllast;
- int i;
- int error;
-
- agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
- agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, sc->sa.agfl_bp);
- flfirst = be32_to_cpu(agf->agf_flfirst);
- fllast = be32_to_cpu(agf->agf_fllast);
-
- /* Nothing to walk in an empty AGFL. */
- if (agf->agf_flcount == cpu_to_be32(0))
- return 0;
-
- /* first to last is a consecutive list. */
- if (fllast >= flfirst) {
- for (i = flfirst; i <= fllast; i++) {
- error = fn(sc, be32_to_cpu(agfl_bno[i]), priv);
- if (error)
- return error;
- if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
- return error;
- }
-
- return 0;
- }
-
- /* first to the end */
- for (i = flfirst; i < xfs_agfl_size(mp); i++) {
- error = fn(sc, be32_to_cpu(agfl_bno[i]), priv);
- if (error)
- return error;
- if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
- return error;
- }
-
- /* the start to last. */
- for (i = 0; i <= fllast; i++) {
- error = fn(sc, be32_to_cpu(agfl_bno[i]), priv);
- if (error)
- return error;
- if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
- return error;
- }
-
- return 0;
-}
-
/* Superblock */
/* Cross-reference with the other btrees. */
@@ -678,6 +616,7 @@ struct xfs_scrub_agfl_info {
unsigned int sz_entries;
unsigned int nr_entries;
xfs_agblock_t *entries;
+ struct xfs_scrub_context *sc;
};
/* Cross-reference with the other btrees. */
@@ -699,12 +638,12 @@ xfs_scrub_agfl_block_xref(
/* Scrub an AGFL block. */
STATIC int
xfs_scrub_agfl_block(
- struct xfs_scrub_context *sc,
+ struct xfs_mount *mp,
xfs_agblock_t agbno,
void *priv)
{
- struct xfs_mount *mp = sc->mp;
struct xfs_scrub_agfl_info *sai = priv;
+ struct xfs_scrub_context *sc = sai->sc;
xfs_agnumber_t agno = sc->sa.agno;
if (xfs_verify_agbno(mp, agno, agbno) &&
@@ -715,6 +654,9 @@ xfs_scrub_agfl_block(
xfs_scrub_agfl_block_xref(sc, agbno, priv);
+ if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+ return XFS_BTREE_QUERY_RANGE_ABORT;
+
return 0;
}
@@ -794,6 +736,7 @@ xfs_scrub_agfl(
goto out;
}
memset(&sai, 0, sizeof(sai));
+ sai.sc = sc;
sai.sz_entries = agflcount;
sai.entries = kmem_zalloc(sizeof(xfs_agblock_t) * agflcount,
KM_MAYFAIL);
@@ -804,7 +747,12 @@ xfs_scrub_agfl(
/* Check the blocks in the AGFL. */
xfs_rmap_ag_owner(&sai.oinfo, XFS_RMAP_OWN_AG);
- error = xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, &sai);
+ error = xfs_agfl_walk(sc->mp, XFS_BUF_TO_AGF(sc->sa.agf_bp),
+ sc->sa.agfl_bp, xfs_scrub_agfl_block, &sai);
+ if (error == XFS_BTREE_QUERY_RANGE_ABORT) {
+ error = 0;
+ goto out_free;
+ }
if (error)
goto out_free;
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 119d9b6db887..a660087b606e 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -129,10 +129,6 @@ int xfs_scrub_ag_read_headers(struct xfs_scrub_context *sc, xfs_agnumber_t agno,
void xfs_scrub_ag_btcur_free(struct xfs_scrub_ag *sa);
int xfs_scrub_ag_btcur_init(struct xfs_scrub_context *sc,
struct xfs_scrub_ag *sa);
-int xfs_scrub_walk_agfl(struct xfs_scrub_context *sc,
- int (*fn)(struct xfs_scrub_context *, xfs_agblock_t bno,
- void *),
- void *priv);
int xfs_scrub_count_rmap_ownedby_ag(struct xfs_scrub_context *sc,
struct xfs_btree_cur *cur,
struct xfs_owner_info *oinfo,
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 6/8] xfs: hoist xfs_scrub_agfl_walk to libxfs as xfs_agfl_walk
2018-05-10 19:18 ` [PATCH 6/8] xfs: hoist xfs_scrub_agfl_walk to libxfs as xfs_agfl_walk Darrick J. Wong
@ 2018-05-11 15:20 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:20 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:38PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> This function is basically a generic AGFL block iterator, so promote it
> to libxfs ahead of online repair wanting to use it.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_alloc.c | 37 +++++++++++++++++++++
> fs/xfs/libxfs/xfs_alloc.h | 5 +++
> fs/xfs/scrub/agheader.c | 78 ++++++++-------------------------------------
> fs/xfs/scrub/common.h | 4 --
> 4 files changed, 55 insertions(+), 69 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 5410635893df..dc9dd3805d97 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -3180,3 +3180,40 @@ xfs_alloc_has_record(
>
> return xfs_btree_has_record(cur, &low, &high, exists);
> }
> +
> +/*
> + * Walk all the blocks in the AGFL. The @walk_fn can return any negative
> + * error code or XFS_BTREE_QUERY_RANGE_ABORT.
> + */
> +int
> +xfs_agfl_walk(
> + struct xfs_mount *mp,
> + struct xfs_agf *agf,
> + struct xfs_buf *agflbp,
> + xfs_agfl_walk_fn walk_fn,
> + void *priv)
> +{
> + __be32 *agfl_bno;
> + unsigned int i;
> + int error;
> +
> + agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
> + i = be32_to_cpu(agf->agf_flfirst);
> +
> + /* Nothing to walk in an empty AGFL. */
> + if (agf->agf_flcount == cpu_to_be32(0))
> + return 0;
> +
> + /* Otherwise, walk from first to last, wrapping as needed. */
> + for (;;) {
> + error = walk_fn(mp, be32_to_cpu(agfl_bno[i]), priv);
> + if (error)
> + return error;
> + if (i == be32_to_cpu(agf->agf_fllast))
> + break;
> + if (++i == xfs_agfl_size(mp))
> + i = 0;
> + }
> +
> + return 0;
> +}
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index 46d48c6f83b7..0747adcd57d6 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -262,4 +262,9 @@ bool xfs_verify_fsbno(struct xfs_mount *mp, xfs_fsblock_t fsbno);
> int xfs_alloc_has_record(struct xfs_btree_cur *cur, xfs_agblock_t bno,
> xfs_extlen_t len, bool *exist);
>
> +typedef int (*xfs_agfl_walk_fn)(struct xfs_mount *mp, xfs_agblock_t bno,
> + void *priv);
> +int xfs_agfl_walk(struct xfs_mount *mp, struct xfs_agf *agf,
> + struct xfs_buf *agflbp, xfs_agfl_walk_fn walk_fn, void *priv);
> +
> #endif /* __XFS_ALLOC_H__ */
> diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> index 831acc0a328f..1f71793f7db4 100644
> --- a/fs/xfs/scrub/agheader.c
> +++ b/fs/xfs/scrub/agheader.c
> @@ -38,68 +38,6 @@
> #include "scrub/common.h"
> #include "scrub/trace.h"
>
> -/*
> - * Walk all the blocks in the AGFL. The fn function can return any negative
> - * error code or XFS_BTREE_QUERY_RANGE_ABORT.
> - */
> -int
> -xfs_scrub_walk_agfl(
> - struct xfs_scrub_context *sc,
> - int (*fn)(struct xfs_scrub_context *,
> - xfs_agblock_t bno, void *),
> - void *priv)
> -{
> - struct xfs_agf *agf;
> - __be32 *agfl_bno;
> - struct xfs_mount *mp = sc->mp;
> - unsigned int flfirst;
> - unsigned int fllast;
> - int i;
> - int error;
> -
> - agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
> - agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, sc->sa.agfl_bp);
> - flfirst = be32_to_cpu(agf->agf_flfirst);
> - fllast = be32_to_cpu(agf->agf_fllast);
> -
> - /* Nothing to walk in an empty AGFL. */
> - if (agf->agf_flcount == cpu_to_be32(0))
> - return 0;
> -
> - /* first to last is a consecutive list. */
> - if (fllast >= flfirst) {
> - for (i = flfirst; i <= fllast; i++) {
> - error = fn(sc, be32_to_cpu(agfl_bno[i]), priv);
> - if (error)
> - return error;
> - if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> - return error;
> - }
> -
> - return 0;
> - }
> -
> - /* first to the end */
> - for (i = flfirst; i < xfs_agfl_size(mp); i++) {
> - error = fn(sc, be32_to_cpu(agfl_bno[i]), priv);
> - if (error)
> - return error;
> - if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> - return error;
> - }
> -
> - /* the start to last. */
> - for (i = 0; i <= fllast; i++) {
> - error = fn(sc, be32_to_cpu(agfl_bno[i]), priv);
> - if (error)
> - return error;
> - if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> - return error;
> - }
> -
> - return 0;
> -}
> -
> /* Superblock */
>
> /* Cross-reference with the other btrees. */
> @@ -678,6 +616,7 @@ struct xfs_scrub_agfl_info {
> unsigned int sz_entries;
> unsigned int nr_entries;
> xfs_agblock_t *entries;
> + struct xfs_scrub_context *sc;
> };
>
> /* Cross-reference with the other btrees. */
> @@ -699,12 +638,12 @@ xfs_scrub_agfl_block_xref(
> /* Scrub an AGFL block. */
> STATIC int
> xfs_scrub_agfl_block(
> - struct xfs_scrub_context *sc,
> + struct xfs_mount *mp,
> xfs_agblock_t agbno,
> void *priv)
> {
> - struct xfs_mount *mp = sc->mp;
> struct xfs_scrub_agfl_info *sai = priv;
> + struct xfs_scrub_context *sc = sai->sc;
> xfs_agnumber_t agno = sc->sa.agno;
>
> if (xfs_verify_agbno(mp, agno, agbno) &&
> @@ -715,6 +654,9 @@ xfs_scrub_agfl_block(
>
> xfs_scrub_agfl_block_xref(sc, agbno, priv);
>
> + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> + return XFS_BTREE_QUERY_RANGE_ABORT;
> +
> return 0;
> }
>
> @@ -794,6 +736,7 @@ xfs_scrub_agfl(
> goto out;
> }
> memset(&sai, 0, sizeof(sai));
> + sai.sc = sc;
> sai.sz_entries = agflcount;
> sai.entries = kmem_zalloc(sizeof(xfs_agblock_t) * agflcount,
> KM_MAYFAIL);
> @@ -804,7 +747,12 @@ xfs_scrub_agfl(
>
> /* Check the blocks in the AGFL. */
> xfs_rmap_ag_owner(&sai.oinfo, XFS_RMAP_OWN_AG);
> - error = xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, &sai);
> + error = xfs_agfl_walk(sc->mp, XFS_BUF_TO_AGF(sc->sa.agf_bp),
> + sc->sa.agfl_bp, xfs_scrub_agfl_block, &sai);
> + if (error == XFS_BTREE_QUERY_RANGE_ABORT) {
> + error = 0;
> + goto out_free;
> + }
> if (error)
> goto out_free;
>
> diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
> index 119d9b6db887..a660087b606e 100644
> --- a/fs/xfs/scrub/common.h
> +++ b/fs/xfs/scrub/common.h
> @@ -129,10 +129,6 @@ int xfs_scrub_ag_read_headers(struct xfs_scrub_context *sc, xfs_agnumber_t agno,
> void xfs_scrub_ag_btcur_free(struct xfs_scrub_ag *sa);
> int xfs_scrub_ag_btcur_init(struct xfs_scrub_context *sc,
> struct xfs_scrub_ag *sa);
> -int xfs_scrub_walk_agfl(struct xfs_scrub_context *sc,
> - int (*fn)(struct xfs_scrub_context *, xfs_agblock_t bno,
> - void *),
> - void *priv);
> int xfs_scrub_count_rmap_ownedby_ag(struct xfs_scrub_context *sc,
> struct xfs_btree_cur *cur,
> struct xfs_owner_info *oinfo,
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 7/8] xfs: make xfs_bmapi_remapi work with attribute forks
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
` (5 preceding siblings ...)
2018-05-10 19:18 ` [PATCH 6/8] xfs: hoist xfs_scrub_agfl_walk to libxfs as xfs_agfl_walk Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:20 ` Brian Foster
2018-05-10 19:18 ` [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags Darrick J. Wong
7 siblings, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Add a new flags argument to xfs_bmapi_remapi so that we can pass BMAPI
flags into the function. This enables us to pass in BMAPI_ATTRFORK so
that we can remap things into the attribute fork. Eventually the
online repair code will use this to rebuild attribute forks, so make it
non-static.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 28 ++++++++++++++++------------
fs/xfs/libxfs/xfs_bmap.h | 4 ++++
2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 0fd051064ff0..b63e15a114f3 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4520,30 +4520,34 @@ xfs_bmapi_write(
return error;
}
-static int
+int
xfs_bmapi_remap(
struct xfs_trans *tp,
struct xfs_inode *ip,
xfs_fileoff_t bno,
xfs_filblks_t len,
xfs_fsblock_t startblock,
- struct xfs_defer_ops *dfops)
+ struct xfs_defer_ops *dfops,
+ int flags)
{
struct xfs_mount *mp = ip->i_mount;
- struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+ struct xfs_ifork *ifp;
struct xfs_btree_cur *cur = NULL;
xfs_fsblock_t firstblock = NULLFSBLOCK;
struct xfs_bmbt_irec got;
struct xfs_iext_cursor icur;
+ int whichfork = xfs_bmapi_whichfork(flags);
int logflags = 0, error;
+ ifp = XFS_IFORK_PTR(ip, whichfork);
ASSERT(len > 0);
ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
if (unlikely(XFS_TEST_ERROR(
- (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
- XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
+ (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
+ XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
mp, XFS_ERRTAG_BMAPIFORMAT))) {
XFS_ERROR_REPORT("xfs_bmapi_remap", XFS_ERRLEVEL_LOW, mp);
return -EFSCORRUPTED;
@@ -4553,7 +4557,7 @@ xfs_bmapi_remap(
return -EIO;
if (!(ifp->if_flags & XFS_IFEXTENTS)) {
- error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
+ error = xfs_iread_extents(tp, ip, whichfork);
if (error)
return error;
}
@@ -4568,7 +4572,7 @@ xfs_bmapi_remap(
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
if (ifp->if_flags & XFS_IFBROOT) {
- cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
+ cur = xfs_bmbt_init_cursor(mp, tp, ip, whichfork);
cur->bc_private.b.firstblock = firstblock;
cur->bc_private.b.dfops = dfops;
cur->bc_private.b.flags = 0;
@@ -4579,16 +4583,16 @@ xfs_bmapi_remap(
got.br_blockcount = len;
got.br_state = XFS_EXT_NORM;
- error = xfs_bmap_add_extent_hole_real(tp, ip, XFS_DATA_FORK, &icur,
- &cur, &got, &firstblock, dfops, &logflags, 0);
+ error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
+ &cur, &got, &firstblock, dfops, &logflags, flags);
if (error)
goto error0;
- if (xfs_bmap_wants_extents(ip, XFS_DATA_FORK)) {
+ if (xfs_bmap_wants_extents(ip, whichfork)) {
int tmp_logflags = 0;
error = xfs_bmap_btree_to_extents(tp, ip, cur,
- &tmp_logflags, XFS_DATA_FORK);
+ &tmp_logflags, whichfork);
logflags |= tmp_logflags;
}
@@ -6162,7 +6166,7 @@ xfs_bmap_finish_one(
switch (type) {
case XFS_BMAP_MAP:
error = xfs_bmapi_remap(tp, ip, startoff, *blockcount,
- startblock, dfops);
+ startblock, dfops, 0);
*blockcount = 0;
break;
case XFS_BMAP_UNMAP:
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 6046012674c8..2c233f9f1a26 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -297,4 +297,8 @@ static inline int xfs_bmap_fork_to_state(int whichfork)
xfs_failaddr_t xfs_bmap_validate_extent(struct xfs_inode *ip, int whichfork,
struct xfs_bmbt_irec *irec);
+int xfs_bmapi_remap(struct xfs_trans *tp, struct xfs_inode *ip,
+ xfs_fileoff_t bno, xfs_filblks_t len, xfs_fsblock_t startblock,
+ struct xfs_defer_ops *dfops, int flags);
+
#endif /* __XFS_BMAP_H__ */
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 7/8] xfs: make xfs_bmapi_remapi work with attribute forks
2018-05-10 19:18 ` [PATCH 7/8] xfs: make xfs_bmapi_remapi work with attribute forks Darrick J. Wong
@ 2018-05-11 15:20 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:20 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Add a new flags argument to xfs_bmapi_remapi so that we can pass BMAPI
> flags into the function. This enables us to pass in BMAPI_ATTRFORK so
> that we can remap things into the attribute fork. Eventually the
> online repair code will use this to rebuild attribute forks, so make it
> non-static.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_bmap.c | 28 ++++++++++++++++------------
> fs/xfs/libxfs/xfs_bmap.h | 4 ++++
> 2 files changed, 20 insertions(+), 12 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 0fd051064ff0..b63e15a114f3 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4520,30 +4520,34 @@ xfs_bmapi_write(
> return error;
> }
>
> -static int
> +int
> xfs_bmapi_remap(
> struct xfs_trans *tp,
> struct xfs_inode *ip,
> xfs_fileoff_t bno,
> xfs_filblks_t len,
> xfs_fsblock_t startblock,
> - struct xfs_defer_ops *dfops)
> + struct xfs_defer_ops *dfops,
> + int flags)
> {
> struct xfs_mount *mp = ip->i_mount;
> - struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> + struct xfs_ifork *ifp;
> struct xfs_btree_cur *cur = NULL;
> xfs_fsblock_t firstblock = NULLFSBLOCK;
> struct xfs_bmbt_irec got;
> struct xfs_iext_cursor icur;
> + int whichfork = xfs_bmapi_whichfork(flags);
> int logflags = 0, error;
>
> + ifp = XFS_IFORK_PTR(ip, whichfork);
> ASSERT(len > 0);
> ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
>
> if (unlikely(XFS_TEST_ERROR(
> - (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> - XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> + (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> + XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> mp, XFS_ERRTAG_BMAPIFORMAT))) {
> XFS_ERROR_REPORT("xfs_bmapi_remap", XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> @@ -4553,7 +4557,7 @@ xfs_bmapi_remap(
> return -EIO;
>
> if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> - error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
> + error = xfs_iread_extents(tp, ip, whichfork);
> if (error)
> return error;
> }
> @@ -4568,7 +4572,7 @@ xfs_bmapi_remap(
> xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>
> if (ifp->if_flags & XFS_IFBROOT) {
> - cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
> + cur = xfs_bmbt_init_cursor(mp, tp, ip, whichfork);
> cur->bc_private.b.firstblock = firstblock;
> cur->bc_private.b.dfops = dfops;
> cur->bc_private.b.flags = 0;
> @@ -4579,16 +4583,16 @@ xfs_bmapi_remap(
> got.br_blockcount = len;
> got.br_state = XFS_EXT_NORM;
>
> - error = xfs_bmap_add_extent_hole_real(tp, ip, XFS_DATA_FORK, &icur,
> - &cur, &got, &firstblock, dfops, &logflags, 0);
> + error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
> + &cur, &got, &firstblock, dfops, &logflags, flags);
> if (error)
> goto error0;
>
> - if (xfs_bmap_wants_extents(ip, XFS_DATA_FORK)) {
> + if (xfs_bmap_wants_extents(ip, whichfork)) {
> int tmp_logflags = 0;
>
> error = xfs_bmap_btree_to_extents(tp, ip, cur,
> - &tmp_logflags, XFS_DATA_FORK);
> + &tmp_logflags, whichfork);
> logflags |= tmp_logflags;
> }
>
> @@ -6162,7 +6166,7 @@ xfs_bmap_finish_one(
> switch (type) {
> case XFS_BMAP_MAP:
> error = xfs_bmapi_remap(tp, ip, startoff, *blockcount,
> - startblock, dfops);
> + startblock, dfops, 0);
> *blockcount = 0;
> break;
> case XFS_BMAP_UNMAP:
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 6046012674c8..2c233f9f1a26 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -297,4 +297,8 @@ static inline int xfs_bmap_fork_to_state(int whichfork)
> xfs_failaddr_t xfs_bmap_validate_extent(struct xfs_inode *ip, int whichfork,
> struct xfs_bmbt_irec *irec);
>
> +int xfs_bmapi_remap(struct xfs_trans *tp, struct xfs_inode *ip,
> + xfs_fileoff_t bno, xfs_filblks_t len, xfs_fsblock_t startblock,
> + struct xfs_defer_ops *dfops, int flags);
> +
> #endif /* __XFS_BMAP_H__ */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags
2018-05-10 19:18 [PATCH v5 0/8] xfs-4.18: scrub fixes Darrick J. Wong
` (6 preceding siblings ...)
2018-05-10 19:18 ` [PATCH 7/8] xfs: make xfs_bmapi_remapi work with attribute forks Darrick J. Wong
@ 2018-05-10 19:18 ` Darrick J. Wong
2018-05-11 15:20 ` Brian Foster
2018-05-11 23:46 ` [PATCH v2 " Darrick J. Wong
7 siblings, 2 replies; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-10 19:18 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Teach xfs_bmapi_remap how to map in unwritten extent and to skip rmap
updates. This enables us to rebuild real and unwritten extents from the
rmapbt.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index b63e15a114f3..7660efb5beb0 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4543,7 +4543,9 @@ xfs_bmapi_remap(
ASSERT(len > 0);
ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
- ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
+ ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
+ XFS_BMAPI_NORMAP)));
+ ASSERT(!(flags & XFS_BMAPI_ATTRFORK) || !(flags & XFS_BMAPI_PREALLOC));
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
@@ -4581,7 +4583,10 @@ xfs_bmapi_remap(
got.br_startoff = bno;
got.br_startblock = startblock;
got.br_blockcount = len;
- got.br_state = XFS_EXT_NORM;
+ if (flags & XFS_BMAPI_PREALLOC)
+ got.br_state = XFS_EXT_UNWRITTEN;
+ else
+ got.br_state = XFS_EXT_NORM;
error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
&cur, &got, &firstblock, dfops, &logflags, flags);
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags
2018-05-10 19:18 ` [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags Darrick J. Wong
@ 2018-05-11 15:20 ` Brian Foster
2018-05-11 23:14 ` Darrick J. Wong
2018-05-11 23:46 ` [PATCH v2 " Darrick J. Wong
1 sibling, 1 reply; 24+ messages in thread
From: Brian Foster @ 2018-05-11 15:20 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, May 10, 2018 at 12:18:56PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Teach xfs_bmapi_remap how to map in unwritten extent and to skip rmap
> updates. This enables us to rebuild real and unwritten extents from the
> rmapbt.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index b63e15a114f3..7660efb5beb0 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4543,7 +4543,9 @@ xfs_bmapi_remap(
> ASSERT(len > 0);
> ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> - ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
> + ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
> + XFS_BMAPI_NORMAP)));
> + ASSERT(!(flags & XFS_BMAPI_ATTRFORK) || !(flags & XFS_BMAPI_PREALLOC));
A little confusing.. how about something like?
ASSERT((flags & XFS_BMAPI_ATTRFORK|XFS_BMAPI_PREALLOC) !=
XFS_BMAPI_ATTRFORK|XFS_BMAPI_PREALLOC);
>
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> @@ -4581,7 +4583,10 @@ xfs_bmapi_remap(
> got.br_startoff = bno;
> got.br_startblock = startblock;
> got.br_blockcount = len;
> - got.br_state = XFS_EXT_NORM;
> + if (flags & XFS_BMAPI_PREALLOC)
> + got.br_state = XFS_EXT_UNWRITTEN;
> + else
> + got.br_state = XFS_EXT_NORM;
It's hard to see the wider picture without any users of this, but I do
notice that the one current caller appears to have everything passed
down to it to construct an xfs_bmbt_irec (in fact the data appears to
come from an irec in the first place).
Brian
>
> error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
> &cur, &got, &firstblock, dfops, &logflags, flags);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags
2018-05-11 15:20 ` Brian Foster
@ 2018-05-11 23:14 ` Darrick J. Wong
2018-05-14 10:26 ` Brian Foster
0 siblings, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-11 23:14 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Fri, May 11, 2018 at 11:20:27AM -0400, Brian Foster wrote:
> On Thu, May 10, 2018 at 12:18:56PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Teach xfs_bmapi_remap how to map in unwritten extent and to skip rmap
> > updates. This enables us to rebuild real and unwritten extents from the
> > rmapbt.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/xfs/libxfs/xfs_bmap.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index b63e15a114f3..7660efb5beb0 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -4543,7 +4543,9 @@ xfs_bmapi_remap(
> > ASSERT(len > 0);
> > ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
> > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > - ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
> > + ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
> > + XFS_BMAPI_NORMAP)));
> > + ASSERT(!(flags & XFS_BMAPI_ATTRFORK) || !(flags & XFS_BMAPI_PREALLOC));
>
> A little confusing.. how about something like?
>
> ASSERT((flags & XFS_BMAPI_ATTRFORK|XFS_BMAPI_PREALLOC) !=
> XFS_BMAPI_ATTRFORK|XFS_BMAPI_PREALLOC);
Ok.
> >
> > if (unlikely(XFS_TEST_ERROR(
> > (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> > @@ -4581,7 +4583,10 @@ xfs_bmapi_remap(
> > got.br_startoff = bno;
> > got.br_startblock = startblock;
> > got.br_blockcount = len;
> > - got.br_state = XFS_EXT_NORM;
> > + if (flags & XFS_BMAPI_PREALLOC)
> > + got.br_state = XFS_EXT_UNWRITTEN;
> > + else
> > + got.br_state = XFS_EXT_NORM;
>
> It's hard to see the wider picture without any users of this, but I do
> notice that the one current caller appears to have everything passed
> down to it to construct an xfs_bmbt_irec (in fact the data appears to
> come from an irec in the first place).
Later on in the online repair series (which I will send out after the
review for this series concludes) I'll make use of xfs_bmapi_remap to
reconstruct the bmbt from the rmap, which is why I've been busy adding
BMAPI_ATTRFORK, BMAPI_NORMAP, and BMAPI_PREALLOC to this function.
This patch[1] in my branch demonstrates the new uses of this function.
--D
[1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/commit/?h=djwong-devel&id=7d8601ba8a938179ec0dae18b8a0848122eaf5e5
> Brian
>
> >
> > error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
> > &cur, &got, &firstblock, dfops, &logflags, flags);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags
2018-05-11 23:14 ` Darrick J. Wong
@ 2018-05-14 10:26 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-14 10:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Fri, May 11, 2018 at 04:14:03PM -0700, Darrick J. Wong wrote:
> On Fri, May 11, 2018 at 11:20:27AM -0400, Brian Foster wrote:
> > On Thu, May 10, 2018 at 12:18:56PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > Teach xfs_bmapi_remap how to map in unwritten extent and to skip rmap
> > > updates. This enables us to rebuild real and unwritten extents from the
> > > rmapbt.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > fs/xfs/libxfs/xfs_bmap.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index b63e15a114f3..7660efb5beb0 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -4543,7 +4543,9 @@ xfs_bmapi_remap(
> > > ASSERT(len > 0);
> > > ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
> > > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > > - ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
> > > + ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
> > > + XFS_BMAPI_NORMAP)));
> > > + ASSERT(!(flags & XFS_BMAPI_ATTRFORK) || !(flags & XFS_BMAPI_PREALLOC));
> >
> > A little confusing.. how about something like?
> >
> > ASSERT((flags & XFS_BMAPI_ATTRFORK|XFS_BMAPI_PREALLOC) !=
> > XFS_BMAPI_ATTRFORK|XFS_BMAPI_PREALLOC);
>
> Ok.
>
> > >
> > > if (unlikely(XFS_TEST_ERROR(
> > > (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> > > @@ -4581,7 +4583,10 @@ xfs_bmapi_remap(
> > > got.br_startoff = bno;
> > > got.br_startblock = startblock;
> > > got.br_blockcount = len;
> > > - got.br_state = XFS_EXT_NORM;
> > > + if (flags & XFS_BMAPI_PREALLOC)
> > > + got.br_state = XFS_EXT_UNWRITTEN;
> > > + else
> > > + got.br_state = XFS_EXT_NORM;
> >
> > It's hard to see the wider picture without any users of this, but I do
> > notice that the one current caller appears to have everything passed
> > down to it to construct an xfs_bmbt_irec (in fact the data appears to
> > come from an irec in the first place).
>
> Later on in the online repair series (which I will send out after the
> review for this series concludes) I'll make use of xfs_bmapi_remap to
> reconstruct the bmbt from the rmap, which is why I've been busy adding
> BMAPI_ATTRFORK, BMAPI_NORMAP, and BMAPI_PREALLOC to this function.
>
> This patch[1] in my branch demonstrates the new uses of this function.
>
Thanks. FWIW, this caller also looks like it ultimately passes down data
from an xfs_bmbt_irec. Perhaps this or some of the others would need a
local copy of the record to handle incremental/partial calls as such,
but I wonder how much code would be cleaned out by just passing down an
irec directly rather than unrolling the fields and turning it back into
an xfs_bmbt_irec in xfs_bmapi_remap(). But that's a cleanup for another
patch...
Brian
> --D
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/commit/?h=djwong-devel&id=7d8601ba8a938179ec0dae18b8a0848122eaf5e5
>
> > Brian
> >
> > >
> > > error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
> > > &cur, &got, &firstblock, dfops, &logflags, flags);
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags
2018-05-10 19:18 ` [PATCH 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags Darrick J. Wong
2018-05-11 15:20 ` Brian Foster
@ 2018-05-11 23:46 ` Darrick J. Wong
2018-05-14 10:26 ` Brian Foster
1 sibling, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2018-05-11 23:46 UTC (permalink / raw)
To: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Teach xfs_bmapi_remap how to map in unwritten extent and to skip rmap
updates. This enables us to rebuild real and unwritten extents from the
rmapbt.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: tidy up the assert checking for ATTR|PREALLOC
---
fs/xfs/libxfs/xfs_bmap.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index b63e15a114f3..7b0e2b551e23 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4543,7 +4543,10 @@ xfs_bmapi_remap(
ASSERT(len > 0);
ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
- ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
+ ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
+ XFS_BMAPI_NORMAP)));
+ ASSERT((flags & (XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC)) !=
+ (XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC));
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
@@ -4581,7 +4584,10 @@ xfs_bmapi_remap(
got.br_startoff = bno;
got.br_startblock = startblock;
got.br_blockcount = len;
- got.br_state = XFS_EXT_NORM;
+ if (flags & XFS_BMAPI_PREALLOC)
+ got.br_state = XFS_EXT_UNWRITTEN;
+ else
+ got.br_state = XFS_EXT_NORM;
error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
&cur, &got, &firstblock, dfops, &logflags, flags);
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 8/8] xfs: teach xfs_bmapi_remap to accept some bmapi flags
2018-05-11 23:46 ` [PATCH v2 " Darrick J. Wong
@ 2018-05-14 10:26 ` Brian Foster
0 siblings, 0 replies; 24+ messages in thread
From: Brian Foster @ 2018-05-14 10:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Fri, May 11, 2018 at 04:46:16PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Teach xfs_bmapi_remap how to map in unwritten extent and to skip rmap
> updates. This enables us to rebuild real and unwritten extents from the
> rmapbt.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: tidy up the assert checking for ATTR|PREALLOC
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_bmap.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index b63e15a114f3..7b0e2b551e23 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4543,7 +4543,10 @@ xfs_bmapi_remap(
> ASSERT(len > 0);
> ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> - ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK)));
> + ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
> + XFS_BMAPI_NORMAP)));
> + ASSERT((flags & (XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC)) !=
> + (XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC));
>
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> @@ -4581,7 +4584,10 @@ xfs_bmapi_remap(
> got.br_startoff = bno;
> got.br_startblock = startblock;
> got.br_blockcount = len;
> - got.br_state = XFS_EXT_NORM;
> + if (flags & XFS_BMAPI_PREALLOC)
> + got.br_state = XFS_EXT_UNWRITTEN;
> + else
> + got.br_state = XFS_EXT_NORM;
>
> error = xfs_bmap_add_extent_hole_real(tp, ip, whichfork, &icur,
> &cur, &got, &firstblock, dfops, &logflags, flags);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread