xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Sat, 18 Jan 2014 22:17:45 +0800
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1389981723-54311-1-git-send-email-bfoster@xxxxxxxxxx>
References: <1389981723-54311-1-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
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().

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

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