On Sun, Apr 22, 2018 at 08:06:44AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Separate the disk dquot read and allocation functionality into
> two helper functions, then refactor dqread to call them directly.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_dquot.c | 261 ++++++++++++++++++++--------------------------------
> 1 file changed, 99 insertions(+), 162 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 7154909..ef4cd56 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
...
> @@ -574,74 +532,53 @@ xfs_qm_dqread(
> xfs_dqid_t id,
> uint type,
> uint flags,
> - struct xfs_dquot **O_dqpp)
> + struct xfs_dquot **dqpp)
> {
> struct xfs_dquot *dqp;
> - struct xfs_disk_dquot *ddqp;
> struct xfs_buf *bp;
> - struct xfs_trans *tp = NULL;
> + struct xfs_trans *tp;
> int error;
>
> dqp = xfs_qm_dqinit_once(mp, id, type);
> trace_xfs_dqread(dqp);
>
> - if (flags & XFS_QMOPT_DQALLOC) {
> + /* Try to read the buffer... */
> + error = xfs_qm_dqread_ondisk(mp, dqp, &bp);
> + if (error == -ENOENT && (flags & XFS_QMOPT_DQALLOC)) {
> + /* ...or allocate a new block and buffer. */
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
> XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);
> if (error)
> - goto error0;
> - }
> + goto err;
>
> - /*
> - * get a pointer to the on-disk dquot and the buffer containing it
> - * dqp already knows its own type (GROUP/USER).
> - */
> - error = xfs_qm_dqtobp(&tp, dqp, &ddqp, &bp, flags);
> - if (error) {
> - /*
> - * This can happen if quotas got turned off (ESRCH),
> - * or if the dquot didn't exist on disk and we ask to
> - * allocate (ENOENT).
> - */
> - trace_xfs_dqread_fail(dqp);
> - goto error1;
> - }
> + error = xfs_qm_dqalloc_ondisk(&tp, dqp, &bp);
> + if (error)
> + goto err_cancel;
>
> - xfs_qm_dqinit_from_buf(dqp, ddqp);
> + error = xfs_trans_commit(tp);
> + }
> + ASSERT(xfs_buf_islocked(bp));
Doesn't this fail if the tx commit above fails? Granted we've probably
shut down...
Otherwise looks fine modulo Christoph's comments.
Brian
P.S., I see that assert was fixed up in the next patch, but then we have
a couple of the same calls separated by the init call. That probably
should be fixed up here in any event...
> + if (error)
> + goto err;
>
> - /* Mark the buf so that this will stay incore a little longer */
> - xfs_buf_set_ref(bp, XFS_DQUOT_REF);
> + xfs_qm_dqinit_from_buf(dqp, bp);
>
> /*
> - * We got the buffer with a xfs_trans_read_buf() (in dqtobp())
> - * So we need to release with xfs_trans_brelse().
> - * The strategy here is identical to that of inodes; we lock
> - * the dquot in xfs_qm_dqget() before making it accessible to
> - * others. This is because dquots, like inodes, need a good level of
> - * concurrency, and we don't want to take locks on the entire buffers
> - * for dquot accesses.
> - * Note also that the dquot buffer may even be dirty at this point, if
> - * this particular dquot was repaired. We still aren't afraid to
> - * brelse it because we have the changes incore.
> + * At this point we should have a clean locked buffer. Release the
> + * buffer since the incore dquot has its own copy and locking
> + * protocol so we needn't tie up the buffer any further.
> */
> ASSERT(xfs_buf_islocked(bp));
> - xfs_trans_brelse(tp, bp);
> -
> - if (tp) {
> - error = xfs_trans_commit(tp);
> - if (error)
> - goto error0;
> - }
> -
> - *O_dqpp = dqp;
> + xfs_buf_relse(bp);
> + *dqpp = dqp;
> return error;
>
> -error1:
> - if (tp)
> - xfs_trans_cancel(tp);
> -error0:
> +err_cancel:
> + xfs_trans_cancel(tp);
> +err:
> + trace_xfs_dqread_fail(dqp);
> xfs_qm_dqdestroy(dqp);
> - *O_dqpp = NULL;
> + *dqpp = NULL;
> return error;
> }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
