Re: [PATCH 4/4] xfs: V5 filesystem format support

[Andrei Borzenkov]

В Mon, 14 Jul 2014 17:21:31 +0200
Jan Kara <address@hidden> пишет:

> Add support for new XFS on disk format. We have to handle optional
> filetype fields in directory entries, additional CRC, LSN, UUID entries
> in some structures, etc.
> 
> Signed-off-by: Jan Kara <address@hidden>
> ---
>  grub-core/fs/xfs.c | 249 
> +++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 177 insertions(+), 72 deletions(-)
> 
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 7e247a32df5c..7f2582d33e02 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -34,6 +34,15 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define XFS_INODE_FORMAT_EXT 2
>  #define XFS_INODE_FORMAT_BTREE       3
>  
> +/* Superblock version field flags */
> +#define XFS_SB_VERSION_NUMBITS          0x000f
> +#define XFS_SB_VERSION_MOREBITSBIT      0x8000
> +
> +/* features2 field flags */
> +#define XFS_SB_VERSION2_FTYPE           0x00000200      /* inode type in dir 
> */
> +
> +/* incompat feature flags */
> +#define XFS_SB_FEAT_INCOMPAT_FTYPE      (1 << 0)        /* filetype in 
> dirent */
>  
>  struct grub_xfs_sblock
>  {
> @@ -45,7 +54,9 @@ struct grub_xfs_sblock
>    grub_uint64_t rootino;
>    grub_uint8_t unused3[20];
>    grub_uint32_t agsize;
> -  grub_uint8_t unused4[20];
> +  grub_uint8_t unused4[12];
> +  grub_uint16_t version;
> +  grub_uint8_t unused5[6];
>    grub_uint8_t label[12];
>    grub_uint8_t log2_bsize;
>    grub_uint8_t log2_sect;
> @@ -54,12 +65,19 @@ struct grub_xfs_sblock
>    grub_uint8_t log2_agblk;
>    grub_uint8_t unused6[67];
>    grub_uint8_t log2_dirblk;
> +  grub_uint8_t unused7[7];
> +  grub_uint32_t features2;
> +  grub_uint8_t unused8[4];
> +  grub_uint32_t sb_features_compat;
> +  grub_uint32_t sb_features_ro_compat;
> +  grub_uint32_t sb_features_incompat;
> +  grub_uint32_t sb_features_log_incompat;
>  } GRUB_PACKED;
>  
>  struct grub_xfs_dir_header
>  {
>    grub_uint8_t count;
> -  grub_uint8_t smallino;
> +  grub_uint8_t largeino;
>    union
>    {
>      grub_uint32_t i4;
> @@ -67,14 +85,16 @@ struct grub_xfs_dir_header
>    } GRUB_PACKED parent;
>  } GRUB_PACKED;
>  
> +/* Structure for directory entry inlined in the inode */
>  struct grub_xfs_dir_entry
>  {
>    grub_uint8_t len;
>    grub_uint16_t offset;
>    char name[1];
> -  /* Inode number follows, 32 bits.  */
> +  /* Inode number follows, 32 / 64 bits.  */
>  } GRUB_PACKED;
>  
> +/* Structure for directory entry in a block */
>  struct grub_xfs_dir2_entry
>  {
>    grub_uint64_t inode;
> @@ -90,7 +110,8 @@ struct grub_xfs_btree_node
>    grub_uint16_t numrecs;
>    grub_uint64_t left;
>    grub_uint64_t right;
> -  grub_uint64_t keys[1];
> +  /* In V5 here follow crc, uuid, etc. */
> +  /* Then follow keys and block pointers */
>  }  GRUB_PACKED;
>  
>  struct grub_xfs_btree_root
> @@ -123,17 +144,6 @@ struct grub_xfs_inode
>    grub_uint16_t unused3;
>    grub_uint8_t fork_offset;
>    grub_uint8_t unused4[17];
> -  union
> -  {
> -    char raw[156];
> -    struct dir
> -    {
> -      struct grub_xfs_dir_header dirhead;
> -      struct grub_xfs_dir_entry direntry[1];
> -    } dir;
> -    grub_xfs_extent extents[XFS_INODE_EXTENTS];
> -    struct grub_xfs_btree_root btree;
> -  } GRUB_PACKED data;

Can we make it grub_uint8_t xfs_v3_extra[76] or similar to avoid magic
numbers later when computing data offset?

