On Sun, Nov 10, 2019 at 07:24:01AM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@xxxxxxxxxx>
Looks good, but on minor nit.
<snip>
Personally, I'd just strip most of the comments here completely
as the variable names describe the function of them almost entirely.
This was an old pattern from Irix days, and we've slowly been
removing it as we go as it's mostly clutter and largely redundant
due to the code itself being self describing. Such as:
> @@ -30,33 +30,51 @@ enum {
> /*
> * The incore dquot structure
> */
> -typedef struct xfs_dquot {
> - uint dq_flags; /* various flags (XFS_DQ_*) */
> - struct list_head q_lru; /* global free list of dquots */
> - struct xfs_mount*q_mount; /* filesystem this relates to */
> - uint q_nrefs; /* # active refs from inodes */
> - xfs_daddr_t q_blkno; /* blkno of dquot buffer */
> - int q_bufoffset; /* off of dq in buffer (# dquots) */
> - xfs_fileoff_t q_fileoffset; /* offset in quotas file */
> -
> - xfs_disk_dquot_t q_core; /* actual usage & quotas */
> - xfs_dq_logitem_t q_logitem; /* dquot log item */
> - xfs_qcnt_t q_res_bcount; /* total regular nblks used+reserved */
> - xfs_qcnt_t q_res_icount; /* total inos allocd+reserved */
> - xfs_qcnt_t q_res_rtbcount;/* total realtime blks used+reserved */
> - xfs_qcnt_t q_prealloc_lo_wmark;/* prealloc throttle wmark */
> - xfs_qcnt_t q_prealloc_hi_wmark;/* prealloc disabled wmark */
> - int64_t q_low_space[XFS_QLOWSP_MAX];
> - struct mutex q_qlock; /* quota lock */
> - struct completion q_flush; /* flush completion queue */
> - atomic_t q_pincount; /* dquot pin count */
> - wait_queue_head_t q_pinwait; /* dquot pinning wait queue */
> -} xfs_dquot_t;
> +struct xfs_dquot {
> + /* various flags (XFS_DQ_*) */
> + uint dq_flags;
Convention: xx_flags contain various flags for xx. Self
documenting.
> + /* global free list of dquots */
> + struct list_head q_lru;
Convention - xx_lru generally points to a LRU ordered free list.
Self documenting.
> + /* filesystem this relates to */
> + struct xfs_mount *q_mount;
Convention - xx_mount always points to the filesystem context. Self
documenting.
> + /* # active refs from inodes */
> + uint q_nrefs;
Convention: xx_refs is always an object reference count. Self
documenting.
> + /* blkno of dquot buffer */
> + xfs_daddr_t q_blkno;
Convention: xfs_daddr_t = disk address, blkno = block number. Reads
as "block number on disk of quota object". Self documenting.
> + /* off of dq in buffer (# dquots) */
> + int q_bufoffset;
That comment is wrong. It's not an offset in "# dquots", it's a byte
offset from the start of the buffer. So, the comment should be
removed because plain offsets are in bytes by convention and so
"int q_bufoffset;" is actually self documenting.
> + /* offset in quotas file */
> + xfs_fileoff_t q_fileoffset;
> + /* actual usage & quotas */
> + struct xfs_disk_dquot q_core;
> + /* dquot log item */
> + xfs_dq_logitem_t q_logitem;
These are all self documenting, follow convention.
> + /* total regular nblks used+reserved */
> + xfs_qcnt_t q_res_bcount;
> + /* total inos allocd+reserved */
> + xfs_qcnt_t q_res_icount;
> + /* total realtime blks used+reserved */
> + xfs_qcnt_t q_res_rtbcount;
These are actually Useful comments, because they count both used and
reserved objects.
> + /* prealloc throttle wmark */
> + xfs_qcnt_t q_prealloc_lo_wmark;
> + /* prealloc disabled wmark */
> + xfs_qcnt_t q_prealloc_hi_wmark;
Obvious from the name - watermarks are preallocation thresholds.
> + int64_t q_low_space[XFS_QLOWSP_MAX];
I never commented this one when I added it because it can't be
described in 3-4 words. :)
> + /* quota lock */
> + struct mutex q_qlock;
> + /* flush completion queue */
> + struct completion q_flush;
> + /* dquot pin count */
> + atomic_t q_pincount;
> + /* dquot pinning wait queue */
> + struct wait_queue_head q_pinwait;
And these are all self documenting.
So, really, most of the comments can be removed from this structure.
The only reason for a comment describing a variable is if it's
function or contents is not obvious from it's name, such as the
accounting variables above.
Note that we do the same thing to local variable declarations as we
are changing them. The original Irix code had the same comment
convention for function and local declarations - it's largely just
clutter and noise now, so we also remove them as we go...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
