xfs
[Top] [All Lists]

Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc

To: Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 17 Jan 2014 13:33:55 -0500
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52D97289.7030401@xxxxxxxxxxx>
References: <1389981723-54311-1-git-send-email-bfoster@xxxxxxxxxx> <52D97289.7030401@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
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@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);
>>              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);
>>  }
>>
> 

<Prev in Thread] Current Thread [Next in Thread>