xfs
[Top] [All Lists]

Re: [PATCH 12/12] repair: cleanup inode record macros

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH 12/12] repair: cleanup inode record macros
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 11 Jan 2012 06:30:20 -0500
In-reply-to: <20111202174743.697687824@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111202174619.179530033@xxxxxxxxxxxxxxxxxxxxxx> <20111202174743.697687824@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
ping?

On Fri, Dec 02, 2011 at 12:46:31PM -0500, Christoph Hellwig wrote:
> Remove indirections in the inode record bit manipulation macros and flatten
> them to a single level of inlines.  Also use a common IREC_MASK define
> instead of duplicating it for every bitmask.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: xfsprogs-dev/repair/incore.h
> ===================================================================
> --- xfsprogs-dev.orig/repair/incore.h 2011-12-02 11:04:59.000000000 +0000
> +++ xfsprogs-dev/repair/incore.h      2011-12-02 11:17:31.000000000 +0000
> @@ -289,6 +289,9 @@ typedef struct ino_tree_node  {
>       } ino_un;
>  } ino_tree_node_t;
>  
> +#define INOS_PER_IREC        (sizeof(__uint64_t) * NBBY)
> +#define      IREC_MASK(i)    ((__uint64_t)1 << (i))
> +
>  typedef struct nlink_ops {
>       const int       nlink_size;
>       void            (*disk_nlink_set)(ino_tree_node_t *, int, __uint32_t);
> @@ -299,7 +302,6 @@ typedef struct nlink_ops {
>  } nlink_ops_t;
>  
>  
> -#define INOS_PER_IREC                (sizeof(__uint64_t) * NBBY)
>  void         add_ino_ex_data(xfs_mount_t *mp);
>  
>  /*
> @@ -381,92 +383,73 @@ void                    
> clear_uncertain_ino_cache(xfs_agn
>               ((ino_tree_node_t *) ((ino_node_ptr)->avl_node.avl_forw))
>  
>  /*
> - * Bit manipulations for processed field
> + * Has an inode been processed for phase 6 (reference count checking)?
> + *
> + * add_inode_refchecked() is set on an inode when it gets traversed
> + * during the reference count phase (6).  It's set so that if the inode
> + * is a directory, it's traversed (and it's links counted) only once.
>   */
> -#define      XFS_INOPROC_MASK(i)     ((__uint64_t)1 << (i))
> -#define      XFS_INOPROC_MASKN(i,n)  ((__uint64_t)((1 << (n)) - 1) << (i))
> +static inline void add_inode_refchecked(struct ino_tree_node *irec, int 
> offset)
> +{
> +     irec->ino_un.ex_data->ino_processed |= IREC_MASK(offset);
> +}
>  
> -#define      XFS_INOPROC_IS_PROC(rp, i) \
> -     (((rp)->ino_un.ex_data->ino_processed & XFS_INOPROC_MASK((i))) == 0LL \
> -             ? 0 : 1)
> -#define      XFS_INOPROC_SET_PROC(rp, i) \
> -     ((rp)->ino_un.ex_data->ino_processed |= XFS_INOPROC_MASK((i)))
> -/*
> -#define      XFS_INOPROC_CLR_PROC(rp, i) \
> -     ((rp)->ino_un.ex_data->ino_processed &= ~XFS_INOPROC_MASK((i)))
> -*/
> +static inline int is_inode_refchecked(struct ino_tree_node *irec, int offset)
> +{
> +     return (irec->ino_un.ex_data->ino_processed & IREC_MASK(offset)) != 0;
> +}
>  
>  /*
> - * same for ir_confirmed.
> + * set/test is inode known to be valid (although perhaps corrupt)
>   */
> -#define      XFS_INOCF_MASK(i)       ((__uint64_t)1 << (i))
> -#define      XFS_INOCF_MASKN(i,n)    ((__uint64_t)((1 << (n)) - 1) << (i))
> -
> -#define      XFS_INOCF_IS_CF(rp, i) \
> -             (((rp)->ino_confirmed & XFS_INOCF_MASK((i))) == 0LL \
> -                     ? 0 : 1)
> -#define      XFS_INOCF_SET_CF(rp, i) \
> -                     ((rp)->ino_confirmed |= XFS_INOCF_MASK((i)))
> -#define      XFS_INOCF_CLR_CF(rp, i) \
> -                     ((rp)->ino_confirmed &= ~XFS_INOCF_MASK((i)))
> +static inline void set_inode_confirmed(struct ino_tree_node *irec, int 
> offset)
> +{
> +     irec->ino_confirmed |= IREC_MASK(offset);
> +}
>  
> -/*
> - * same for backptr->ino_reached
> - */
> -#define      XFS_INO_RCHD_MASK(i)    ((__uint64_t)1 << (i))
> +static inline int is_inode_confirmed(struct ino_tree_node *irec, int offset)
> +{
> +     return (irec->ino_confirmed & IREC_MASK(offset)) != 0;
> +}
>  
> -#define      XFS_INO_RCHD_IS_RCHD(rp, i) \
> -     (((rp)->ino_un.ex_data->ino_reached & XFS_INO_RCHD_MASK((i))) == 0LL \
> -             ? 0 : 1)
> -#define      XFS_INO_RCHD_SET_RCHD(rp, i) \
> -             ((rp)->ino_un.ex_data->ino_reached |= XFS_INO_RCHD_MASK((i)))
> -#define      XFS_INO_RCHD_CLR_RCHD(rp, i) \
> -             ((rp)->ino_un.ex_data->ino_reached &= ~XFS_INO_RCHD_MASK((i)))
>  /*
>   * set/clear/test is inode a directory inode
>   */
> -#define      XFS_INO_ISADIR_MASK(i)  ((__uint64_t)1 << (i))
> -
> -#define inode_isadir(ino_rec, ino_offset) \
> -     (((ino_rec)->ino_isa_dir & XFS_INO_ISADIR_MASK((ino_offset))) == 0LL \
> -             ? 0 : 1)
> -#define set_inode_isadir(ino_rec, ino_offset) \
> -             ((ino_rec)->ino_isa_dir |= XFS_INO_ISADIR_MASK((ino_offset)))
> -#define clear_inode_isadir(ino_rec, ino_offset) \
> -             ((ino_rec)->ino_isa_dir &= ~XFS_INO_ISADIR_MASK((ino_offset)))
> -
> -
> -/*
> - * set/clear/test is inode known to be valid (although perhaps corrupt)
> - */
> -#define clear_inode_confirmed(ino_rec, ino_offset) \
> -                     XFS_INOCF_CLR_CF((ino_rec), (ino_offset))
> +static inline void set_inode_isadir(struct ino_tree_node *irec, int offset)
> +{
> +     irec->ino_isa_dir |= IREC_MASK(offset);
> +}
>  
> -#define set_inode_confirmed(ino_rec, ino_offset) \
> -                     XFS_INOCF_SET_CF((ino_rec), (ino_offset))
> +static inline void clear_inode_isadir(struct ino_tree_node *irec, int offset)
> +{
> +     irec->ino_isa_dir &= ~IREC_MASK(offset);
> +}
>  
> -#define is_inode_confirmed(ino_rec, ino_offset) \
> -                     XFS_INOCF_IS_CF(ino_rec, ino_offset)
> +static inline int inode_isadir(struct ino_tree_node *irec, int offset)
> +{
> +     return (irec->ino_isa_dir & IREC_MASK(offset)) != 0;
> +}
>  
>  /*
>   * set/clear/test is inode free or used
>   */
> -#define set_inode_free(ino_rec, ino_offset) \
> -     XFS_INOCF_SET_CF((ino_rec), (ino_offset)), \
> -     XFS_INOBT_SET_FREE((ino_rec), (ino_offset))
> -
> -#define set_inode_used(ino_rec, ino_offset) \
> -     XFS_INOCF_SET_CF((ino_rec), (ino_offset)), \
> -     XFS_INOBT_CLR_FREE((ino_rec), (ino_offset))
> +static inline void set_inode_free(struct ino_tree_node *irec, int offset)
> +{
> +     set_inode_confirmed(irec, offset);
> +     irec->ir_free |= XFS_INOBT_MASK(offset);
>  
> -#define XFS_INOBT_IS_FREE(ino_rec, ino_offset) \
> -     (((ino_rec)->ir_free & XFS_INOBT_MASK(ino_offset)) != 0)
> +}
>  
> -#define is_inode_used(ino_rec, ino_offset)   \
> -     !XFS_INOBT_IS_FREE((ino_rec), (ino_offset))
> +static inline void set_inode_used(struct ino_tree_node *irec, int offset)
> +{
> +     set_inode_confirmed(irec, offset);
> +     irec->ir_free &= ~XFS_INOBT_MASK(offset);
> +}
>  
> -#define is_inode_free(ino_rec, ino_offset)   \
> -     XFS_INOBT_IS_FREE((ino_rec), (ino_offset))
> +static inline int is_inode_free(struct ino_tree_node *irec, int offset)
> +{
> +     return (irec->ir_free & XFS_INOBT_MASK(offset)) != 0;
> +}
>  
>  /*
>   * add_inode_reached() is set on inode I only if I has been reached
> @@ -478,88 +461,54 @@ void                    
> clear_uncertain_ino_cache(xfs_agn
>   * an inode that we've counted is removed.
>   */
>  
> -static inline int
> -is_inode_reached(ino_tree_node_t *ino_rec, int ino_offset)
> -{
> -     ASSERT(ino_rec->ino_un.ex_data != NULL);
> -     return(XFS_INO_RCHD_IS_RCHD(ino_rec, ino_offset));
> -}
> -
> -static inline void
> -add_inode_reached(ino_tree_node_t *ino_rec, int ino_offset)
> +static inline void add_inode_ref(struct ino_tree_node *irec, int offset)
>  {
> -     ASSERT(ino_rec->ino_un.ex_data != NULL);
> -
> -     (*ino_rec->nlinkops->counted_nlink_inc)(ino_rec, ino_offset);
> -     XFS_INO_RCHD_SET_RCHD(ino_rec, ino_offset);
> +     ASSERT(irec->ino_un.ex_data != NULL);
>  
> -     ASSERT(is_inode_reached(ino_rec, ino_offset));
> +     irec->nlinkops->counted_nlink_inc(irec, offset);
>  }
>  
> -static inline void
> -add_inode_ref(ino_tree_node_t *ino_rec, int ino_offset)
> +static inline void drop_inode_ref(struct ino_tree_node *irec, int offset)
>  {
> -     ASSERT(ino_rec->ino_un.ex_data != NULL);
> +     ASSERT(irec->ino_un.ex_data != NULL);
>  
> -     (*ino_rec->nlinkops->counted_nlink_inc)(ino_rec, ino_offset);
> +     if (irec->nlinkops->counted_nlink_dec(irec, offset) == 0)
> +             irec->ino_un.ex_data->ino_reached &= ~IREC_MASK(offset);
>  }
>  
> -static inline void
> -drop_inode_ref(ino_tree_node_t *ino_rec, int ino_offset)
> +static inline __uint32_t num_inode_references(struct ino_tree_node *irec,
> +             int offset)
>  {
> -     ASSERT(ino_rec->ino_un.ex_data != NULL);
> +     ASSERT(irec->ino_un.ex_data != NULL);
>  
> -     if ((*ino_rec->nlinkops->counted_nlink_dec)(ino_rec, ino_offset) == 0)
> -             XFS_INO_RCHD_CLR_RCHD(ino_rec, ino_offset);
> +     return irec->nlinkops->counted_nlink_get(irec, offset);
>  }
>  
> -static inline int
> -is_inode_referenced(ino_tree_node_t *ino_rec, int ino_offset)
> +static inline int is_inode_reached(struct ino_tree_node *irec, int offset)
>  {
> -     ASSERT(ino_rec->ino_un.ex_data != NULL);
> -
> -     return (*ino_rec->nlinkops->counted_nlink_get)(ino_rec, ino_offset) > 0;
> +     ASSERT(irec->ino_un.ex_data != NULL);
> +     return (irec->ino_un.ex_data->ino_reached & IREC_MASK(offset)) != 0;
>  }
>  
> -static inline __uint32_t
> -num_inode_references(ino_tree_node_t *ino_rec, int ino_offset)
> +static inline void add_inode_reached(struct ino_tree_node *irec, int offset)
>  {
> -     ASSERT(ino_rec->ino_un.ex_data != NULL);
> -
> -     return (*ino_rec->nlinkops->counted_nlink_get)(ino_rec, ino_offset);
> +     add_inode_ref(irec, offset);
> +     irec->ino_un.ex_data->ino_reached |= IREC_MASK(offset);
>  }
>  
> -static inline void
> -set_inode_disk_nlinks(ino_tree_node_t *ino_rec, int ino_offset, __uint32_t 
> nlinks)
> +static inline void set_inode_disk_nlinks(struct ino_tree_node *irec, int 
> offset,
> +             __uint32_t nlinks)
>  {
> -     (*ino_rec->nlinkops->disk_nlink_set)(ino_rec, ino_offset, nlinks);
> +     irec->nlinkops->disk_nlink_set(irec, offset, nlinks);
>  }
>  
> -static inline __uint32_t
> -get_inode_disk_nlinks(ino_tree_node_t *ino_rec, int ino_offset)
> +static inline __uint32_t get_inode_disk_nlinks(struct ino_tree_node *irec,
> +             int offset)
>  {
> -     return (*ino_rec->nlinkops->disk_nlink_get)(ino_rec, ino_offset);
> +     return irec->nlinkops->disk_nlink_get(irec, offset);
>  }
>  
>  /*
> - * has an inode been processed for phase 6 (reference count checking)?
> - * add_inode_refchecked() is set on an inode when it gets traversed
> - * during the reference count phase (6).  It's set so that if the inode
> - * is a directory, it's traversed (and it's links counted) only once.
> - */
> -#ifndef XR_INO_REF_DEBUG
> -#define add_inode_refchecked(ino, ino_rec, ino_offset) \
> -             XFS_INOPROC_SET_PROC((ino_rec), (ino_offset))
> -#define is_inode_refchecked(ino, ino_rec, ino_offset) \
> -             (XFS_INOPROC_IS_PROC(ino_rec, ino_offset) != 0LL)
> -#else
> -void add_inode_refchecked(xfs_ino_t ino,
> -                     ino_tree_node_t *ino_rec, int ino_offset);
> -int is_inode_refchecked(xfs_ino_t ino,
> -                     ino_tree_node_t *ino_rec, int ino_offset);
> -#endif /* XR_INO_REF_DEBUG */
> -
> -/*
>   * set/get inode number of parent -- works for directory inodes only
>   */
>  void         set_inode_parent(ino_tree_node_t *irec, int ino_offset,
> Index: xfsprogs-dev/repair/incore_ino.c
> ===================================================================
> --- xfsprogs-dev.orig/repair/incore_ino.c     2011-12-02 11:04:59.000000000 
> +0000
> +++ xfsprogs-dev/repair/incore_ino.c  2011-12-02 11:05:19.000000000 +0000
> @@ -784,19 +784,3 @@ incore_ino_init(xfs_mount_t *mp)
>  
>       full_ino_ex_data = 0;
>  }
> -
> -#ifdef XR_INO_REF_DEBUG
> -void
> -add_inode_refchecked(xfs_ino_t ino, ino_tree_node_t *ino_rec, int ino_offset)
> -{
> -     XFS_INOPROC_SET_PROC((ino_rec), (ino_offset));
> -
> -     ASSERT(is_inode_refchecked(ino, ino_rec, ino_offset));
> -}
> -
> -int
> -is_inode_refchecked(xfs_ino_t ino, ino_tree_node_t *ino_rec, int ino_offset)
> -{
> -     return(XFS_INOPROC_IS_PROC(ino_rec, ino_offset) == 0LL ? 0 : 1);
> -}
> -#endif /* XR_INO_REF_DEBUG */
> Index: xfsprogs-dev/include/libxfs.h
> ===================================================================
> --- xfsprogs-dev.orig/include/libxfs.h        2011-12-02 11:04:59.000000000 
> +0000
> +++ xfsprogs-dev/include/libxfs.h     2011-12-02 11:05:19.000000000 +0000
> @@ -503,8 +503,6 @@ extern unsigned long      libxfs_physmem(void
>  #include <xfs/xfs_log.h>
>  #include <xfs/xfs_log_priv.h>
>  
> -#define XFS_INOBT_CLR_FREE(rp,i)     ((rp)->ir_free &= ~XFS_INOBT_MASK(i))
> -#define XFS_INOBT_SET_FREE(rp,i)     ((rp)->ir_free |= XFS_INOBT_MASK(i))
>  #define XFS_INOBT_IS_FREE_DISK(rp,i)         \
>                       ((be64_to_cpu((rp)->ir_free) & XFS_INOBT_MASK(i)) != 0)
>  
> Index: xfsprogs-dev/repair/phase6.c
> ===================================================================
> --- xfsprogs-dev.orig/repair/phase6.c 2011-12-02 11:04:59.000000000 +0000
> +++ xfsprogs-dev/repair/phase6.c      2011-12-02 11:05:19.000000000 +0000
> @@ -3258,7 +3258,7 @@ process_dir_inode(
>        * remaining illegal directory entries.
>        */
>  
> -     ASSERT(!is_inode_refchecked(ino, irec, ino_offset) || dotdot_update);
> +     ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
>  
>       error = libxfs_iget(mp, NULL, ino, 0, &ip, 0);
>       if (error) {
> @@ -3282,7 +3282,7 @@ process_dir_inode(
>                       }
>               }
>  
> -             add_inode_refchecked(ino, irec, 0);
> +             add_inode_refchecked(irec, 0);
>               return;
>       }
>  
> @@ -3300,7 +3300,7 @@ process_dir_inode(
>               add_inode_reached(irec, ino_offset);
>       }
>  
> -     add_inode_refchecked(ino, irec, ino_offset);
> +     add_inode_refchecked(irec, ino_offset);
>  
>       hashtab = dir_hash_init(ip->i_d.di_size);
>  
> Index: xfsprogs-dev/repair/phase7.c
> ===================================================================
> --- xfsprogs-dev.orig/repair/phase7.c 2011-12-02 11:16:58.000000000 +0000
> +++ xfsprogs-dev/repair/phase7.c      2011-12-02 11:17:27.000000000 +0000
> @@ -143,10 +143,9 @@ phase7(xfs_mount_t *mp)
>                                       continue;
>  
>                               ASSERT(no_modify || is_inode_reached(irec, j));
> -                             ASSERT(no_modify ||
> -                                             is_inode_referenced(irec, j));
>  
>                               nrefs = num_inode_references(irec, j);
> +                             ASSERT(no_modify || nrefs > 0);
>  
>                               if (get_inode_disk_nlinks(irec, j) != nrefs)
>                                       update_inode_nlinks(mp,
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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