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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
