Re: [PATCH 15/16] xfs: kill xfs_qm_idtodq

Dave Chinner
2011-12-05T06:16:02Z

On Mon, Nov 28, 2011 at 03:27:37AM -0500, Christoph Hellwig wrote:
> This function doesn't help the code flow, so merge the dquot allocation and
> transaction handling into xfs_qm_dqread.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This version is much easier to follow.

Couple of small things:

> ---
> fs/xfs/xfs_dquot.c | 134 ++++++++++++++++++-----------------------------------
> 1 file changed, 47 insertions(+), 87 deletions(-)
>
> Index: xfs/fs/xfs/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:56:40.861789014 +0100
> +++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:56:42.488446869 +0100
> @@ -550,36 +550,59 @@ xfs_qm_dqtobp(
> * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
> * and release the buffer immediately.
> *
> + * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
> */
> -/* ARGSUSED */
> STATIC int
> xfs_qm_dqread(
> - xfs_trans_t **tpp,
> - xfs_dqid_t id,
> - xfs_dquot_t *dqp, /* dquot to get filled in */
> - uint flags)
> + struct xfs_mount *mp,
> + xfs_dqid_t id,
^^^ whitespace damage
> + uint type,
^^^^^ whitespace damage

> + uint flags,
> + struct xfs_dquot **O_dqpp)
> {
> - xfs_disk_dquot_t *ddqp;
> - xfs_buf_t *bp;
> - int error;
> - xfs_trans_t *tp;
> + struct xfs_dquot *dqp;
> + struct xfs_disk_dquot *ddqp;
> + struct xfs_buf *bp;
> + struct xfs_trans *tp = NULL;
> + int error;
> + int cancelflags = 0;

Extra whitespace in the bp/tp/error declarations.

>
> - ASSERT(tpp);
> + dqp = xfs_qm_dqinit(mp, id, type);
>
> trace_xfs_dqread(dqp);
>
> + if (flags & XFS_QMOPT_DQALLOC) {
> + tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
> + error = xfs_trans_reserve(tp, XFS_QM_DQALLOC_SPACE_RES(mp),
> + XFS_WRITE_LOG_RES(mp) +
> + BBTOB(mp->m_quotainfo->qi_dqchunklen) - 1 +
> + 128,

That reservation value is asking for a macro or separate variable
and a comment explaining it....

therwise, looks OK.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com