* [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc @ 2014-01-17 18:02 Brian Foster 2014-01-17 18:12 ` Eric Sandeen 2014-01-18 14:17 ` Jeff Liu 0 siblings, 2 replies; 5+ messages in thread From: Brian Foster @ 2014-01-17 18:02 UTC (permalink / raw) To: xfs The dquot allocation path in xfs_qm_dqread() currently uses the attribute set log reservation, which appears to be incorrect. We have reports of transaction reservation overruns with the current code. E.g., a repeated run of xfstests test generic/270 on a 512b block size fs occassionally produces the following in dmesg: XFS (sdN): xlog_write: reservation summary: trans type = QM_DQALLOC (30) unit res = 7080 bytes current res = -632 bytes total reg = 0 bytes (o/flow = 0 bytes) ophdrs = 0 (ophdr space = 0 bytes) ophdr + reg = 0 bytes num regions = 0 XFS (sdN): xlog_write: reservation ran out. Need to up reservation The dquot allocation case should consist of a write reservation (i.e., we are allocating a range of the internal quota file) plus the size of the actual dquots. We already have a log reservation definition for this operation (tr_qm_dqalloc). Use it in xfs_qm_dqread() and update the log reservation calculation function to use the write res. calculation function rather than reading the assumed to be pre-calculated value directly. Signed-off-by: Brian Foster <bfoster@redhat.com> --- Hi all, This issue was reported here: https://bugzilla.redhat.com/show_bug.cgi?id=1052787 ... and the patch seems to address the reservation overrun from my testing. It also runs through an xfstests regression. Thanks. Brian fs/xfs/xfs_dquot.c | 2 +- fs/xfs/xfs_trans_resv.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index 6b1e695..06280c6 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -614,7 +614,7 @@ xfs_qm_dqread( if (flags & XFS_QMOPT_DQALLOC) { tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC); - error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm, + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc, XFS_QM_DQALLOC_SPACE_RES(mp), 0); if (error) goto error1; diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c index 2fd59c0..60b7d40 100644 --- a/fs/xfs/xfs_trans_resv.c +++ b/fs/xfs/xfs_trans_resv.c @@ -651,8 +651,7 @@ STATIC uint xfs_calc_qm_dqalloc_reservation( struct xfs_mount *mp) { - ASSERT(M_RES(mp)->tr_write.tr_logres); - return M_RES(mp)->tr_write.tr_logres + + return xfs_calc_write_reservation(mp) + xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1); } -- 1.8.1.4 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc 2014-01-17 18:02 [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc Brian Foster @ 2014-01-17 18:12 ` Eric Sandeen 2014-01-17 18:33 ` Brian Foster 2014-01-18 14:17 ` Jeff Liu 1 sibling, 1 reply; 5+ messages in thread From: Eric Sandeen @ 2014-01-17 18:12 UTC (permalink / raw) To: Brian Foster, xfs On 1/17/14, 12:02 PM, Brian Foster wrote: > The dquot allocation path in xfs_qm_dqread() currently uses the > attribute set log reservation, which appears to be incorrect. We > have reports of transaction reservation overruns with the current > code. E.g., a repeated run of xfstests test generic/270 on a 512b > block size fs occassionally produces the following in dmesg: > > XFS (sdN): xlog_write: reservation summary: > trans type = QM_DQALLOC (30) > unit res = 7080 bytes > current res = -632 bytes > total reg = 0 bytes (o/flow = 0 bytes) > ophdrs = 0 (ophdr space = 0 bytes) > ophdr + reg = 0 bytes > num regions = 0 > > XFS (sdN): xlog_write: reservation ran out. Need to up reservation > > The dquot allocation case should consist of a write reservation > (i.e., we are allocating a range of the internal quota file) plus > the size of the actual dquots. We already have a log reservation > definition for this operation (tr_qm_dqalloc). Use it in > xfs_qm_dqread() and update the log reservation calculation function > to use the write res. calculation function rather than reading the > assumed to be pre-calculated value directly. good catch; I think there is at least one more issue in the patch that refactored all of this: - if ((error = xfs_trans_reserve(tp, resblks, - XFS_GROWRTALLOC_LOG_RES(mp), 0, - XFS_TRANS_PERM_LOG_RES, - XFS_DEFAULT_PERM_LOG_COUNT))) + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_growdata, + resblks, 0); shouldn't it be tr_growrtalloc ? -Eric > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > Hi all, > > This issue was reported here: > > https://bugzilla.redhat.com/show_bug.cgi?id=1052787 > > ... and the patch seems to address the reservation overrun from my testing. > It also runs through an xfstests regression. Thanks. > > Brian > > fs/xfs/xfs_dquot.c | 2 +- > fs/xfs/xfs_trans_resv.c | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index 6b1e695..06280c6 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -614,7 +614,7 @@ xfs_qm_dqread( > > if (flags & XFS_QMOPT_DQALLOC) { > tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC); > - error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm, > + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc, > XFS_QM_DQALLOC_SPACE_RES(mp), 0); > if (error) > goto error1; > diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c > index 2fd59c0..60b7d40 100644 > --- a/fs/xfs/xfs_trans_resv.c > +++ b/fs/xfs/xfs_trans_resv.c > @@ -651,8 +651,7 @@ STATIC uint > xfs_calc_qm_dqalloc_reservation( > struct xfs_mount *mp) > { > - ASSERT(M_RES(mp)->tr_write.tr_logres); > - return M_RES(mp)->tr_write.tr_logres + > + return xfs_calc_write_reservation(mp) + > xfs_calc_buf_res(1, > XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1); > } > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc 2014-01-17 18:12 ` Eric Sandeen @ 2014-01-17 18:33 ` Brian Foster 0 siblings, 0 replies; 5+ messages in thread From: Brian Foster @ 2014-01-17 18:33 UTC (permalink / raw) To: Eric Sandeen, xfs On 01/17/2014 01:12 PM, Eric Sandeen wrote: > On 1/17/14, 12:02 PM, Brian Foster wrote: >> The dquot allocation path in xfs_qm_dqread() currently uses the >> attribute set log reservation, which appears to be incorrect. We >> have reports of transaction reservation overruns with the current >> code. E.g., a repeated run of xfstests test generic/270 on a 512b >> block size fs occassionally produces the following in dmesg: >> >> XFS (sdN): xlog_write: reservation summary: >> trans type = QM_DQALLOC (30) >> unit res = 7080 bytes >> current res = -632 bytes >> total reg = 0 bytes (o/flow = 0 bytes) >> ophdrs = 0 (ophdr space = 0 bytes) >> ophdr + reg = 0 bytes >> num regions = 0 >> >> XFS (sdN): xlog_write: reservation ran out. Need to up reservation >> >> The dquot allocation case should consist of a write reservation >> (i.e., we are allocating a range of the internal quota file) plus >> the size of the actual dquots. We already have a log reservation >> definition for this operation (tr_qm_dqalloc). Use it in >> xfs_qm_dqread() and update the log reservation calculation function >> to use the write res. calculation function rather than reading the >> assumed to be pre-calculated value directly. > > > good catch; I think there is at least one more issue in the patch that > refactored all of this: > > - if ((error = xfs_trans_reserve(tp, resblks, > - XFS_GROWRTALLOC_LOG_RES(mp), 0, > - XFS_TRANS_PERM_LOG_RES, > - XFS_DEFAULT_PERM_LOG_COUNT))) > + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_growdata, > + resblks, 0); > > shouldn't it be tr_growrtalloc ? > Yep, looks like it. XFS_GROWRTALLOC_LOG_RES() maps to tr_growrtalloc prior to that commit (3d3c8b5222). Thanks for catching that, I'll fire another one off shortly... Brian > -Eric > >> Signed-off-by: Brian Foster <bfoster@redhat.com> >> --- >> Hi all, >> >> This issue was reported here: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1052787 >> >> ... and the patch seems to address the reservation overrun from my testing. >> It also runs through an xfstests regression. Thanks. >> >> Brian >> >> fs/xfs/xfs_dquot.c | 2 +- >> fs/xfs/xfs_trans_resv.c | 3 +-- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c >> index 6b1e695..06280c6 100644 >> --- a/fs/xfs/xfs_dquot.c >> +++ b/fs/xfs/xfs_dquot.c >> @@ -614,7 +614,7 @@ xfs_qm_dqread( >> >> if (flags & XFS_QMOPT_DQALLOC) { >> tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC); >> - error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm, >> + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc, >> XFS_QM_DQALLOC_SPACE_RES(mp), 0); >> if (error) >> goto error1; >> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c >> index 2fd59c0..60b7d40 100644 >> --- a/fs/xfs/xfs_trans_resv.c >> +++ b/fs/xfs/xfs_trans_resv.c >> @@ -651,8 +651,7 @@ STATIC uint >> xfs_calc_qm_dqalloc_reservation( >> struct xfs_mount *mp) >> { >> - ASSERT(M_RES(mp)->tr_write.tr_logres); >> - return M_RES(mp)->tr_write.tr_logres + >> + return xfs_calc_write_reservation(mp) + >> xfs_calc_buf_res(1, >> XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1); >> } >> > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc 2014-01-17 18:02 [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc Brian Foster 2014-01-17 18:12 ` Eric Sandeen @ 2014-01-18 14:17 ` Jeff Liu 2014-01-20 13:54 ` Brian Foster 1 sibling, 1 reply; 5+ messages in thread From: Jeff Liu @ 2014-01-18 14:17 UTC (permalink / raw) To: Brian Foster, xfs On 01/18 2014 02:02 AM, Brian Foster wrote: > The dquot allocation path in xfs_qm_dqread() currently uses the > attribute set log reservation, which appears to be incorrect. We > have reports of transaction reservation overruns with the current > code. E.g., a repeated run of xfstests test generic/270 on a 512b > block size fs occassionally produces the following in dmesg: > > XFS (sdN): xlog_write: reservation summary: > trans type = QM_DQALLOC (30) > unit res = 7080 bytes > current res = -632 bytes > total reg = 0 bytes (o/flow = 0 bytes) > ophdrs = 0 (ophdr space = 0 bytes) > ophdr + reg = 0 bytes > num regions = 0 > > XFS (sdN): xlog_write: reservation ran out. Need to up reservation > > The dquot allocation case should consist of a write reservation > (i.e., we are allocating a range of the internal quota file) plus > the size of the actual dquots. We already have a log reservation > definition for this operation (tr_qm_dqalloc). Use it in > xfs_qm_dqread() and update the log reservation calculation function > to use the write res. calculation function rather than reading the > assumed to be pre-calculated value directly. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > Hi all, > > This issue was reported here: > > https://bugzilla.redhat.com/show_bug.cgi?id=1052787 > > ... and the patch seems to address the reservation overrun from my testing. > It also runs through an xfstests regression. Thanks. > > Brian > > fs/xfs/xfs_dquot.c | 2 +- > fs/xfs/xfs_trans_resv.c | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index 6b1e695..06280c6 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -614,7 +614,7 @@ xfs_qm_dqread( > > if (flags & XFS_QMOPT_DQALLOC) { > tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC); > - error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm, > + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc, > XFS_QM_DQALLOC_SPACE_RES(mp), 0); Ah, another issue I introduced for refactoring xfs_trans_reserve() interface. > if (error) > goto error1; > diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c > index 2fd59c0..60b7d40 100644 > --- a/fs/xfs/xfs_trans_resv.c > +++ b/fs/xfs/xfs_trans_resv.c > @@ -651,8 +651,7 @@ STATIC uint > xfs_calc_qm_dqalloc_reservation( > struct xfs_mount *mp) > { > - ASSERT(M_RES(mp)->tr_write.tr_logres); > - return M_RES(mp)->tr_write.tr_logres + > + return xfs_calc_write_reservation(mp) + > xfs_calc_buf_res(1, > XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1); > } I'm fine to call xfs_calc_write_reservation() rather than the current hard-code, especially we can get rid of the ASSERT() though tr_write.tr_logres should not be zero as it is initialized in advance at xfs_trans_resv_calc(). Just a little nit pick :), that would be better to consolidate the comments in this fix as well, i.e, * the write transaction log space: M_RES(mp)->tr_write.tr_logres. Thanks, -Jeff _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc 2014-01-18 14:17 ` Jeff Liu @ 2014-01-20 13:54 ` Brian Foster 0 siblings, 0 replies; 5+ messages in thread From: Brian Foster @ 2014-01-20 13:54 UTC (permalink / raw) To: Jeff Liu, xfs On 01/18/2014 09:17 AM, Jeff Liu wrote: > On 01/18 2014 02:02 AM, Brian Foster wrote: >> The dquot allocation path in xfs_qm_dqread() currently uses the >> attribute set log reservation, which appears to be incorrect. We >> have reports of transaction reservation overruns with the current >> code. E.g., a repeated run of xfstests test generic/270 on a 512b >> block size fs occassionally produces the following in dmesg: >> >> XFS (sdN): xlog_write: reservation summary: >> trans type = QM_DQALLOC (30) >> unit res = 7080 bytes >> current res = -632 bytes >> total reg = 0 bytes (o/flow = 0 bytes) >> ophdrs = 0 (ophdr space = 0 bytes) >> ophdr + reg = 0 bytes >> num regions = 0 >> >> XFS (sdN): xlog_write: reservation ran out. Need to up reservation >> >> The dquot allocation case should consist of a write reservation >> (i.e., we are allocating a range of the internal quota file) plus >> the size of the actual dquots. We already have a log reservation >> definition for this operation (tr_qm_dqalloc). Use it in >> xfs_qm_dqread() and update the log reservation calculation function >> to use the write res. calculation function rather than reading the >> assumed to be pre-calculated value directly. >> >> Signed-off-by: Brian Foster <bfoster@redhat.com> >> --- >> Hi all, >> >> This issue was reported here: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1052787 >> >> ... and the patch seems to address the reservation overrun from my testing. >> It also runs through an xfstests regression. Thanks. >> >> Brian >> >> fs/xfs/xfs_dquot.c | 2 +- >> fs/xfs/xfs_trans_resv.c | 3 +-- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c >> index 6b1e695..06280c6 100644 >> --- a/fs/xfs/xfs_dquot.c >> +++ b/fs/xfs/xfs_dquot.c >> @@ -614,7 +614,7 @@ xfs_qm_dqread( >> >> if (flags & XFS_QMOPT_DQALLOC) { >> tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC); >> - error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm, >> + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc, >> XFS_QM_DQALLOC_SPACE_RES(mp), 0); > > Ah, another issue I introduced for refactoring xfs_trans_reserve() interface. >> if (error) >> goto error1; >> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c >> index 2fd59c0..60b7d40 100644 >> --- a/fs/xfs/xfs_trans_resv.c >> +++ b/fs/xfs/xfs_trans_resv.c >> @@ -651,8 +651,7 @@ STATIC uint >> xfs_calc_qm_dqalloc_reservation( >> struct xfs_mount *mp) >> { >> - ASSERT(M_RES(mp)->tr_write.tr_logres); >> - return M_RES(mp)->tr_write.tr_logres + >> + return xfs_calc_write_reservation(mp) + >> xfs_calc_buf_res(1, >> XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1); >> } > > I'm fine to call xfs_calc_write_reservation() rather than the current hard-code, > especially we can get rid of the ASSERT() though tr_write.tr_logres should not > be zero as it is initialized in advance at xfs_trans_resv_calc(). > Right... I just decided to call the write calc size function because that seems to be the standard operating procedure with the new code and it eliminates the fact that this is subject to initialization order, even if not a bug at the moment. :) > Just a little nit pick :), that would be better to consolidate the comments in > this fix as well, i.e, > * the write transaction log space: M_RES(mp)->tr_write.tr_logres. > Ok, I'll send a new patch that cleans up the comment. Thanks for review. Brian > > Thanks, > -Jeff > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-20 13:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-17 18:02 [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc Brian Foster 2014-01-17 18:12 ` Eric Sandeen 2014-01-17 18:33 ` Brian Foster 2014-01-18 14:17 ` Jeff Liu 2014-01-20 13:54 ` Brian Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).