xfs
[Top] [All Lists]

Re: [PATCH v9 1/6] xfs: Move code around and remove typedefs

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v9 1/6] xfs: Move code around and remove typedefs
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Mon, 24 Jun 2013 17:21:33 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130624053154.GL29376@dastard>
Organization: IBM
References: <1372042107-27332-1-git-send-email-sekharan@xxxxxxxxxx> <1372042107-27332-2-git-send-email-sekharan@xxxxxxxxxx> <20130624053154.GL29376@dastard>
Reply-to: sekharan@xxxxxxxxxx
On Mon, 2013-06-24 at 15:31 +1000, Dave Chinner wrote:
> On Sun, Jun 23, 2013 at 09:48:22PM -0500, Chandra Seetharaman wrote:
> > Removed some typedefs, defined new functions, made some code clean up all in
> > preparation of the series.
> > 
> > No functional changes.
> 
> This does a lot of different stuff. Can you separate out the actual
> factoring changes (e.g. xfs_is_quota_inode() and xfs_dquot_tree())
> into separate patches, and the same with the struct xfs_dquot_acct
> as it changes both logic and structure...

Sure. I will follow Ben's suggestion in splitting these.
> 
> > diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
> > index 5d16a6e..4b330f2 100644
> > --- a/fs/xfs/xfs_qm.h
> > +++ b/fs/xfs/xfs_qm.h
> > @@ -42,57 +42,89 @@ extern struct kmem_zone *xfs_qm_dqtrxzone;
> >   * The mount structure keeps a pointer to this.
> >   */
> >  typedef struct xfs_quotainfo {
> > -   struct radix_tree_root qi_uquota_tree;
> > -   struct radix_tree_root qi_gquota_tree;
> > -   struct mutex qi_tree_lock;
> > -   xfs_inode_t     *qi_uquotaip;    /* user quota inode */
> > -   xfs_inode_t     *qi_gquotaip;    /* group quota inode */
> > -   struct list_head qi_lru_list;
> > -   struct mutex     qi_lru_lock;
> > -   int              qi_lru_count;
> > -   int              qi_dquots;
> > -   time_t           qi_btimelimit;  /* limit for blks timer */
> > -   time_t           qi_itimelimit;  /* limit for inodes timer */
> > -   time_t           qi_rtbtimelimit;/* limit for rt blks timer */
> > -   xfs_qwarncnt_t   qi_bwarnlimit;  /* limit for blks warnings */
> > -   xfs_qwarncnt_t   qi_iwarnlimit;  /* limit for inodes warnings */
> > -   xfs_qwarncnt_t   qi_rtbwarnlimit;/* limit for rt blks warnings */
> > -   struct mutex     qi_quotaofflock;/* to serialize quotaoff */
> > -   xfs_filblks_t    qi_dqchunklen;  /* # BBs in a chunk of dqs */
> > -   uint             qi_dqperchunk;  /* # ondisk dqs in above chunk */
> > -   xfs_qcnt_t       qi_bhardlimit;  /* default data blk hard limit */
> > -   xfs_qcnt_t       qi_bsoftlimit;  /* default data blk soft limit */
> > -   xfs_qcnt_t       qi_ihardlimit;  /* default inode count hard limit */
> > -   xfs_qcnt_t       qi_isoftlimit;  /* default inode count soft limit */
> > -   xfs_qcnt_t       qi_rtbhardlimit;/* default realtime blk hard limit */
> > -   xfs_qcnt_t       qi_rtbsoftlimit;/* default realtime blk soft limit */
> > -   struct shrinker  qi_shrinker;
> > +   struct radix_tree_root  qi_uquota_tree;
> > +   struct radix_tree_root  qi_gquota_tree;
> > +   struct mutex            qi_tree_lock;
> > +   struct xfs_inode        *qi_uquotaip;    /* user quota inode */
> > +   struct xfs_inode        *qi_gquotaip;    /* group quota inode */
> > +   struct list_head        qi_lru_list;
> > +   struct mutex            qi_lru_lock;
> > +   int             qi_lru_count;
> > +   int             qi_dquots;
> > +   time_t          qi_btimelimit;   /* limit for blks timer */
> > +   time_t          qi_itimelimit;   /* limit for inodes timer */
> > +   time_t          qi_rtbtimelimit;/* limit for rt blks timer */
> > +   xfs_qwarncnt_t  qi_bwarnlimit;   /* limit for blks warnings */
> > +   xfs_qwarncnt_t  qi_iwarnlimit;   /* limit for inodes warnings */
> > +   xfs_qwarncnt_t  qi_rtbwarnlimit;/* limit for rt blks warnings */
> > +   struct mutex    qi_quotaofflock;/* to serialize quotaoff */
> > +   xfs_filblks_t   qi_dqchunklen;   /* # BBs in a chunk of dqs */
> > +   uint            qi_dqperchunk;   /* # ondisk dqs in above chunk */
> > +   xfs_qcnt_t      qi_bhardlimit;   /* default data blk hard limit */
> > +   xfs_qcnt_t      qi_bsoftlimit;   /* default data blk soft limit */
> > +   xfs_qcnt_t      qi_ihardlimit;   /* default inode count hard limit */
> > +   xfs_qcnt_t      qi_isoftlimit;   /* default inode count soft limit */
> > +   xfs_qcnt_t      qi_rtbhardlimit;/* default realtime blk hard limit */
> > +   xfs_qcnt_t      qi_rtbsoftlimit;/* default realtime blk soft limit */
> > +   struct shrinker qi_shrinker;
> >  } xfs_quotainfo_t;
> 
> I don't see much point in random whitespace cleanups like this. If
> you are going to change them, at least clean it up properly, so all
> variables are aligned, and all the comments are aligned as well. In
> most cases, the comments are redundant because the variable names
> are very descriptive and so probably could be removed....

I will align all these at the cost of these "valuable" comments :)
> 
> Cheers,
> 
> Dave.


<Prev in Thread] Current Thread [Next in Thread>