xfs
[Top] [All Lists]

Re: [PATCH 2/3] xfs: consolidate superblock logging functions

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 2/3] xfs: consolidate superblock logging functions
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 9 Jan 2015 10:20:30 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150108143632.GB33789@xxxxxxxxxxxxxxx>
References: <1420667465-7453-1-git-send-email-david@xxxxxxxxxxxxx> <1420667465-7453-3-git-send-email-david@xxxxxxxxxxxxx> <20150108143632.GB33789@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jan 08, 2015 at 09:36:33AM -0500, Brian Foster wrote:
> On Thu, Jan 08, 2015 at 08:51:04AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > We now have several superblock loggin functions that are identical
> > except for the transaction reservation and whether it shoul dbe a
> > synchronous transaction or not. Consolidate these all into a single
> > function, a single reserveration and a sync flag and call it
> > xfs_sync_sb().
> > 
> > Also, xfs_mod_sb() is not really a modification function - it's the
> > operation of logging the superblock buffer. hence change the name of
> > it to reflect this.
> > 
> > Note that we have to change the mp->m_update_flags that are passed
> > around at mount time to a boolean simply to indicate a superblock
> > update is needed.
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> 
> A question and one other little thing...

[snip]

> >  #define XFS_TRANS_QM_DQCLUSTER             32
> >  #define XFS_TRANS_QM_QINOCREATE            33
> >  #define XFS_TRANS_QM_QUOTAOFF_END  34
> > -#define XFS_TRANS_SB_UNIT          35
> > -#define XFS_TRANS_FSYNC_TS         36
> > -#define    XFS_TRANS_GROWFSRT_ALLOC        37
> > -#define    XFS_TRANS_GROWFSRT_ZERO         38
> > -#define    XFS_TRANS_GROWFSRT_FREE         39
> > -#define    XFS_TRANS_SWAPEXT               40
> > -#define    XFS_TRANS_SB_COUNT              41
> > -#define    XFS_TRANS_CHECKPOINT            42
> > -#define    XFS_TRANS_ICREATE               43
> > -#define    XFS_TRANS_CREATE_TMPFILE        44
> > -#define    XFS_TRANS_TYPE_MAX              44
> > +#define XFS_TRANS_FSYNC_TS         35
> > +#define    XFS_TRANS_GROWFSRT_ALLOC        36
> > +#define    XFS_TRANS_GROWFSRT_ZERO         37
> > +#define    XFS_TRANS_GROWFSRT_FREE         38
> > +#define    XFS_TRANS_SWAPEXT               39
> > +#define    XFS_TRANS_CHECKPOINT            40
> > +#define    XFS_TRANS_ICREATE               41
> > +#define    XFS_TRANS_CREATE_TMPFILE        42
> > +#define    XFS_TRANS_TYPE_MAX              43
> >  /* new transaction types need to be reflected in xfs_logprint(8) */
> >  
> >  #define XFS_TRANS_TYPES \
> > @@ -113,7 +111,6 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> >     { XFS_TRANS_SETATTR_SIZE,       "SETATTR_SIZE" }, \
> >     { XFS_TRANS_INACTIVE,           "INACTIVE" }, \
> >     { XFS_TRANS_CREATE,             "CREATE" }, \
> > -   { XFS_TRANS_CREATE_TMPFILE,     "CREATE_TMPFILE" }, \
> >     { XFS_TRANS_CREATE_TRUNC,       "CREATE_TRUNC" }, \
> >     { XFS_TRANS_TRUNCATE_FILE,      "TRUNCATE_FILE" }, \
> >     { XFS_TRANS_REMOVE,             "REMOVE" }, \
> > @@ -134,23 +131,23 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> >     { XFS_TRANS_ATTR_RM,            "ATTR_RM" }, \
> >     { XFS_TRANS_ATTR_FLAG,          "ATTR_FLAG" }, \
> >     { XFS_TRANS_CLEAR_AGI_BUCKET,   "CLEAR_AGI_BUCKET" }, \
> > -   { XFS_TRANS_QM_SBCHANGE,        "QM_SBCHANGE" }, \
> > +   { XFS_TRANS_SB_CHANGE,          "SBCHANGE" }, \
> > +   { XFS_TRANS_DUMMY1,             "DUMMY1" }, \
> > +   { XFS_TRANS_DUMMY2,             "DUMMY2" }, \
> 
> I take it we can't just remove these dummy types, for userspace
> compatibility reasons, right? At least, it looks like logprint could get
> confused if other transaction types inherited these values.

With delayed logging, userspace will never see these as they never
get to disk. Userspace will only ever see checkpoints and unmount
records. My plan is to eventually remove this stuff from the kernel
(it isn['t used anymore except in tracing) and define it in
xfs_logprint in a way that works for current and legacy filesystems.
So for the moment, I'm just making the kernel/user values
consistent.

> > @@ -1574,29 +1574,6 @@ xfs_qm_dqfree_one(
> >     xfs_qm_dqdestroy(dqp);
> >  }
> >  
> > -/*
> > - * Start a transaction and write the incore superblock changes to
> > - * disk. flags parameter indicates which fields have changed.
> > - */
> > -int
> > -xfs_qm_write_sb_changes(
> 
> This guy still has a function declaration in xfs_qm.h. The rest looks
> good to me:

Good catch, I'll kill it.

> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

Thanks!

-Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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