Re: [PATCH v3 2/6] xfs: introduce xfs_dialloc_roll()

Gao Xiang
2020-12-07T14:12:52Z

On Mon, Dec 07, 2020 at 02:43:50PM +0100, Christoph Hellwig wrote:
> > + /*
> > + * We want the quota changes to be associated with the next transaction,
> > + * NOT this one. So, detach the dqinfo from this and attach it to the
> > + * next transaction.
> > + */
> > + if (tp->t_dqinfo) {
> > + dqinfo = tp->t_dqinfo;
> > + tp->t_dqinfo = NULL;
> > + tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> > + tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
>
> No need for the braces here.

Ok, will fix.

>
> > + if (error) {
> > + xfs_buf_relse(agibp);
> > + return error;
> > + }
>
> I haven't looked over the other patches if there is a good reason for
> it, but to me keeping the xfs_buf_relse in the caller would seem more
> natural.

This part is inherited from Dave's original patch, but I guess I could
move this to all callers if needed, no strong opinion from myself.

>
> >
> > +/* XXX: will be removed in the following patch */
> > +int
>
> I don't think the comment is very helpful. As of this patch the
> declaration is obviously needed, and that is all that matters.

Ok, will remove it.

>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>