Re: [PATCH v3 1/4] xfs: remove the xfs_disk_dquot_t and xfs_dquot_t

[Dave Chinner <david@fromorbit.com>]

On Sun, Nov 10, 2019 at 07:24:01AM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>

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@fromorbit.com