>  } GRUB_PACKED;
>  
>  struct grub_xfs_dirblock_tail
> @@ -157,6 +167,8 @@ struct grub_xfs_data
>    int pos;
>    int bsize;
>    grub_uint32_t agsize;
> +  unsigned int hasftype:1;
> +  unsigned int hascrc:1;
>    struct grub_fshelp_node diropen;
>  };
>  
> @@ -164,6 +176,24 @@ static grub_dl_t my_mod;
>  
>  
>  
> +static int grub_xfs_sb_hascrc(struct grub_xfs_data *data)
> +{
> +  return (grub_be_to_cpu16(data->sblock.version) & XFS_SB_VERSION_NUMBITS) 
> == 5;

Could you define name for magic constant?

Also to avoid run-time computation:
data->sblock.version &
grub_cpu_to_b16_compile_time(XFS_SB_VERSION_NUMBITS) ==
grub_cpu_to_b16_compile_time(5)

> +}
> +
> +static int grub_xfs_sb_hasftype(struct grub_xfs_data *data)
> +{
> +  grub_uint32_t version = grub_be_to_cpu16(data->sblock.version);
> +
> +  if ((version & XFS_SB_VERSION_NUMBITS) == 5 &&
> +      grub_be_to_cpu32(data->sblock.sb_features_incompat) & 
> XFS_SB_FEAT_INCOMPAT_FTYPE)
> +    return 1;
> +  if (version & XFS_SB_VERSION_MOREBITSBIT &&
> +      grub_be_to_cpu32(data->sblock.features2) & XFS_SB_VERSION2_FTYPE)
> +    return 1;
> +  return 0;
> +}
> +

Same. All are constants, so *_compile_time.

>  /* Filetype information as used in inodes.  */
>  #define FILETYPE_INO_MASK    0170000
>  #define FILETYPE_INO_REG     0100000
> @@ -219,18 +249,6 @@ GRUB_XFS_EXTENT_SIZE (grub_xfs_extent *exts, int ex)
>    return (grub_be_to_cpu32 (exts[ex][3]) & ((1 << 21) - 1));
>  }
>  
> -static inline int
> -GRUB_XFS_ROUND_TO_DIRENT (int pos)
> -{
> -  return ((((pos) + 8 - 1) / 8) * 8);
> -}
> -
> -static inline int
> -GRUB_XFS_NEXT_DIRENT (int pos, int len)
> -{
> -  return (pos) + GRUB_XFS_ROUND_TO_DIRENT (8 + 1 + len + 2);
> -}
> -
>  
>  static inline grub_uint64_t
>  grub_xfs_inode_block (struct grub_xfs_data *data,
> @@ -261,6 +279,96 @@ grub_xfs_inode_size(struct grub_xfs_data *data)
>       return 1 << data->sblock.log2_inode;
>  }
>  
> +static void *
> +grub_xfs_inode_data(struct grub_xfs_inode *inode)
> +{
> +     if (inode->version <= 2)
> +             return ((char *)inode) + 100;

That is sizeof(struct grub_xfs_inode) in your patch, right?

> +     return ((char *)inode) + 176;

Please avoid magic numbers. At least give it meaningful name, or as
suggested above just define structure to contain it.

> +}
> +
> +static struct grub_xfs_dir_entry *
> +grub_xfs_inline_de(struct grub_xfs_dir_header *head)
> +{
> +     /*
> +      * With small inode numbers the header is 4 bytes smaller because of
> +      * smaller parent pointer
> +      */
> +     return (void *)(((char *)head) + sizeof(struct grub_xfs_dir_header) -
> +             (head->largeino ? 0 : sizeof(grub_uint32_t)));
> +}
> +
> +static grub_uint8_t *
> +grub_xfs_inline_de_inopos(struct grub_xfs_data *data,
> +                       struct grub_xfs_dir_entry *de)
> +{
> +     return ((grub_uint8_t *)(de + 1)) + de->len - 1 +
The outer parens are somehow confusing.

> +              (data->hasftype ? 1 : 0);
> +}
> +
> +static struct grub_xfs_dir_entry *
> +grub_xfs_inline_next_de(struct grub_xfs_data *data,
> +                     struct grub_xfs_dir_header *head,
> +                     struct grub_xfs_dir_entry *de)
> +{
> +  char *p = (char *)de + sizeof(struct grub_xfs_dir_entry) - 1 + de->len;
> +
> +  p += head->largeino ? sizeof(grub_uint64_t) : sizeof(grub_uint32_t);
> +  if (data->hasftype)
> +    p++;
> +       
> +  return (struct grub_xfs_dir_entry *)p;
> +}
> +
> +static struct grub_xfs_dirblock_tail *
> +grub_xfs_dir_tail(struct grub_xfs_data *data, void *dirblock)
> +{
> +  int dirblksize = 1 << (data->sblock.log2_bsize + data->sblock.log2_dirblk);
> +
> +  return (struct grub_xfs_dirblock_tail *)
> +    ((char *)dirblock + dirblksize - sizeof (struct grub_xfs_dirblock_tail));
> +}
> +
> +static struct grub_xfs_dir2_entry *
> +grub_xfs_first_de(struct grub_xfs_data *data, void *dirblock)
> +{
> +  if (data->hascrc)
> +    return (struct grub_xfs_dir2_entry *)((char *)dirblock + 64);
> +  return (struct grub_xfs_dir2_entry *)((char *)dirblock + 16);
> +}
> +
> +static inline int
> +grub_xfs_round_dirent_size (int len)
> +{
> +  return (len + 7) & ~7;
> +}

