On Wed, Oct 18, 2023 at 08:16:48AM +0200, Christoph Hellwig wrote: > On Tue, Oct 17, 2023 at 08:54:08AM -0700, Darrick J. Wong wrote: > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > Consolidate the arguments passed around the rt allocator into a > > struct xfs_rtalloc_arg similar to how the btree allocator arguments > > are consolidated in a struct xfs_alloc_arg.... > > Overall this looks good to me, but a few cosmetic comments: > > > + struct xfs_rtalloc_args *args, > > + xfs_fileoff_t block, /* block number in bitmap or summary */ > > + int issum, /* is summary not bitmap */ > > + struct xfs_buf **bpp) /* output: buffer for the block */ > > we should either also document the new args argument, or drop all the > other argument comments like we've done in many places. If we want > to keep them it would be good to do the trivial reformatting so that > they don't extend over the 80 character readability limit. I prefer to leave the comments because they were immensely helpful for figuring out the correct variable types in the refactoring series. If they can help someone else (e.g. 2024 me) figure out what this code does, they're worth leaving in. However, I'll fix the formatting errors to try to get the lines back to around 80col. I'll also get rid of the pointless comments like: int error; /* error value */ > > + struct xfs_mount *mp = args->mount; > > mount is a bit of an odd name for the member. We usuall calls this > mp in most structures like our normal variable name (with tp->t_mountp > as one notable odd exception). > > > + error = xfs_trans_read_buf(mp, args->trans, mp->m_ddev_targp, > > XFS_FSB_TO_DADDR(mp, map.br_startblock), > > mp->m_bsize, 0, &bp, &xfs_rtbuf_ops); > > ->trans has some precedence in the dir/attr code, but I think tp > would still be more logical. Agreed. --D