ALIGN_UP?

> +
> +static struct grub_xfs_dir2_entry *
> +grub_xfs_next_de(struct grub_xfs_data *data, struct grub_xfs_dir2_entry *de)
> +{
> +  int size = sizeof (struct grub_xfs_dir2_entry) + de->len + 2 /* Tag */;
> +
> +  if (data->hasftype)
> +    size++;          /* File type */
> +  return (struct grub_xfs_dir2_entry *)
> +                     (((char *)de) + grub_xfs_round_dirent_size (size));

What's wrong with ALIGN_UP (size, 8)?

> +}
> +
> +static grub_uint64_t *
> +grub_xfs_btree_keys(struct grub_xfs_data *data,
> +                 struct grub_xfs_btree_node *leaf)
> +{
> +  char *p = (char *)(leaf + 1);
> +
> +  if (data->hascrc)
> +    p += 48; /* crc, uuid, ... */
> +  /*
> +   * We have to first type to void * to avoid complaints about possible
> +   * alignment issues on some architectures
> +   */
> +  return (grub_uint64_t *)(void *)p;


Leaving it as grub_uint64_t keys and using &keys[6] would avoid this
warning as well, not? Also having keys[0] will likely simplify other
places as well (we do require GCC in any case).

> +}
> +
>  static grub_err_t
>  grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
>                    struct grub_xfs_inode *inode)
> @@ -268,6 +376,9 @@ grub_xfs_read_inode (struct grub_xfs_data *data, 
> grub_uint64_t ino,
>    grub_uint64_t block = grub_xfs_inode_block (data, ino);
>    int offset = grub_xfs_inode_offset (data, ino);
>  
> +  grub_dprintf("xfs", "Reading inode (%llu) - %llu, %d\n",
> +            (unsigned long long) ino,
> +            (unsigned long long) block, offset);
>    /* Read the inode.  */
>    if (grub_disk_read (data->disk, block, offset, grub_xfs_inode_size(data),
>                     inode))
> @@ -290,6 +401,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, 
> grub_disk_addr_t fileblock)
>  
>    if (node->inode.format == XFS_INODE_FORMAT_BTREE)
>      {
> +      struct grub_xfs_btree_root *root;
>        const grub_uint64_t *keys;
>        int recoffset;
>  
> @@ -297,15 +409,15 @@ grub_xfs_read_block (grub_fshelp_node_t node, 
> grub_disk_addr_t fileblock)
>        if (leaf == 0)
>          return 0;
>  
> -      nrec = grub_be_to_cpu16 (node->inode.data.btree.numrecs);
> -      keys = &node->inode.data.btree.keys[0];
> +      root = grub_xfs_inode_data(&node->inode);
> +      nrec = grub_be_to_cpu16 (root->numrecs);
> +      keys = &root->keys[0];
>        if (node->inode.fork_offset)
>       recoffset = (node->inode.fork_offset - 1) / 2;
>        else
>       recoffset = (grub_xfs_inode_size(node->data)
> -                  - ((char *) &node->inode.data.btree.keys
> -                     - (char *) &node->inode))
> -       / (2 * sizeof (grub_uint64_t));
> +                  - ((char *) keys - (char *) &node->inode))
> +                             / (2 * sizeof (grub_uint64_t));
>        do
>          {
>            int i;
> @@ -327,7 +439,10 @@ grub_xfs_read_block (grub_fshelp_node_t node, 
> grub_disk_addr_t fileblock)
>                                0, node->data->bsize, leaf))
>              return 0;
>  
> -          if (grub_strncmp ((char *) leaf->magic, "BMAP", 4))
> +       if ((!node->data->hascrc &&
> +            grub_strncmp ((char *) leaf->magic, "BMAP", 4)) ||
> +           (node->data->hascrc &&
> +            grub_strncmp ((char *) leaf->magic, "BMA3", 4)))
>              {
>                grub_free (leaf);
>                grub_error (GRUB_ERR_BAD_FS, "not a correct XFS BMAP node");
> @@ -335,8 +450,8 @@ grub_xfs_read_block (grub_fshelp_node_t node, 
> grub_disk_addr_t fileblock)
>              }
>  
>            nrec = grub_be_to_cpu16 (leaf->numrecs);
> -          keys = &leaf->keys[0];
> -       recoffset = ((node->data->bsize - ((char *) &leaf->keys
> +          keys = grub_xfs_btree_keys(node->data, leaf);
> +       recoffset = ((node->data->bsize - ((char *) keys
>                                            - (char *) leaf))
>                      / (2 * sizeof (grub_uint64_t)));
>       }
> @@ -346,7 +461,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, 
> grub_disk_addr_t fileblock)
>    else if (node->inode.format == XFS_INODE_FORMAT_EXT)
>      {
>        nrec = grub_be_to_cpu32 (node->inode.nextents);
> -      exts = &node->inode.data.extents[0];
> +      exts = grub_xfs_inode_data(&node->inode);
>      }
>    else
>      {
> @@ -404,7 +519,7 @@ grub_xfs_read_symlink (grub_fshelp_node_t node)
>    switch (node->inode.format)
>      {
>      case XFS_INODE_FORMAT_INO:
> -      return grub_strndup (node->inode.data.raw, size);
> +      return grub_strndup (grub_xfs_inode_data(&node->inode), size);
>  
>      case XFS_INODE_FORMAT_EXT:
>        {
> @@ -501,23 +616,18 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>      {
>      case XFS_INODE_FORMAT_INO:
>        {
> -     struct grub_xfs_dir_entry *de = &diro->inode.data.dir.direntry[0];
> -     int smallino = !diro->inode.data.dir.dirhead.smallino;
> +     struct grub_xfs_dir_header *head = grub_xfs_inode_data(&diro->inode);
> +     struct grub_xfs_dir_entry *de = grub_xfs_inline_de(head);
> +     int smallino = !head->largeino;
>       int i;
>       grub_uint64_t parent;
>  
>       /* If small inode numbers are used to pack the direntry, the
>          parent inode number is small too.  */
>       if (smallino)
> -       {
> -         parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent.i4);
> -         /* The header is a bit smaller than usual.  */
> -         de = (struct grub_xfs_dir_entry *) ((char *) de - 4);
> -       }
> +       parent = grub_be_to_cpu32 (head->parent.i4);
>       else
> -       {
> -         parent = grub_be_to_cpu64(diro->inode.data.dir.dirhead.parent.i8);
> -       }
> +       parent = grub_be_to_cpu64 (head->parent.i8);
>  
>       /* Synthesize the direntries for `.' and `..'.  */
>       if (iterate_dir_call_hook (diro->ino, ".", &ctx))
> @@ -526,12 +636,10 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>       if (iterate_dir_call_hook (parent, "..", &ctx))
>         return 1;
>  
> -     for (i = 0; i < diro->inode.data.dir.dirhead.count; i++)
> +     for (i = 0; i < head->count; i++)
>         {
>           grub_uint64_t ino;
> -         grub_uint8_t *inopos = (((grub_uint8_t *) de)
> -                         + sizeof (struct grub_xfs_dir_entry)
> -                         + de->len - 1);
> +         grub_uint8_t *inopos = grub_xfs_inline_de_inopos(dir->data, de);
>           grub_uint8_t c;
>  
>           /* inopos might be unaligned.  */
> @@ -556,10 +664,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>             return 1;
>           de->name[de->len] = c;
>  
> -         de = ((struct grub_xfs_dir_entry *)
> -               (((char *) de)+ sizeof (struct grub_xfs_dir_entry) + de->len
> -                + ((smallino ? sizeof (grub_uint32_t)
> -                    : sizeof (grub_uint64_t))) - 1));
> +         de = grub_xfs_inline_next_de(dir->data, head, de);
>         }
>       break;
>        }
> @@ -586,15 +691,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>                   >> dirblk_log2);
>            blk++)
>         {
> -         /* The header is skipped, the first direntry is stored
> -            from byte 16.  */
> -         int pos = 16;
> +         struct grub_xfs_dir2_entry *direntry =
> +                                     grub_xfs_first_de(dir->data, dirblock);
>           int entries;
> -         int tail_start = (dirblk_size
> -                           - sizeof (struct grub_xfs_dirblock_tail));
> -
> -         struct grub_xfs_dirblock_tail *tail;
> -         tail = (struct grub_xfs_dirblock_tail *) &dirblock[tail_start];
> +         struct grub_xfs_dirblock_tail *tail =
> +                                     grub_xfs_dir_tail(dir->data, dirblock);
>  
>           numread = grub_xfs_read_file (dir, 0, 0,
>                                         blk << dirblk_log2,
> @@ -606,13 +707,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>                      - grub_be_to_cpu32 (tail->leaf_stale));
>  
>           /* Iterate over all entries within this block.  */
> -         while (pos < tail_start)
> +         while ((char *)direntry < (char *)tail)
>             {
> -             struct grub_xfs_dir2_entry *direntry;
>               grub_uint8_t *freetag;
>               char *filename;
>  
> -             direntry = (struct grub_xfs_dir2_entry *) &dirblock[pos];
>               freetag = (grub_uint8_t *) direntry;
>  
>               if (grub_get_unaligned16 (freetag) == 0XFFFF)
> @@ -620,14 +719,16 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>                   grub_uint8_t *skip = (freetag + sizeof (grub_uint16_t));
>  
>                   /* This entry is not used, go to the next one.  */
> -                 pos += grub_be_to_cpu16 (grub_get_unaligned16 (skip));
> +                 direntry = (struct grub_xfs_dir2_entry *)
> +                             (((char *)direntry) +
> +                             grub_be_to_cpu16 (grub_get_unaligned16 (skip)));
>  
>                   continue;
>                 }
>  
> -             filename = &dirblock[pos + sizeof (*direntry)];
> -             /* The byte after the filename is for the tag, which
> -                is not used by GRUB.  So it can be overwritten.  */
> +             filename = (char *)(direntry + 1);
> +             /* The byte after the filename is for the filetype, padding, or
> +                tag, which is not used by GRUB.  So it can be overwritten. */
>               filename[direntry->len] = '\0';
>  
>               if (iterate_dir_call_hook (grub_be_to_cpu64(direntry->inode), 
> @@ -644,8 +745,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>                 break;
>  
>               /* Select the next directory entry.  */
> -             pos = GRUB_XFS_NEXT_DIRENT (pos, direntry->len);
> -             pos = GRUB_XFS_ROUND_TO_DIRENT (pos);
> +             direntry = grub_xfs_next_de(dir->data, direntry);
>             }
>         }
>       grub_free (dirblock);
> @@ -670,6 +770,7 @@ grub_xfs_mount (grub_disk_t disk)
>    if (!data)
>      return 0;
>  
> +  grub_dprintf("xfs", "Reading sb\n");
>    /* Read the superblock.  */
>    if (grub_disk_read (disk, 0, 0,
>                     sizeof (struct grub_xfs_sblock), &data->sblock))
> @@ -697,9 +798,13 @@ grub_xfs_mount (grub_disk_t disk)
>    data->diropen.inode_read = 1;
>    data->bsize = grub_be_to_cpu32 (data->sblock.bsize);
>    data->agsize = grub_be_to_cpu32 (data->sblock.agsize);
> +  data->hasftype = grub_xfs_sb_hasftype(data);
> +  data->hascrc = grub_xfs_sb_hascrc(data);
>  
>    data->disk = disk;
>    data->pos = 0;
> +  grub_dprintf("xfs", "Reading root ino %llu\n",
> +            (unsigned long long) grub_cpu_to_be64(data->sblock.rootino));
>  
>    grub_xfs_read_inode (data, data->diropen.ino, &data->diropen.inode);
>